-
Notifications
You must be signed in to change notification settings - Fork 276
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
Eval local HF models with flag, add LLaMA and Alpaca #1505
Conversation
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 promising!
@@ -10,6 +10,8 @@ | |||
FULL_FUNCTIONALITY_TEXT_MODEL_TAG, | |||
) | |||
|
|||
# The path where local HuggingFace models should be downloaded or symlinked, e.g. ./helm-models/llama-7b | |||
LOCAL_MODEL_DIR = "./helm-models" |
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.
"./huggingface_models"
?
Reasoning:
_
for consistency withprod_env
andbenchmark_output
huggingface
for clarity that this only supports huggingface format- Don't need
helm
because we are already in a HELM working directory
src/helm/proxy/models.py
Outdated
Model( | ||
group="local", | ||
creator_organization="Meta", | ||
name="local/llama-7b", |
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 these should all be group="huggingface"
and name="huggingface/llama-7b"
for consistency.
I would like this to be meta/llama-7b
but unfortunately the pre-existing code base ties the group to the "client" used to serve the model, which is why we have huggingface/gpt2
instead of openai/gpt2
or together/t5-11b
instead of google/t5-11b
. I would like to make everything be the correct group eventually, but for now we should be consistent with existing conventions. It's also not obvious to me that "local" means "local Hugging Face" as opposed to "local some other framework".
Also unfortunately these model names are annoying to change because we have to do a MongoDB migration every time we change one of these.
cc @percyliang for opinions about naming
I notice that in huggingfaceserver, all the models are loaded in "cuda:0". Since we will run the model locally (maybe with |
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.
What's the status of this?
I know we said we wanted to move fast on adding new models but maybe I should wait for the changes to be made and the PR to be merged before diving deep into this?
@yifanmai what do you think?
elif organization == "local": | ||
# HACK since we want to use the huggingface sqlite file. TODO avoid this | ||
client = HuggingFaceClient(cache_config=cache_config) |
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.
Are we not planning to have other local models than HuggingFace btw?
@@ -28,6 +30,9 @@ class HuggingFaceModelConfig: | |||
|
|||
If None, use the default revision.""" | |||
|
|||
path: Optional[str] = None | |||
"""Local path to the Hugging Face model weights""" |
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 add the default path here: ./huggingface_models
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.
Thanks! I added a comment explaining how this parameter is set and what it means:)
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 don't set it directly to the local path by default (like path: str = ./huggingface_models
) because None
means to load it from the hub instead.
model_name = match.group("model_name") | ||
assert model_name | ||
return HuggingFaceModelConfig( | ||
namespace="local", |
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.
Why don't we put something more explicit? I personally feel like "None" is not super clear. Could we maybe put something like "internal"? "local-model"?
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.
What's the status of this?
I know we said we wanted to move fast on adding new models but maybe I should wait for the changes to be made and the PR to be merged before diving deep into this?
@yifanmai what do you think?
|
||
|
||
def _get_singleton_server(model_config: HuggingFaceModelConfig) -> HuggingFaceServer: | ||
global _servers_lock |
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.
nit: docstring should go after the method definition. See PEP 257
def _get_singleton_server(model_config: HuggingFaceModelConfig) -> HuggingFaceServer:
"""Lookup or create a new HuggingFaceServer that will be shared among all threads.
When --num-threads > 1, multiple threads will attempt to instantiate etc."""
) | ||
else: | ||
raise Exception(f"Unknown HuggingFace model: {model}") | ||
|
||
return self.model_server_instances[model] | ||
else: |
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.
The else is still here...
Thank you for helping get this merged @yifanmai ! |
Is --enable-local-huggingface-models still available in the latest version of Helm? |
Hi, it will be included in v0.2.3, which should be released later this week. |
Addresses #1486 and adds LLaMA and Alpaca
This PR allows you to evaluate any HuggingFace model you have stored on the machine which you are running HELM.
Instructions
You have two options for telling HELM where your model is stored:
--enable-local-huggingface-models <path 1> [<path 2> ...]
./huggingface_models
, which is set by the constantLOCAL_HUGGINGFACE_MODEL_DIR
inhuggingface_model_registry.py
. This only works for the local models that are already listed inmodels.py
since they need to be inALL_MODELS
in order to createRunSpec
s. Currently I've listedhuggingface/llama-7b
,huggingface/alpaca-7b
.Then, you can refer to your model in any run specs as
huggingface/<model_name>
, where<model_name>
either comes frommodels.py
or is the name of the directly containing directory in the path specified with--enable-local-huggingface-models
. For example, if I pass--enable-local-huggingface-models /home/quevedo/llama-13b
, I can refer to this model in a run spec ashuggingface/llama-13b
.Feedback request
Making the command line option optional led to some messy code, Is there any way we can avoid having to recreate theNow we pre-register local models fromHuggingFaceModelConfig
multiple times throughout the code? Maybe I can register the model beforehand, as if the user registered it with the flag.models.py
in the_huggingface_model_registry
dict.