-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Adding xgrammar #29064
Adding xgrammar #29064
Conversation
Hi! This is the staged-recipes linter and your PR looks excellent! 🚀 |
Hi! This is the friendly automated conda-forge-linting service. I wanted to let you know that I linted all conda-recipes in your PR ( Here's what I've got... For recipes/xgrammar/recipe.yaml:
For recipes/xgrammar/recipe.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13209596163. Examine the logs at this URL for more detail. |
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( I do have some suggestions for making it better though... For recipes/xgrammar/recipe.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/13209621265. Examine the logs at this URL for more detail. |
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
@conda-forge/help-python-c @conda-forge/help-python This should be ready for review now. |
Nice!!! Is it problematic to be vendoring these? https://github.com/mlc-ai/xgrammar/tree/v0.1.8/3rdparty |
Already handled. With dlpack I get it from conda and use the -Isystem line to fix the included since it's not included in the tarball because of the submodule so it works. It's a header only include that sets up a shared struct that has to be forward abi compatible from any version with any other library that uses it. The other is googletest but since we are not running tests we don't need it. |
Ah, good catch. Because it wasn't a submodule, I didn't think much of it. Header-only includes libs are funny things since they don't link anything, statically or dynamically. The gain by unvendoring in that case is only that if the source is fixed in some way all the dependent packages can get rebuilt. That other case in that other feedstock was funny in that it was trying to prevent picking up the unvendored version so I don't know what they were doing there. Maybe it's a case that they conditionally not use it or maybe it's used a feature also disabled. |
Ok, I dug into this after trying to use the conda-forge version. It turns out that picojson stopped being maintained in 2015, and now a series of hard forks exist across a dozen or so projects. A whole bunch of ML frameworks seem to be copying different updated versions from each other, making their own changes, and they are all slightly incompatible with each other now. So that explains why jwt-cpp-feedstock explicitly doesn't want to use the conda-forge version. Since it's a header only include you can just make it work by disabling warnings without disabling them everywhere. This looks like one of the latest forks folks are copying from, but the upstream project is dead-dead and has compile issues with modern C++ compilers because of const correctness. So, in this case, we can look at it as a fork and not a vendor version for the sake of being vendored. The upstream project is graveyard of 10+ years of ignored PRs to fix it and these changes all exist in everyone else's forks: https://github.com/kazuho/picojson/pulls?q=sort%3Aupdated-desc+is%3Apr+is%3Aopen |
Checklist
url
) rather than a repo (e.g.git_url
) is used in your recipe (see here for more details).