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

[core] clean up cudagraph batchsize padding logic #10996

Merged
merged 34 commits into from
Dec 13, 2024
Merged

[core] clean up cudagraph batchsize padding logic #10996

merged 34 commits into from
Dec 13, 2024

Conversation

youkaichao
Copy link
Member

@youkaichao youkaichao commented Dec 8, 2024 •
edited by github-actions bot
Loading

unify the logic of cudagraph batchsize padding, and allow user-specified cudagraph capture sizes.

users might want to customize cudagraph capture sizes to accelerate start time, while only capture cudagraph for certain sizes they care about.

@youkaichao
draft
81d93c4 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
fix cudagraph logic
a69adbc 
Signed-off-by: youkaichao <[email protected]>
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 8, 2024

👋 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.

🚀

@youkaichao youkaichao changed the title [core] allow user-specified cudagraph capture sizes [core] clean up cudagraph batchsize padding logic Dec 8, 2024
@youkaichao
add max_capture_size
f2db1d0 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
add _MAX_BATCH_SIZE_TO_CAPTURE
08b6dd4 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
remove dead code
3a1501a 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
fix
6907008 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
fix
42c9300 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
fix
4c8adcb 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
fix
69f8ff2 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
hide some details in string form
830e34d 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
hide some details in string form
a8f3ef1 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
hide some details in string form
8edddfd 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
hide some details in string form
2f7a17d 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
fix pydantic
7379c67 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
fix enforce eager
1c8067c 
Signed-off-by: youkaichao <[email protected]>
WoosukKwon
WoosukKwon reviewed Dec 12, 2024
View reviewed changes
vllm/config.py Outdated Show resolved Hide resolved
@mergify Mergify
Copy link

mergify bot commented Dec 12, 2024

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @youkaichao.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Dec 12, 2024
@mergify mergify bot removed the needs-rebase label Dec 12, 2024
@youkaichao
rename to pad_for_cudagraph
224c6f2 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
remove mamba import
957a72e 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
unify one function
d3c3bdc 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
fix
f952d18 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
fix
e92559c 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
fix mamba
a02b8f1 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
use list
24b548a 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
comment
4ba82c0 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
remove comments
859e3ee 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
comments
ce0cff9 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
comments
3f750a2 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
comments
56512ba 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
fix
5d6928a 
Signed-off-by: youkaichao <[email protected]>
youkaichao
youkaichao commented Dec 12, 2024
View reviewed changes
vllm/model_executor/models/jamba.py Outdated
@@ -420,6 +420,9 @@ def __init__(self, *, vllm_config: VllmConfig, prefix: str = ""):

self.make_empty_intermediate_tensors = (
self.model.make_empty_intermediate_tensors)
self.max_batch_size = (vllm_config.pad_for_cudagraph(
self.scheduler_config.max_num_seqs)
if self.scheduler_config else 8192 + 2)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tlrmchlsmth do you know when self.scheduler_config will be None here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I don't. @mzusman do you know?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be None, just added it as a safety guard - It was added before the whole vllm_config was available inside the modeling file

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! can you try to remove it then?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, will open a PR shortly

WoosukKwon
WoosukKwon reviewed Dec 13, 2024
View reviewed changes
vllm/config.py Outdated Show resolved Hide resolved
@youkaichao
remove if
f4a7a77 
Signed-off-by: youkaichao <[email protected]>
WoosukKwon
WoosukKwon approved these changes Dec 13, 2024
View reviewed changes
Copy link
Collaborator

@WoosukKwon WoosukKwon left a 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!

@youkaichao youkaichao enabled auto-merge (squash) December 13, 2024 01:15
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Dec 13, 2024
@youkaichao
fix jamba
07d77a1 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
fix mamba
c454391 
Signed-off-by: youkaichao <[email protected]>
@youkaichao
fix both
74f69b6 
Signed-off-by: youkaichao <[email protected]>
@youkaichao youkaichao merged commit be39e3c into vllm-project:main Dec 13, 2024
54 checks passed
@youkaichao youkaichao deleted the cudagraph_sizes branch December 13, 2024 06:58
BKitor pushed a commit to BKitor/vllm that referenced this pull request Dec 30, 2024
@youkaichao @BKitor
[core] clean up cudagraph batchsize padding logic (vllm-project#10996)
4e18318 
Signed-off-by: youkaichao <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mzusman mzusman mzusman left review comments

@tlrmchlsmth tlrmchlsmth tlrmchlsmth left review comments

@WoosukKwon WoosukKwon WoosukKwon approved these changes

@DarkLight1337 DarkLight1337 Awaiting requested review from DarkLight1337 DarkLight1337 is a code owner

@ywang96 ywang96 Awaiting requested review from ywang96 ywang96 is a code owner

@robertgshaw2-neuralmagic robertgshaw2-neuralmagic Awaiting requested review from robertgshaw2-neuralmagic robertgshaw2-neuralmagic is a code owner

@njhill njhill Awaiting requested review from njhill njhill is a code owner

@comaniac comaniac Awaiting requested review from comaniac comaniac is a code owner

@alexm-neuralmagic alexm-neuralmagic Awaiting requested review from alexm-neuralmagic alexm-neuralmagic is a code owner

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.
4 participants
@youkaichao @tlrmchlsmth @mzusman @WoosukKwon