-
-
Notifications
You must be signed in to change notification settings - Fork 11.3k
[Chore] Rename SchedulerConfig.chunked_prefill_enabled
#28735
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
[Chore] Rename SchedulerConfig.chunked_prefill_enabled
#28735
Conversation
…unked_prefill`` 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 is a nice cleanup that removes the redundant SchedulerConfig.chunked_prefill_enabled property and replaces all its usages with SchedulerConfig.enable_chunked_prefill. The changes are consistent across the codebase, including configurations, scheduler logic, and tests. The refactoring is well-executed and improves code clarity. I have reviewed all the changes and found no issues. The PR is good to merge.
Signed-off-by: DarkLight1337 <[email protected]>
Signed-off-by: DarkLight1337 <[email protected]>
|
AFAIK this parameter now only affects the multimodal case when determining max mm items per batch. I wonder whether we should log some warning if it's set to a non-default value since folks might think they can disable prefill chunking when they in fact can't. But actually is that also what |
|
Hmm, we can control chunked prefill for pooling models though? |
|
Scheduler code seems to take care of chunked prefill enable/disable, but it's possible that model runner makes some assumptions about that for MM or generative models, not 100% sure about this. |
@DarkLight1337 ah sorry yes I missed that. Still I wonder if we should warn in non-pooling case. |
In any case let's merge this first. We can update the message in another PR. |
…t#28735) Signed-off-by: DarkLight1337 <[email protected]> Signed-off-by: George D. Torres <[email protected]>
Purpose
SchedulerConfig.chunked_prefill_enabledis redundant withSchedulerConfig.enable_chunked_prefill, so I've renamed all occurrences of the former to the latter. For plugin compatibility, I've deprecated the old property instead of removing it immediately.Follow-up to #28665 (comment)
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.