Simplify config registration #2142
Conversation
yifanmai
commented
Dec 13, 2023
yifanmai
commented
- Ensure that config registration is only performed at entry points
- Ensure that custom config registration does not get overwritten by default model registration
- Removed some unused / unnecessary helper methods
- Cleaned up comments and documentation
yifanmai
changed the title
| # Can't send release_dates in ModelMetadata bacause dates cannot be round-tripped to and from JSON easily. | ||
| # TODO(#2158): Either fix this or delete get_general_info. | ||
| all_models = [dataclasses.replace(model_metadata, release_date=None) for model_metadata in ALL_MODELS_METADATA] | ||
| return GeneralInfo(version=VERSION, example_queries=example_queries, all_models=all_models) |
There was a problem hiding this comment.
Actually a pre-existing issue that wasn't caught in the test before because the test forgot to register models, so ALL_MODELS_METADATA was empty.
| @@ -248,6 +251,9 @@ def main(): | |||
| args = parser.parse_args() | |||
| validate_args(args) | |||
|
|
|||
| register_configs_from_package() | |||
| register_configs_from_directory(args.local_path) | |||
There was a problem hiding this comment.
Is there any doc that says local-path controls model registration? Current doc says it's for ServerService
src/helm/benchmark/run.py
Outdated
| @@ -248,6 +251,9 @@ def main(): | |||
| args = parser.parse_args() | |||
| validate_args(args) | |||
|
|
|||
| register_configs_from_package() | |||
There was a problem hiding this comment.
I was a bit confused by the name package until I saw the function definition as I thought it takes in a package name. Would it be better to name it as register_configs_from_helm_package or register_builtin_configs?
There was a problem hiding this comment.
Good point. Changed to register_builtin_configs_from_helm_package.
| @@ -58,7 +59,10 @@ def __init__(self, base_path: str = "prod_env", root_mode=False, mongo_uri: str | |||
| self.toxicity_classifier_client: Optional[ToxicityClassifierClient] = None | |||
|
|
|||
| def get_general_info(self) -> GeneralInfo: | |||
| return GeneralInfo(version=VERSION, example_queries=example_queries, all_models=ALL_MODELS_METADATA) | |||
| # Can't send release_dates in ModelMetadata bacause dates cannot be round-tripped to and from JSON easily. | |||
There was a problem hiding this comment.
Can it get serialized as a string?
There was a problem hiding this comment.
Not easily - the type of release_date is not string, so mypy typechecking will complain.
