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.

Sign up for GitHub

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

Jump to bottom

[Bugfix] Eagle: change config name for fc bias #9580

Merged
merged 1 commit into from
Oct 22, 2024
Merged

[Bugfix] Eagle: change config name for fc bias #9580

merged 1 commit into from
Oct 22, 2024

Conversation

gopalsarda
Copy link
Contributor

@gopalsarda gopalsarda commented Oct 22, 2024 •
edited
Loading

  • Changes were made to load fc bias from config as part of this PR - [Bugfix] load fc bias from config for eagle #8790
  • bias param in config is also used to decide if attention has a bias or not in LlamaDecoderLayer
  • Due to the same param name, model is not loaded properly for cases which do not have bias in attention in decoder layer but have a bias in eagle fc layer.
  • Have changed the param to eagle_fc_bias to avoid conflict.

@gopalsarda
[Bugfix] Eagle: change config name for fc bias
fcea109 
- Changes were made to load fc bias from config as part of this PR - vllm-project#8790
- `bias` param in config is also used to decide if attention has a bias or not in [LlamaDecoderLayer](https://github.com/vllm-project/vllm/blob/main/vllm/model_executor/models/llama.py#L215)
- Due to the same param name, model is not loaded properly for cases which do not have attention in decoder layer but have a bias in eagle fc layer.
- Have changed the param to `eagle_fc_bias` to avoid conflict.
@github-actions GitHub Actions
Copy link

👋 Hi! Thank you for contributing to the vLLM project.
Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

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:

  • Add ready label to the PR
  • Enable auto-merge.

🚀

@DarkLight1337
Copy link
Member

@sohamparikh are you OK with this change?

@sohamparikh
Copy link
Contributor

Yes, LGTM!
Thanks for the catch @gopalsarda!

@DarkLight1337 DarkLight1337 enabled auto-merge (squash) October 22, 2024 14:43
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 22, 2024
@DarkLight1337
Copy link
Member

Thanks, let's get this merged then.

@DarkLight1337 DarkLight1337 merged commit 08075c3 into vllm-project:main Oct 22, 2024
71 checks passed
charlifu pushed a commit to charlifu/vllm that referenced this pull request Oct 23, 2024
@gopalsarda @charlifu
[Bugfix] Eagle: change config name for fc bias (vllm-project#9580)
7b15c22 
Signed-off-by: charlifu <charlifu@amd.com>
Alvant pushed a commit to compressa-ai/vllm that referenced this pull request Oct 26, 2024
@gopalsarda @Alvant
[Bugfix] Eagle: change config name for fc bias (vllm-project#9580)
65c761d 
Signed-off-by: Alvant <alvasian@yandex.ru>
MErkinSag pushed a commit to MErkinSag/vllm that referenced this pull request Oct 26, 2024
@gopalsarda
[Bugfix] Eagle: change config name for fc bias (vllm-project#9580)
cf64c09 
Signed-off-by: Erkin Sagiroglu <erkin@infra-aipipeline-1-at1-prox-prod-a.ipa.corp.telnyx.com>
garg-amit pushed a commit to garg-amit/vllm that referenced this pull request Oct 28, 2024
@gopalsarda @garg-amit
[Bugfix] Eagle: change config name for fc bias (vllm-project#9580)
d3d47bb 
Signed-off-by: Amit Garg <mitgarg17495@gmail.com>
FerdinandZhong pushed a commit to FerdinandZhong/vllm that referenced this pull request Oct 29, 2024
@gopalsarda @FerdinandZhong
[Bugfix] Eagle: change config name for fc bias (vllm-project#9580)
a427af2 
Signed-off-by: qishuai <ferdinandzhong@gmail.com>
NickLucche pushed a commit to NickLucche/vllm that referenced this pull request Oct 31, 2024
@gopalsarda @NickLucche
[Bugfix] Eagle: change config name for fc bias (vllm-project#9580)
8938d65 
Signed-off-by: NickLucche <nlucches@redhat.com>
NickLucche pushed a commit to NickLucche/vllm that referenced this pull request Oct 31, 2024
@gopalsarda @NickLucche
[Bugfix] Eagle: change config name for fc bias (vllm-project#9580)
4255cf3 
Signed-off-by: NickLucche <nlucches@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DarkLight1337 DarkLight1337 DarkLight1337 approved these changes

Assignees
No one assigned
Labels
ready ONLY add when PR is ready to merge/full CI is needed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gopalsarda @DarkLight1337 @sohamparikh