-
-
Notifications
You must be signed in to change notification settings - Fork 11.5k
[Bugfix] Fix wrong CLI defaults for dynamic SchedulerConfig fields
#28872
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: DarkLight1337 <[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.
Code Review
This pull request correctly fixes a bug where CLI arguments for dynamic SchedulerConfig fields (max_num_batched_tokens, max_num_seqs, enable_chunked_prefill) were using static defaults from SchedulerConfig, preventing the intended dynamic default logic from executing. By overriding the default to None for these CLI arguments, the engine can now correctly apply dynamic defaults based on the usage context and hardware. The changes are clear and address the issue effectively. I've also pointed out a similar issue with async_scheduling that could be addressed for consistency.
| "--max-num-batched-tokens", | ||
| **{ | ||
| **scheduler_kwargs["max_num_batched_tokens"], | ||
| "default": None, | ||
| }, | ||
| ) | ||
| scheduler_group.add_argument( | ||
| "--max-num-seqs", **scheduler_kwargs["max_num_seqs"] | ||
| "--max-num-seqs", | ||
| **{ | ||
| **scheduler_kwargs["max_num_seqs"], | ||
| "default": None, | ||
| }, |
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.
While you're fixing the CLI defaults for dynamic SchedulerConfig fields, it seems like async_scheduling might be another case that needs a similar change.
Currently, EngineArgs.async_scheduling defaults to False (from SchedulerConfig.async_scheduling), and the CLI argument also defaults to False. This means the logic in VllmConfig.__post_init__ to dynamically enable async_scheduling (where self.scheduler_config.async_scheduling is None) will never be triggered.
To enable the dynamic default behavior for async_scheduling, you might need to:
- Change the default value of
async_schedulinginEngineArgstoNone. - Update its
add_argumentcall inadd_cli_argsto setdefault=None, similar to the other fields in this PR.
This would make its behavior consistent with the other dynamically configured scheduler arguments.
|
Looking at the failing test - it seems that |
Signed-off-by: DarkLight1337 <[email protected]>
|
It is caused by this code, which seems intentional. cc @NickLucche @russellb For now I have worked around it by increasing |
|
The previously failing test now passes, merging as the other entrypoint test is failing on main |
…llm-project#28872) Signed-off-by: DarkLight1337 <[email protected]>
Purpose
Fix an issue caused by #28665 because the CLI still uses the defaults from
SchedulerConfig. Sorry for breaking this!Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.