[Bugfix] Fix wrong CLI defaults for dynamic SchedulerConfig fields#28872
[Bugfix] Fix wrong CLI defaults for dynamic SchedulerConfig fields#28872vllm-bot merged 2 commits intovllm-project:mainfrom
SchedulerConfig fields#28872Conversation
Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
There was a problem hiding this comment.
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.
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 |
|
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 <tlleungac@connect.ust.hk>
…llm-project#28872) Signed-off-by: DarkLight1337 <tlleungac@connect.ust.hk>
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.