-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
[CI/Build] Split up models tests #10069
Conversation
Signed-off-by: DarkLight1337 <[email protected]>
👋 Hi! Thank you for contributing to the vLLM project. Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can do one of these:
🚀 |
It looks like sharding doesn't work over parameter combinations. |
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Would be great if we could get this in before the next nightly! |
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
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 for the fix!
I merged it to fix the test on main ASAP. also left some comments. |
self.language_model = PersimmonForCausalLM(config.text_config, | ||
cache_config=cache_config, | ||
quant_config=quant_config) | ||
self.language_model = PersimmonForCausalLM( |
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 would be better if this can use init_vllm_registered_model
.
I think this config.text_config
misses an architectures
field. we can manually add the field, and then call init_vllm_registered_model
.
we also need to change the signature of init_vllm_registered_model
in the future.
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 has model_type
defined so it should still be possible to match if we add this model to the registry.
def with_hf_config(self, hf_config: PretrainedConfig) -> "VllmConfig": | ||
model_config = copy.deepcopy(self.model_config) | ||
model_config.hf_config = hf_config | ||
|
||
return replace(self, model_config=model_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.
we should flatten VllmConfig
in the future, hf_config
can be the same level as model_config
. and we can have a top-level function vllm_config.replace_with(hf_config=hf_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.
Some values in model_config
depend on the hf_config
, so it might be better to just have a convenience property to access hf_config
directly from VllmConfig
, instead of changing the nested structure.
Signed-off-by: DarkLight1337 <[email protected]> Signed-off-by: OmerD <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]> Signed-off-by: Loc Huynh <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]> Signed-off-by: Jee Jee Li <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]> Signed-off-by: Sumit Dubey <[email protected]>
To reduce the impact of flakiness (e.g. connection failures), this PR splits up the model tests into groups.
I have also fixed VLM test failures that were introduced by #8346 and #9983.