-
-
Notifications
You must be signed in to change notification settings - Fork 11.1k
[V1] [Bugfix] eagle bugfix and enable correct lm_head for multimodal (2) #18781
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
[V1] [Bugfix] eagle bugfix and enable correct lm_head for multimodal (2) #18781
Conversation
Signed-off-by: Ronald Xu <ronaldxu@amazon.com>
Signed-off-by: Ronald Xu <ronaldxu@amazon.com>
Signed-off-by: Ronald Xu <ronaldxu@amazon.com>
|
👋 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 🚀 |
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 - can you share where was the architectures overwritten and how was this fixed?
|
@ekagra-ranjan Previously, I had automatically overwrote the attributes of self with self.model. However, vllm adds the "Eagle" or "Eagle3" prefix to the architectures attribute a couple lines above (https://github.com/vllm-project/vllm/blob/main/vllm/transformers_utils/configs/eagle.py#L51), so LlamaForCausalLM -> EagleLlamaForCausalLM. So, my previous commit was overwriting this with the original architecture (because I did not have any conditional statements stopping the I think when I was working on this the code for adding the "Eagle" or "Eagle3" prefix did not exist yet, so I had just modified the config.json file of the Eagle model itself to have the "Eagle" prefix. For example, I modified the architectures field manually in (https://huggingface.co/yuhuili/EAGLE-LLaMA3.1-Instruct-8B/blob/main/config.json) to be EagleLlamaForCausalLM. I guess vllm decided to incorporate the Eagle prefix at runtime rather than have an Eagle model's config.json point to the Eagle cls. |
…ft_model_config.hf_config.model_type will get overwritten to the main model type Signed-off-by: Gregory Shtrasberg <Gregory.Shtrasberg@amd.com>
…(2) (vllm-project#18781) Signed-off-by: Ronald Xu <ronaldxu@amazon.com> Signed-off-by: amit <amit.man@gmail.com>
This is a fixed version of #18034 . My previous code overwrote all model attributes, but we should only be overwriting the defaults - I had accidentally also overwrote the
'architectures'attribute that was set a couple lines earlier.@ekagra-ranjan @DarkLight1337 would appreciate a review, thanks!