[Build] Upgrade xgrammar to get a security fix#36168
[Build] Upgrade xgrammar to get a security fix#36168russellb merged 2 commits intovllm-project:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request upgrades the xgrammar dependency to version 0.1.32 to address a security vulnerability. It also changes the version pinning from an exact version to a version range. My review focuses on the dependency versioning strategy. While the security fix is important, the new version range of >= 0.1.32, < 1.0.0 is too broad for a pre-1.0 library and could lead to future build breakages due to backward-incompatible changes in minor releases (e.g., 0.2.0). I have suggested a more restrictive version range to mitigate this risk while still allowing for patch updates.
Note: Security Review has been skipped due to the limited scope of the PR.
| diskcache == 5.6.3 | ||
| lark == 1.2.2 | ||
| xgrammar == 0.1.29; platform_machine == "x86_64" or platform_machine == "aarch64" or platform_machine == "arm64" or platform_machine == "s390x" or platform_machine == "ppc64le" | ||
| xgrammar >= 0.1.32, < 1.0.0; platform_machine == "x86_64" or platform_machine == "aarch64" or platform_machine == "arm64" or platform_machine == "s390x" or platform_machine == "ppc64le" |
There was a problem hiding this comment.
While upgrading xgrammar to fix the security vulnerability is a good move, the version constraint < 1.0.0 is too permissive for a pre-1.0 library and introduces a risk of future build failures.
According to Semantic Versioning, versions below 1.0.0 can introduce breaking changes even in minor releases (e.g., from 0.1.x to 0.2.0). The current constraint allows such upgrades automatically, which could lead to unexpected build or runtime errors if xgrammar releases a backward-incompatible 0.2.0 version.
To ensure stability and prevent future breakages, it's safer to restrict the allowed versions to the same minor series. This allows for patch releases (which should contain bug and security fixes) while preventing potentially breaking minor releases.
A more robust constraint would be >= 0.1.32, < 0.2.0.
xgrammar >= 0.1.32, < 0.2.0; platform_machine == "x86_64" or platform_machine == "aarch64" or platform_machine == "arm64" or platform_machine == "s390x" or platform_machine == "ppc64le"
There was a problem hiding this comment.
This was based on a conversation with @Ubospica
There was a problem hiding this comment.
This changes look good to me. XGrammar is planning a v0.2 release, but it is compatible with the prior API.
|
We should make sure we unblock relevant structured outputs tests before merging... |
5fbf867 to
b6324d4
Compare
|
Can we merge this now? |
xgrammar 0.1.32 includes a CVE fix: GHSA-7rgv-gqhr-fxg3 While we're at it, don't pin to specific version of xgrammar. After speaking to the primary maintainer, they feel it should be safe to not pin to a specific version at this point. We still have a "< 1.0.0" limit in place in case they want to do a major revision that may not be backwards compatible. Signed-off-by: Russell Bryant <rbryant@redhat.com>
b6324d4 to
0fcba2f
Compare
I see the main structured output tests running and passing in this CI run, so I'm not sure there's anything else that needs to be turned on. Let me know if I'm missing something, though! |
Signed-off-by: Russell Bryant <rbryant@redhat.com>
Signed-off-by: Russell Bryant <rbryant@redhat.com>
Signed-off-by: Russell Bryant <rbryant@redhat.com>
Signed-off-by: Russell Bryant <rbryant@redhat.com>
Signed-off-by: Russell Bryant <rbryant@redhat.com> Signed-off-by: Monishver Chandrasekaran <monishverchandrasekaran@gmail.com>
xgrammar 0.1.32 includes a CVE fix:
GHSA-7rgv-gqhr-fxg3
While we're at it, don't pin to specific version of xgrammar. After
speaking to the primary maintainer, they feel it should be safe to not
pin to a specific version at this point. We still have a "< 1.0.0"
limit in place in case they want to do a major revision that may not
be backwards compatible.
Signed-off-by: Russell Bryant rbryant@redhat.com