-
Notifications
You must be signed in to change notification settings - Fork 26
Manage supported model configurations #445
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
Conversation
- Add config file for models and runtime parameters - Add validation code to compare requested model and runtime parameters with supported configurations - Log warning when requested configuration is not supported - Use configuration file in documentation Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Validate that the batch size is <= a tested upper bound Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>
Hi @maxdebayser I added validation code and unit tests for:
Kindly take another look? Thank you! 🙏🏻 |
@ckadner , I was having trouble to explain my thoughts as review comments, so I put them in code form: ckadner#19 . |
@ckadner , my assumptions aren't correct. Please disregard some of my previous comments about upper bounds. |
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've left a small suggestion, but otherwise it LGTM
One last item to do:
|
Signed-off-by: Christian Kadner <[email protected]>
|
||
matching_models = [ | ||
model for model, config in (known_model_configs or {}).items() | ||
if config.items() <= requested_config.items() |
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.
nice!
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 verification part of the code looks good to me and I like some of the clever tricks used for matching configurations. I have just one question: what is the motivation for maintaining the pre-downloaded config.json
files? Is it to avoid downloads during testing or perhaps to make sure that the configurations are not updated remotely?
Because if you do something like
from transformers import AutoConfig
c = AutoConfig.from_reptrained("peiyi9979/math-shepherd-mistral-7b-prm")
the config.json will be downloaded automatically and cached in the local huggingface cache. And it will also use the config.json
of already downloaded models.
I wanted to have one unit test which verifies that we can consistently match models by their config. So, in that unit test, I need to instantiate a model config for each of our supported models and then use those to verify my runtime configuration validation code can reliably "guess" the correct model. Which is needed to actually verify the runtime configurations. Since we run our unit tests in "offline" mode in our GitHub Actions tests, we need to have those configs available before test execution. We could add a pre-step in our GHA test workflow to download those configs (from HF or GHA cache) instead of keeping them in our unit tests folder. But @joerunde had already set that precedent of keeping 2 config.json files, and I like following precedent :-) |
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, @ckadner , that makes sense to me.
Description
Example:
WARNING 09-05 18:46:43 [runtime_config_validator.py:107] The requested configuration is not supported for model 'ibm-ai-platform/micro-g3.3-8b-instruct-1b': RuntimeConfiguration(platform=, cb=True, tp_size=1, max_model_len=128, max_num_seqs=2, num_blocks=0, warmup_shapes=None)
TODO:
num_blocks
(cpu, gpu ...override)?get_warmup_shapes_from_envs()
does not yield same asplatform.py
:cls._warmup_shapes
Review suggestions:
Related Issues
#435