-
-
Notifications
You must be signed in to change notification settings - Fork 11.6k
[Misc] Use model_redirect to redirect the model name to a local folder. #14116
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
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run 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 either: Add 🚀 |
50acdc7 to
5fecb39
Compare
|
I think this is not necessary anymore since we have migrated the CI/CD to using our own file storage which has pre-downloaded models. |
|
This feature is very convenient for everyone who needs to load models locally. No need to use the model path every time Not just for vllm CI/CD |
|
This pull request has merge conflicts that must be resolved before it can be |
|
Don't we already have |
|
Some of my models use modelscope, some use hf, and some, e.g. deepseek v3, need to be downloaded to another partition. I found that using model_overwrite is very convenient |
|
Personally I don't use ModelScope, so I'll have @Isotr0py review this instead. |
|
@Isotr0py examples/offline_inference/basic/use_model_overwrite.py will be deleted. I think the config is too complicated, and it will also request hf multiple times. |
Isotr0py
left a comment
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.
Hmmm, I'm fine to have a function to redirect the model_repo to a downloaded local directory manually, but I don't really like the name of "overwrite" and the introduction of .model.overwrite (seems it's not a formal file used by neither HF nor modelscope)...
vllm/transformers_utils/config.py
Outdated
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.
| def model_overwrite(model: str): | |
| def maybe_model_redirect(model: str): |
I prefer to use maybe_model_redirect here.
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.
maybe_model_redirect fine
vllm/transformers_utils/tokenizer.py
Outdated
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 move the helper function to vllm.transformers_utils.utils if it's used by both config and tokenizer.
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 just need to call the redirect function when initialize ModelConfig, so that we don't need to add it here and there.
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 don't want to override any model name in ModelConfig.
Yes, reading config is too complicated, and I don't want to write it everywhere, too.
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.
Is this a formal file in modelscope? I can't find it in modelscope's API documentations.
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 named it
I think we just need to redirect to local location when initializing |
It's all very hacky anyway. Maybe I should stop this stupid idea. |
IMO, if we redirect the model_repo to local directory manually, we should also make sure the model's name updated accordingly, otherwise it's a little bit hacky and make bugs caused by outdated custom code difficult to find. (For HF, model with custom code won't be updated automatically when loading an outdated loacl checkpoint) About the server name, we can use |
|
I personally prefer 2
Looking forward to hearing your opinions |
Isotr0py
left a comment
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.
LGTM now!
|
Thanks for reviewing the code |
Head branch was pushed to by a user without write access
Pull request was closed
|
Please restart the test |
|
I'm not sure if this PR is the cause of the problem. It seems that these tests also reported errors yesterday. QVQ |
|
The entrypoint test failure should be unrelated, I can confirm it's passed locally. The V1 test is flaky currently. 😅 |
QVQ |
…r. (vllm-project#14116) Signed-off-by: xinyuxiao <[email protected]>
…r. (vllm-project#14116) Signed-off-by: Louis Ulmer <[email protected]>
…r. (vllm-project#14116) Signed-off-by: Mu Huai <[email protected]>
TLDR
Use model_redirect to redirect the model name to a local folder.
Use model name in the code without hard-coding the model path.
Usage
One redirect rule per line, model_name and redirect_name separated by \t.
For example, given a redirection facebook/opt-125m -> /data/LLM-model/opt-125m
should be equivalent to
Use Case