-
-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Structured Outputs] Refactor bitmask construction into get_grammar_bitmask #23361
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
…itmask Signed-off-by: Woosuk Kwon <[email protected]>
Signed-off-by: Woosuk Kwon <[email protected]>
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
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 refactors the logic for constructing the grammar bitmask into a new get_grammar_bitmask
method within the Scheduler
class. This is a good improvement that enhances code clarity and modularity by separating the bitmask creation from the main scheduling logic. The implementation correctly moves the existing logic without introducing any functional changes. The code is well-structured and the refactoring achieves its goal effectively. I have reviewed the changes and found no issues.
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.
lgtm!
…itmask (vllm-project#23361) Signed-off-by: Woosuk Kwon <[email protected]> Signed-off-by: root <[email protected]>
…itmask (vllm-project#23361) Signed-off-by: Woosuk Kwon <[email protected]>
…itmask (vllm-project#23361) Signed-off-by: Woosuk Kwon <[email protected]> Signed-off-by: Xiao Yu <[email protected]>
…itmask (vllm-project#23361) Signed-off-by: Woosuk Kwon <[email protected]>
…itmask (vllm-project#23361) Signed-off-by: Woosuk Kwon <[email protected]>
…itmask (vllm-project#23361) Signed-off-by: Woosuk Kwon <[email protected]>
@WoosukKwon I believe there's a bug here, we should be passing As indicated by this comment vllm/vllm/v1/core/sched/scheduler.py Lines 551 to 553 in 1fdd5c4
self.running != scheduled_new_reqs + scheduled_running_reqs + scheduled_resumed_reqs) .
|
…itmask (vllm-project#23361) Signed-off-by: Woosuk Kwon <[email protected]>
This PR refactors the scheduler's bitmask handling into the new
get_grammar_bitmask
method. I find this cleaner because bitmask construction is orthogonal to the scheduling logic.This will also help #23233