-
Notifications
You must be signed in to change notification settings - Fork 244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Neurips client #1693
Neurips client #1693
Conversation
cache_config: CacheConfig, | ||
base_url: str = "http://localhost", | ||
port: int = 8080, | ||
timeout: int = 10, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this timeout a constraint for the challenge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@drisspg can we increase the default timeout? 10 seconds is too small for: toy-submission but using 7B LLaMA2, on 4090, for GSM scenario. I believe that is within the specs of the competition you're running? :)
Hey @yifanmai do you think it would be worthwhile to merge something like this in to the main repo? I can re name to http client and rebase cc @msaroufim |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this PR can be merged soon.
After merging this, I'd want to move to something like what #1761 is doing (see the PR description for an example) where a user would specify model_deployments.yaml
and then run helm-run --model-deployments-paths model_deployments.yaml ...
This would ensure that each submitter's model is named something unique when you combine the benchmark_output
files from all submitters, rather than having everything be named neurips/local
.
I can send you a follow-up PR next week that does this early next week. I think the things that need to happen are:
- Support instantiating clients from
model_deployments.yaml
that don't need an API key (like this one). - Support configurable window service / tokenizers.
Example: model_deployments.yaml
model_deployments:
- name: efficiencychallenge/model-1
model_name: efficiencychallenge/model-1
tokenizer_name: "huggingface/gpt2"
max_sequence_length: 2048
client_spec:
class_name: "helm.proxy.clients.http_client.HTTPClient"
args:
url: "http://localhost:8000/"
url: "http://localhost:8000/"
do_cache: false
from .tokenizer_service import TokenizerService | ||
|
||
|
||
class NeuripsWindowService(LocalWindowService): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're keeping this, then rename the class (see the comments on HTTPClient
later)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking about this a bit more: if all of the models listed in the rules have a Hugging Face tokenizer, then we can just use HuggingFaceWindowService
for all of them, and not need a new window service.
In fact, we can even delete the tokenize()
method on the client later, because we just do tokenization using the local Hugging Face tokenizer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think though that since this could be more generic than just the comp it could still be useful to have a HTTPModelWindowServce
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe... this is basically a generic LocalWindowService
subclass and doesn't do anything HTTP specific. I think we can keep this in this PR, and when we get window service configuration in, this class will probably disappear in that refactor.
|
||
@property | ||
def end_of_text_token(self) -> str: | ||
return "<|endoftext|>" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the special tokens need to be user-configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I would imagine so, not sure what the best way of specifying that would be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One idea is to introduce tokenizers.yaml
:
tokenizers.yaml
tokenizers:
- name: "efficiencychallenge/tokenizer-1"
client_spec:
class_name: "helm.proxy.clients.http_client.HTTPClient"
args:
url: "http://localhost:8000/"
prefix_token: "<|endoftext|>"
end_of_text_token: "<|endoftext|>"
model_deployments.yaml
model_deployments:
- name: efficiencychallenge/model-1
model_name: efficiencychallenge/model-1
tokenizer_name: "efficiencychallenge/tokenizer-1"
max_sequence_length: 2048
client_spec:
class_name: "helm.proxy.clients.http_client.HTTPClient"
args:
url: "http://localhost:8000/"
do_cache: false
cc @percyliang any thoughts on configurable tokenizers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like configurable tokenizers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I'll draft a PR for that.
In the meantime, I don't think we need to block this PR; we can keep these changes and clean things up later when we have configurable tokenizers.
1f51e71
to
02e5583
Compare
Could you mark this as "Ready for review" (by clicking the button) when it's ready for me to take another look? Thanks! |
@yifanmai Cool I think I addressed everything but the yaml changes which I do think would make this service much more generic and alot more applicable to different situations |
Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I just caught a few minor issues.
0596e32
to
12dcabb
Compare
To test against local server exposed on 8080