-
Notifications
You must be signed in to change notification settings - Fork 250
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
Allow users to add additional models via configuration file #1761
Conversation
9fb8d51
to
cdb46b7
Compare
@percyliang could you take a look? There are three different groups that need this (safety evals, NeurIPS Efficiency Challenge, AWS) - the latter two urgently - so I'd like to get it in by Monday if possible. The main thing to hammer down is the config file format:
|
src/helm/benchmark/model_registry.py
Outdated
"""Configuration for a registered model.""" | ||
|
||
model_type: str | ||
"""Name of the client type.""" |
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 different names - model type versus client type? Can we standardize and explain / give an example?
src/helm/benchmark/model_registry.py
Outdated
# TODO(#1673): Add tokenizer name and sequence length fields. | ||
|
||
args: Optional[Dict[str, Any]] = None | ||
"""Configuration for the 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.
It's a bit mysterious what the args are supposed to be - can we explain what these are and give one example? Do these have to be untyped?
src/helm/benchmark/run.py
Outdated
@@ -256,6 +263,8 @@ def main(): | |||
register_huggingface_hub_model_config(huggingface_model_name) | |||
for huggingface_model_path in args.enable_local_huggingface_models: | |||
register_huggingface_local_model_config(huggingface_model_path) | |||
for model_config_path in args.model_config_paths: | |||
register_model_configs_from_path(model_config_path) |
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.
Does this mechanism subsume the Hugging Face local model loading?
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, eventually this will replace the Hugging Face mechanism (after some additional refactoring).
|
||
if get_huggingface_model_config(model): | ||
from helm.proxy.clients.huggingface_client import HuggingFaceClient | ||
|
||
client = HuggingFaceClient(cache_config=cache_config) | ||
elif organization == "openai": | ||
elif model_type == "openai": |
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 the term 'model_type' is not exactly right - I think more of Transformer versus RNN. Why not stick with organization? It is really an organizational thing and has nothing to do with the underlying 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.
I think we can just make this a ObjectSpec.class_name
. See the new example model_deployments.yaml
.
@@ -20,6 +22,28 @@ class AI21RequestError(Exception): | |||
pass | |||
|
|||
|
|||
@dataclass(frozen=True) |
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'm a bit confused why we need the custom model config stuff for AI21 given that is something we define and support as opposed to configured by users? Does that mean we need another file that we need to pass into HELM to run AI21 evals?
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.
Cleaned this up; now we just pass in the additional parameters via AI21Client.__init__()
There's also defining the model in a customizable |
Modified this to be closer to our discussion.
|
|
||
@dataclass(frozen=True) | ||
class ModelDeployment: | ||
name: str |
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.
Add docstring about what a deployment is (as opposed to a model).
model_deployments: List[ModelDeployment] | ||
|
||
|
||
_name_to_model_deployment: Dict[str, ModelDeployment] = {} |
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 we want to use a singleton here? I believe schemas are passed around, which might make things more modular / easier to test?
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.
This may be the result of merging multiple configuration files (e.g. the repo's "defaults" configuration file + the user's or 3P repo's configuration files). So this has to be dynamically constructed. So we can't do the same thing as schema.py
.
class ModelMetadata: | ||
name: str | ||
|
||
# Organization that originally created the model (e.g. "EleutherAI") |
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 we want these comments to be """
under the field so that they get pulled into documentation?
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.
Done.
# Note that this may be different from group or the prefix of the model `name` | ||
# ("together" in "together/gpt-j-6b") as the hosting organization | ||
# may be different from the creator organization. We also capitalize | ||
# this field properly to later display in the UI. |
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.
Not sure if we want to capitalize...because then we will have Meta
and meta
... I don't think we should conflate unique names with display names...for example, the display name might have a space, and I don't think we want the creator_organization to have a space.
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.
In the version in schema.yaml
, this was mixed case... maybe we should revisit later.
# but we set it as an int for plotting purposes. | ||
num_parameters: Optional[int] = None | ||
|
||
# Tags corresponding to the properties of the 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.
Comment that this will probably go...this is more of a property of the deployment, right?
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 problem is that some things are properties of the model (e.g. is this a language model or an image model?) and some are properties of the deployment (e.g. window size).
creator_organization: Optional[str] = None | ||
|
||
# How this model is available (e.g., limited) | ||
access: Optional[str] = None |
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.
can say more explicitly this is the maximum access over all deployments
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.
Done.
423bbe5
to
7a4b92b
Compare
This allows the user to register additional models via configuration file.
Example usage: run
helm-run --run-specs mmlu:subject=anatomy,model=ai21/j2-light --suite v1 -m 5 --model-metadata-paths models_metadata.yaml --model-deployment-paths model_deployments.yaml
.model_metadata.yaml
:model_deployments.yaml
:credentials.conf
: