-
Notifications
You must be signed in to change notification settings - Fork 17
[CB] remove VLLM_SPYRE_RM_PADDED_BLOCKS, enable the feature by default #231
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
Signed-off-by: Yannick Schnider <[email protected]>
👋 Hi! Thank you for contributing to vLLM support on Spyre.
Or this can be done with
Now you are good to go 🚀 |
Signed-off-by: Yannick Schnider <[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.
I love negative diff
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.
Nice! Some nits
tests/e2e/test_spyre_cb.py
Outdated
@@ -203,7 +202,7 @@ def get_params_test_blocks_borders_aligned_prompts(): | |||
}, | |||
{ | |||
"step": 70, # Decode sequence 2 | |||
"tkv": 131, | |||
"tkv": 67, |
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.
Worth adding a comment on why the tkv decreased since left padding reduction is default now
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.
Not sure if get_params_test_remove_left_padding
still has any value left since removing the left padding is the default cc @sducouedic
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.
good point, do you agree @sducouedic ?
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.
yep we should remove that one
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.
yep we should remove that one
Signed-off-by: Yannick Schnider <[email protected]>
Signed-off-by: Yannick Schnider <[email protected]>
[CB] remove VLLM_SPYRE_RM_PADDED_BLOCKS, enable the feature by default
This PR removes
VLLM_SPYRE_RM_PADDED_BLOCKS
and enables the removal of padded blocks by default. The feature is working on AIU Spyre, hence there is no need for turning it off anymore.