-
-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Bugfix][Core] Fix xgrammar import failure on unsupported platforms #27148
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][Core] Fix xgrammar import failure on unsupported platforms #27148
Conversation
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 aims to fix an import failure for xgrammar
on unsupported platforms by making the import conditional. While the approach is correct, the implementation introduces critical runtime errors. Setting XgrammarBackend
and validate_xgrammar_grammar
to None
on unsupported platforms causes TypeError
exceptions when these are used without being checked for None
. These issues need to be addressed to prevent crashes. Additionally, there's a suspicious change in a regular expression that seems to be an error and could affect token processing.
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.
💡 Codex Review
vllm/vllm/v1/engine/processor.py
Lines 279 to 283 in 80e578b
if backend.startswith("xgrammar"): | |
# xgrammar with no fallback | |
validate_xgrammar_grammar(params) | |
elif backend.startswith("guidance"): | |
# TODO: ideally we would have the LLTokenizer here as Lark syntax |
When is_xgrammar_supported()
is false, the conditional import at the top of this module assigns validate_xgrammar_grammar = None
, but _validate_structured_output
still calls it for both explicitly selected xgrammar
backends and the "auto" fallback. On unsupported architectures (e.g., RISC‑V) this will raise TypeError: 'NoneType' object is not callable
before any user‑friendly error can be produced, leaving structured outputs unusable. The call should be wrapped in a support check or replaced with a clear exception before reaching this branch.
vllm/vllm/v1/structured_output/__init__.py
Lines 104 to 107 in 80e578b
if backend == "xgrammar": | |
self.backend = XgrammarBackend( | |
self.vllm_config, | |
tokenizer=self.tokenizer, |
For unsupported CPU architectures, XgrammarBackend
is set to None
during import, yet grammar_init
still tries to call it whenever _backend
equals "xgrammar". This again results in TypeError: 'NoneType' object is not callable
instead of the intended graceful fallback, so a request that explicitly selects the xgrammar backend will crash even though the commit message promises a clearer error. The backend selection logic should refuse "xgrammar" before reaching this instantiation or raise a descriptive exception.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
It makes sense to me to conditionally import xgrammar on unsupported platforms. Also it would be great if you could address the comments from gemini! |
Can u also grep all places that had xgrammar. Yixin informed me that xgrammar should be compiled with pip on unsupported architecture. I would prefer to try that before adding comples gating imports |
80e578b
to
acd62ac
Compare
/gemini review |
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 correctly implements conditional imports for xgrammar
to avoid ModuleNotFoundError
on unsupported platforms. This is a good approach, using an is_xgrammar_supported()
utility to provide dummy implementations that raise errors when xgrammar
is used where it's not available. My review has identified two issues: a misleading error message that could confuse users, and an incorrect change to a regular expression that appears to be a mistake and could break tokenization logic. Addressing these points will improve the quality of the change.
acd62ac
to
43d00ba
Compare
/gemini review |
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 effectively resolves the ModuleNotFoundError
for xgrammar
on unsupported platforms by implementing conditional imports based on CPU architecture. The approach of using a helper function is_xgrammar_supported()
and providing fallback implementations that raise clear errors is robust and well-executed. This change will improve the user experience on platforms like RISC-V, allowing them to use vLLM with alternative backends without import issues. I have one major concern regarding an unrelated change in vllm/v1/structured_output/utils.py
which appears to introduce a bug.
Hi @aarnphm, Thanks for the feedback. I completely understand your preference for a compile-time solution over adding conditional imports—it would definitely be a cleaner approach if it were possible. However, the reason I opened this PR is precisely because I ran Here is the full build log, which I've collapsed for readability: Click to expand full RISC-V build log for xgrammarUsing pip 25.2 from /AI/hebo/.venv/lib64/python3.11/site-packages/pip (python 3.11)
Collecting xgrammar
Downloading xgrammar-0.1.25.tar.gz (2.3 MB)
...
(Build steps 1/35 through 34/35 succeed)
...
[35/35] : && /opt/gcc-15.1/bin/g++ -fPIC ... -o cpp/nanobind/xgrammar/xgrammar_bindings.cpython-311-riscv64-linux-gnu.so ...
FAILED: [code=1] cpp/nanobind/xgrammar/xgrammar_bindings.cpython-311-riscv64-linux-gnu.so
: && /opt/gcc-15.1/bin/g++ -fPIC ... -o cpp/nanobind/xgrammar/xgrammar_bindings.cpython-311-riscv64-linux-gnu.so ...
/opt/gcc-15.1/lib/gcc/riscv64-unknown-linux-gnu/15.1.0/../../../../riscv64-unknown-linux-gnu/bin/ld: /tmp/cc1vRzOe.ltrans0.ltrans.o: in function `xgrammar::GrammarMatcher_FillNextTokenBitmask(xgrammar::GrammarMatcher&, nanobind::ndarray<>, int, bool)':
/tmp/pip-install-o0ve9kqg/xgrammar_d53a5abf64a84c308e17ad519272ee4f/cpp/nanobind/nanobind.cc:58:(.text+0x332): undefined reference to `nanobind::detail::bytes_from_cstr(char const*)'
/opt/gcc-15.1/lib/gcc/riscv64-unknown-linux-gnu/15.1.0/../../../../riscv64-unknown-linux-gnu/bin/ld: /tmp/cc1vRzOe.ltrans0.ltrans.o: in function `xgrammar::TokenizerInfo_GetDecodedVocab(xgrammar::TokenizerInfo const&)':
/tmp/pip-install-o0ve9kqg/xgrammar_d53a5abf64a84c308e17ad519272ee4f/cpp/nanobind/nanobind.cc:85:(.text+0x42e): undefined reference to `nanobind::detail::nb_func_new(void const*)'
/opt/gcc-15.1/lib/gcc/riscv64-unknown-linux-gnu/15.1.0/../../../../riscv64-unknown-linux-gnu/bin/ld: /tmp/cc1vRzOe.ltrans0.ltrans.o: in function `nanobind::module_& nanobind::module_::def<xgrammar::Grammar (*)(xgrammar::Grammar const&)>(char const*, xgrammar::Grammar (*&&)(xgrammar::Grammar const&)) [clone .isra.0]':
/tmp/pip-build-env-n3aop1u0/overlay/lib/python3.11/site-packages/nanobind/include/nanobind/nb_func.h:289:(.text+0x4c2): undefined reference to `nanobind::detail::nb_type_new(nanobind::detail::type_init_data const*)'
... (many dozens more 'undefined reference' errors) ...
/opt/gcc-15.1/lib/gcc/riscv64-unknown-linux-gnu/15.1.0/../../../../riscv64-unknown-linux-gnu/bin/ld: final link failed: bad value
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
* CMake build failed
error: subprocess-exited-with-error
× Building wheel for xgrammar (pyproject.toml) did not run successfully.
│ exit code: 1
╰─> See above for output.
note: This error originates from a subprocess, and is likely not a problem with pip.
ERROR: Failed building wheel for xgrammar
Failed to build xgrammar As the log shows, the linker fails with numerous Given this, without the conditional import, vLLM fails on startup with a hard To address your concern about "complex gating imports," I've updated the PR to use a "stub" class/function approach. This avoids adding any I believe this makes the conditional import both necessary and minimally invasive. Let me know your thoughts given this build failure. Thanks! |
Add platform detection to conditionally import xgrammar backend only on supported architectures (x86_64, aarch64, arm64). This fixes import errors on RISC-V, PowerPC, s390x and other unsupported platforms. Changes: - Add is_xgrammar_supported() helper in vllm/v1/structured_output/utils.py - Conditionally import XgrammarBackend in __init__.py based on platform - Conditionally import validate_xgrammar_grammar in processor.py Fixes import errors like: ModuleNotFoundError: No module named 'xgrammar' on platforms where xgrammar is not available per requirements/common.txt. Signed-off-by: lyd1992 <[email protected]> Signed-off-by: ihb2032 <[email protected]>
43d00ba
to
01e06a4
Compare
/gemini review |
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 correctly addresses an import failure for xgrammar
on unsupported platforms by introducing conditional imports based on CPU architecture. The approach of using a helper function is_xgrammar_supported
and providing dummy implementations is sound.
My review identifies one high-severity issue related to potentially unreachable code in the dummy XgrammarBackend
class. This could be misleading for future maintenance. I've suggested a change to make the code more robust and self-documenting by using NotImplementedError
to signal that this code path should not be executed.
def __init__(self, *args, **kwargs): | ||
raise ValueError( | ||
"xgrammar is not supported on this platform. " | ||
"Cannot initialize XgrammarBackend." | ||
) |
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 ValueError
raised here appears to be unreachable. The validation logic in Processor._validate_structured_output
should prevent XgrammarBackend
from being instantiated on an unsupported platform by either rejecting the request (if xgrammar
is explicitly requested) or falling back to another backend (in auto
mode).
If this code path is indeed unreachable, raising NotImplementedError
would be more appropriate than ValueError
to signal a programming error if it's ever triggered. This makes the code more robust and self-documenting. A comment explaining why this path is unexpected would also be beneficial.
def __init__(self, *args, **kwargs):
# This path should be unreachable. The validation in
# `Processor._validate_structured_output` should prevent
# `XgrammarBackend` from being initialized on unsupported platforms.
raise NotImplementedError(
"xgrammar is not supported on this platform and should not be "
"initialized."
)
Purpose
Fix ModuleNotFoundError when importing xgrammar on unsupported platforms (RISC-V, PowerPC, s390x, etc.).
Current Issues:
RISC-V users cannot use vLLM even with alternative backends available
Solution:
Test Plan
Test Result
Before Fix RISC-V Platform
ModuleNotFoundError: No module named 'xgrammar'
File "/AI/hebo/vllm/vllm/v1/structured_output/backend_xgrammar.py", line 131
After Fix RISC-V Platform
$ python -c "from vllm import LLM, SamplingParams"$
No errors - successfully imports core modules