Skip to content

Conversation

@njhill
Copy link
Member

@njhill njhill commented Mar 19, 2025

Importing xgrammar appears to initialize the cuda context, which we don't want to do in the front-end process. It also means that the server can't be started with the (default) multiproc context mode of fork.

I guess this is what LazyLoader is meant to help with, but it doesn't seem to be working as intended since #14694 was merged.

Importing xgrammar appears to initialize the cuda context, which we don't want to do in the front-end process. It also means that the server can't be started with the (default) multiproc context mode of fork.

I guess this is what LazyLoader is meant to help with, but it doesn't seem to be working as intended since vllm-project#14694 was merged.

Signed-off-by: Nick Hill <[email protected]>
@njhill njhill added the bug Something isn't working label Mar 19, 2025
@njhill njhill requested review from mgoin and russellb as code owners March 19, 2025 23:10
@njhill njhill requested review from aarnphm and removed request for mgoin and russellb March 19, 2025 23:10
@github-actions
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.

🚀

@njhill njhill requested a review from russellb March 19, 2025 23:11
@mergify mergify bot added the v1 label Mar 19, 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.

This looks harmless, so I'm fine with it. I don't understand why it fixes anything though :(

@russellb russellb enabled auto-merge (squash) March 19, 2025 23:14
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Mar 19, 2025
@njhill
Copy link
Member Author

njhill commented Mar 19, 2025

Ah I found it's due to references to xgr types in the class definition here - that's what triggers the lazy import.

@russellb
Copy link
Member

Ah I found it's due to references to xgr types in the class definition here - that's what triggers the lazy import.

oh, weird ... I thought that was ignored unless running type checking.

I wonder if using a Forward Reference (putting the type in quotes) would also work around the issue? I'm still fine just merging this PR for now, though.

@russellb russellb merged commit c47aafa into vllm-project:main Mar 20, 2025
40 of 42 checks passed
@aarnphm
Copy link
Collaborator

aarnphm commented Mar 20, 2025

Ah I found it's due to references to xgr types in the class definition here - that's what triggers the lazy import.

oh, weird ... I thought that was ignored unless running type checking.

I wonder if using a Forward Reference (putting the type in quotes) would also work around the issue? I'm still fine just merging this PR for now, though.

You can use from __future__ import annotations to make it into forward reference, but this fix also works

@njhill njhill deleted the lazy-xgrammar branch March 20, 2025 02:52
erictang000 pushed a commit to erictang000/vllm that referenced this pull request Mar 25, 2025
lulmer pushed a commit to lulmer/vllm that referenced this pull request Apr 7, 2025
shreyankg pushed a commit to shreyankg/vllm that referenced this pull request May 3, 2025
RichardoMrMu pushed a commit to RichardoMrMu/vllm that referenced this pull request May 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ready ONLY add when PR is ready to merge/full CI is needed v1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants