-
-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Bugfix]: Clean up chunked prefill logging when using whisper #25075
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
[Bugfix]: Clean up chunked prefill logging when using whisper #25075
Conversation
This pull request has merge conflicts that must be resolved before it can be |
5070792
to
8ea5e85
Compare
6cf6d9e
to
4edcb2f
Compare
vllm/config/scheduler.py
Outdated
is_encoder_decoder: bool = False | ||
"""True if the model is an encoder-decoder model.""" | ||
|
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.
If this already exists in ModelConfig
, why duplicate it here?
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.
True, we likely don't want to store it here as well.
Would an InitVar
be sufficient here?
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 InitVar
solution works.
However, in other cases like this (where two sibling configs interact) I've tended to perform those interactions in the parent's __post_init__
, VllmConfig
in this case. Would that work in this case?
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.
That's where I had it before this change, but we end up with a confusing log message about features being enabled coming from the SchedulerConfig's post_init before VllmConfig's post_init fixed it and disabled them.
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.
Another option would be to perform the Chunked prefill is enabled...
log in the VllmConfig
, but not sure it makes sense to put it there
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.
Ah I see, thankn you for explaining. Let's stick with the initvar
fefc7ab
to
4a48dc5
Compare
59b2a17
to
5e0a186
Compare
Head branch was pushed to by a user without write access
2abc703
to
b721f6c
Compare
Signed-off-by: simondanielsson <simon.danielsson99@hotmail.com>
Signed-off-by: simondanielsson <simon.danielsson99@hotmail.com>
Signed-off-by: simondanielsson <simon.danielsson99@hotmail.com>
Signed-off-by: simondanielsson <simon.danielsson99@hotmail.com>
Signed-off-by: simondanielsson <simon.danielsson99@hotmail.com>
Signed-off-by: simondanielsson <simon.danielsson99@hotmail.com>
Signed-off-by: simondanielsson <simon.danielsson99@hotmail.com>
b721f6c
to
46594df
Compare
Signed-off-by: simondanielsson <simon.danielsson99@hotmail.com>
@russelb conflicts fixed now - should be good to go after CI. Thanks! |
…roject#25075) Signed-off-by: simondanielsson <simon.danielsson99@hotmail.com>
Signed-off-by: simondanielsson <simon.danielsson99@hotmail.com> Signed-off-by: yewentao256 <zhyanwentao@126.com>
Purpose
Closes #25071.
Test Plan
logs should no longer mention
"Chunked prefill is enabled with ..."
:Expecting simply
SchedulerConfig
norVllmConfig
. Verify with new tests.Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.md
andexamples
for a new model.