-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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][Spec Decode] fix: broken test for EAGLE model #11972
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Sungjae Lee <[email protected]>
Signed-off-by: Sungjae Lee <[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:
🚀 |
Signed-off-by: Sungjae Lee <[email protected]>
@@ -69,7 +74,8 @@ def __init__(self, *, vllm_config: VllmConfig, prefix: str = ""): | |||
|
|||
# Modify layer normalization and residual connections as suggested | |||
# in the EAGLE framework: https://github.com/SafeAILab/EAGLE | |||
self.model.model.layers[0].input_layernorm = DummyInputLayerNorm() | |||
self.model.model.layers[0].input_layernorm = DummyInputLayerNorm( | |||
weight=self.model.model.layers[0].input_layernorm.weight) |
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.
With the standard EAGLE model checkpoint, this part is not necessary. However, the EAGLE model ("abhigoyal/vllm-eagle-llama-68m-random") used in spec_decode/e2e/test_eagle_correctness.py
contains an input layernorm in its checkpoint. If I simply remove the weight in this DummyInputLayerNorm, it could cause errors when loading the model's weights.
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.
nit: Can you please add a comment on the need to have these weights for some of the unit tests that we have?
@DarkLight1337 |
Thanks for the quick fix! To avoid starting up a new CI instance just for this test, can you combine this with the speculative decoding CI? |
Signed-off-by: Sungjae Lee <[email protected]>
I was confused whether it was good or not to combine. Thank you for your feedbacks. I just updated it as you recommended. |
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.
#11672 (comment)
here was an issue in the above PR that caused broken tests and a CI test omission.
This PR fixes the broken tests and updates the test pipeline.
test
As follows, broken unit tests were passed in my local environments.