-
-
Notifications
You must be signed in to change notification settings - Fork 7.7k
Improve configs - SchedulerConfig
#16533
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
Merged
DarkLight1337
merged 13 commits into
vllm-project:main
from
hmellor:improve-scheduler-config
Apr 14, 2025
Merged
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
33595ad
Use `Literal` for `distributed_executor_backend` choices
hmellor 65e108a
Fix regex warning in `SortedHelpFormatter`
hmellor 576495c
`nullable_str` -> `optional_str`
hmellor 2d1ca29
Make conversion from config to args more robust
hmellor ec22618
Move docs and defaults to `SchedulerConfig`
hmellor 831985d
Maky `mypy` happy
hmellor 3b3c630
Merge branch 'main' into improve-scheduler-config
hmellor c0cffed
Use `TypeVar`
hmellor e89a86b
Fix union helpers for special typing forms (i.e. `Literal`)
hmellor f136723
`dicts` are passed as `str`
hmellor d3d53eb
Make mypy happy
hmellor 709e127
Add arbitrary defaults with warnings so `VllmConfig()` can be default…
hmellor 72eb95a
Improve `SchedulerConfig.max_model_len` docstring
hmellor File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
Since
None
is resolved insideEngineArgs
orModelConfig
, I feel that we should not allow it to beNone
when initializingSchedulerConfig
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.
The default is
None
so that the default inEngineArgs
isNone.
We have the same problem with the
max_num_batched_tokens
andmax_model_length
. It can't be optional in the config because maths is done on them in__post_init__
, but they must haveNone
defaults so thatEngineArgs
can have them as optional.Where would you change behaviour so that the defaults here are not
None
?Uh oh!
There was an error while loading. Please reload this page.
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.
Maybe we can have a
from_optional
method just like the one forSamplingParams
to resolve theNone
valuesThere 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.
Using post init to overwrite None values leads to a bunch of issues with type checking since downstream access of these attributes will unnecessarily need to consider the None case in order to avoid type checking errors.
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.
Would we then need to change everywhere
SchedulingConfig
is instantiated? And then remember to instantiate it that way?I agree, but that's why there in
SchedulingConfig
the type isint
but inEngineArgs
the type isOptional[int]
, so that the type cheking is happy.Uh oh!
There was an error while loading. Please reload this page.
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 think it's worth noting that prior to this PR, this is how we handled
max _num_batched_tokens
.I changed the other two here so that they would no longer have arbitrary defaults that could be inherited by
EngineArgs
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.
As per offline discussion, this is the cleanest way given how the configs are set up right now. We can address this later