Skip to content

Conversation

shen-shanshan
Copy link
Contributor

@shen-shanshan shen-shanshan commented Jul 31, 2025

Essential Elements of an Effective PR Description Checklist

  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.

Purpose

Currently, the feature structured output is closely coupled with the model_runner, and we need to implement duplicate apply_grammar_bitmask() method in different model_runner of each platform, e.g., gpu_model_runner, npu_model_runner. Once there are changes have made in this method, we need to update the method in all kinds of model_runner to sync these changes.

Thus, maybe it's better to move these structured output related code in model_runner to the structured_output module to make it clearer and more extensible.

Test Plan

Test Result

(Optional) Documentation Update

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 removes a redundant null check for grammar_bitmask. This is a good code cleanup that improves maintainability by removing unnecessary code. The change is straightforward and relies on the caller performing the null check, as stated in the PR description.

@shen-shanshan
Copy link
Contributor Author

CC: @russellb

@shen-shanshan shen-shanshan changed the title [Misc] Remove redundant check for bitmask in model_runner [Structured Output] Remove redundant check for bitmask in model_runner Jul 31, 2025
Copy link

👋 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 fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

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 ready label to the PR or enable auto-merge.

🚀

@shen-shanshan shen-shanshan changed the title [Structured Output] Remove redundant check for bitmask in model_runner [Structured Output] Replace if check for bitmask in model runner to assert Aug 1, 2025
@shen-shanshan shen-shanshan changed the title [Structured Output] Replace if check for bitmask in model runner to assert [Structured Output][Refactor] Move apply_grammar_bitmask() method from ModelRunner to StructuredOutputManager Aug 1, 2025
@shen-shanshan shen-shanshan changed the title [Structured Output][Refactor] Move apply_grammar_bitmask() method from ModelRunner to StructuredOutputManager [Structured Output][Refactor] Move apply_grammar_bitmask() method from ModelRunner to StructuredOutputManager Aug 1, 2025
Copy link
Member

@russellb russellb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The StructuredOutputManager isn't an object used by the model runner right now. How about just making it a utility function somewhere instead of hanging it off of that class? Maybe here https://github.com/vllm-project/vllm/blob/main/vllm/v1/structured_output/utils.py

@shen-shanshan
Copy link
Contributor Author

The StructuredOutputManager isn't an object used by the model runner right now. How about just making it a utility function somewhere instead of hanging it off of that class? Maybe here https://github.com/vllm-project/vllm/blob/main/vllm/v1/structured_output/utils.py

Thanks for your suggestion! I will modify it later~

Copy link

mergify bot commented Aug 7, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @shen-shanshan.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Aug 7, 2025
@mergify mergify bot removed the needs-rebase label Aug 7, 2025
@shen-shanshan shen-shanshan changed the title [Structured Output][Refactor] Move apply_grammar_bitmask() method from ModelRunner to StructuredOutputManager [Structured Output][Refactor] Move apply_grammar_bitmask() method from ModelRunner to structured output utils Aug 8, 2025
@shen-shanshan shen-shanshan force-pushed the so branch 2 times, most recently from 5003b64 to c909e3c Compare August 14, 2025 03:42
@russellb
Copy link
Member

I'm good with this in general, but it will need to be updated one more time.

It's in conflict with main. Before fixing it though, there are a few bug fix PRs that I'd like to merge first. Once they go in, if you can incorporate them into your change, I think we'll be good.

  1. [FIXBUG] Correctly Apply Grammar Bitmask in Mixed Batches #22896
  2. Upgrade xgrammar to 0.1.23 #22988
  3. [Structured Outputs] [Bug] Fix misalignment in apply_grammar_bitmask causing unintended masking and NaN logits #22963

Copy link

mergify bot commented Aug 15, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @shen-shanshan.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Aug 15, 2025
@shen-shanshan
Copy link
Contributor Author

I'm good with this in general, but it will need to be updated one more time.

It's in conflict with main. Before fixing it though, there are a few bug fix PRs that I'd like to merge first. Once they go in, if you can incorporate them into your change, I think we'll be good.

  1. [FIXBUG] Correctly Apply Grammar Bitmask in Mixed Batches #22896
  2. Upgrade xgrammar to 0.1.23 #22988
  3. [Structured Outputs] [Bug] Fix misalignment in apply_grammar_bitmask causing unintended masking and NaN logits #22963

OK, no problem. After these fix PR have been merged, I will update this soon.

@shen-shanshan
Copy link
Contributor Author

@russellb Hello, all the fix PRs you mentioned have already been merged, and I have rebased on the latest code. 😃

Copy link
Member

@russellb russellb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks!

@russellb russellb enabled auto-merge (squash) September 4, 2025 23:31
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 4, 2025
auto-merge was automatically disabled September 8, 2025 01:45

Head branch was pushed to by a user without write access

Signed-off-by: shen-shanshan <[email protected]>
@shen-shanshan
Copy link
Contributor Author

@russellb The CI run failed due to #24366, and I have rebased once again. Now the CI has all passed.

Copy link
Collaborator

@benchislett benchislett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

mergify bot commented Sep 11, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @shen-shanshan.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Sep 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase ready ONLY add when PR is ready to merge/full CI is needed structured-output v1
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants