Skip to content
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

Test inference endpoint model config parsing from path #434

Conversation

albertvillanova
Copy link
Member

@albertvillanova albertvillanova commented Dec 11, 2024

Test inference endpoint model config parsing from path.

As a follow-up of this PR:

this PR implements a test of the parsing of the config file for evaluation in inference endpoints.

To do so, the parsing is factorized into a new method:

InferenceEndpointModelConfig.from_path(model_config_path)

Additionally, a new examples/model_configs/endpoint_model_reuse_existing.yaml is added.

Findings:

  • In the examples/model_configs/endpoint_model.yaml, the field generation.add_special_token is ignored
    • This PR deletes this field
  • There is another naming misalignment: dtype in the config file is named as model_dtype InferenceEndpointModel attribute

Question: should we rename it?

  • I would suggest aligning both names by replacing the dtype field in the config file with model_dtype
    • Note that:
      • it already exists model_name
      • and there are also instance fields called instance_type and instance_size

@HuggingFaceDocBuilderDev
Copy link
Collaborator

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

base_params:
# Pass either model_name, or endpoint_name and true reuse_existing
endpoint_name: "llama-2-7B-lighteval" # needs to be lower case without special characters
reuse_existing: true # defaults to false; if true, ignore all params in instance, and don't delete the endpoint after evaluation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"and does not"

Comment on lines +118 to +119
config["base_params"]["model_dtype"] = config["base_params"].pop("dtype", None)
return cls(**config["base_params"], **config.get("instance", {}))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and much cleaner! We might have to add this to all model types

@albertvillanova
Copy link
Member Author

albertvillanova commented Dec 12, 2024

@clefourrier I am not sure if you agree in the renaming of the dtype field in the config file to model_dtype: #434

We could address that in a subsequent PR

@albertvillanova albertvillanova merged commit f907a34 into huggingface:main Dec 12, 2024
3 checks passed
NathanHB pushed a commit that referenced this pull request Dec 17, 2024
Implement TGI model config from path:
```python
TGIModelConfig.from_path(model_config_path)
```

Follow-up to:
- #434 

Related to:
- #439
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants