[ROCm] Migrate xgrammar to upstream release#31327
[ROCm] Migrate xgrammar to upstream release#31327vllm-bot merged 2 commits intovllm-project:mainfrom
Conversation
Signed-off-by: Andreas Karatzas <akaratza@amd.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Code Review
This pull request updates the xgrammar dependency in requirements/rocm-test.txt from a forked repository to the official PyPI release xgrammar==0.1.29. This is a beneficial change, as it aligns with best practices by using an upstream release over a custom fork, which improves maintainability. The change is straightforward, and the author has confirmed that relevant tests pass on ROCm, mitigating the primary risk associated with such a migration. The PR is well-motivated and appears correct.
Signed-off-by: Andreas Karatzas <akaratza@amd.com>
|
Update: Bumped xgrammar to 0.1.29 in This resolves a dependency conflict with The reason we need 0.1.29 for ROCm: it includes a fix for the Triton kernel warp size on AMD GPUs (mlc-ai/xgrammar#476). Without this fix, structured output on MI300 fails with: The issue was that the Triton kernel hardcoded warp size to 64 (NVIDIA), but AMD GPUs use warp size 32. This caused the kernel to request more threads than available on AMD hardware. |
|
There is an AMD CI run for this PR before and after this addition, and it looks like everything is still the same: |
|
cc @tjtanaa |
|
Renamed the PR and triggered all tests as it also affects the default vLLM installation via |
|
@DarkLight1337 These b200 failures are not xgrammar related and are already known right? I have few of them already here: #30719 |
Signed-off-by: Andreas Karatzas <akaratza@amd.com>
Signed-off-by: Andreas Karatzas <akaratza@amd.com>
Signed-off-by: Andreas Karatzas <akaratza@amd.com> Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
Signed-off-by: Andreas Karatzas <akaratza@amd.com>
This PR updates the xgrammar dependency in
requirements/rocm-test.txtfrom a custom fork to the upstream PyPI release.Changes
xgrammar @ git+https://github.com/divakar-amd/xgrammar@3272f7c...withxgrammar==0.1.29Motivation
It's better practice to use upstream repositories instead of maintaining custom forks when possible. This is a follow-up to a comment from #29702 to eventually migrate to the upstream xgrammar repo.
Testing
Verified on ROCm with the following tests passing: