-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Add Grain feedstock #30294
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
Add Grain feedstock #30294
Conversation
|
Hi! This is the friendly automated conda-forge-linting service. I failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/15568776179. Examine the logs at this URL for more detail. |
|
@mtsokol Any update? |
|
Right, I'll have more time in the next two weeks to finish it. |
|
Thanks a lot, really appreciate your effort. |
|
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 ( |
|
Hi! This is the staged-recipes linter and I found some lint. File-specific lints and/or hints:
|
8e6c36a to
12557d1
Compare
|
Hi @iindyk, |
|
Hi, I'd like to be a maintainer of this feedstock |
7.2.1 is ok as long as the chain of dep updates will not force us to use a new protobuf dependency -- that is going to be painful. I think downgrade may be even more painful |
|
Thanks! Let me try to bump bazel to |
|
Hi! This is the staged-recipes linter and your PR looks excellent! 🚀 |
6b2948c to
9c41fee
Compare
@iindyk Do you know if that's something we have influence over? |
For example, perhaps we can run |
traversaro
left a comment
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.
See #30294 (comment) .
@traversaro Unfortunately we can't narrow the build to this cpp extension only. There is also https://github.com/google/grain/blob/main/grain/proto/execution_summary.proto protobuf file which needs to be build with bazel and I think it's the delinquent of abseil and protobuf builds that you reported. Is it a deal-breaker for conda-forge? |
Ah, I see! It would be cool to have some way to let bazel use the conda-forge's precompiled protobuf and abseil (I played around with this in https://github.com/ami-iit/bazel-cmake-deps-override in the past) but I guess this is not trivial.
As long as the releases are not so often, I guess this is not a big problem, even if the planet would probably be happier if we avoid wasting CI time. : ) However, this reference to
So if are sure that protobuf used by bazel to generate the Python is at least 6.32.0 should be fine? |
It looks like Grain has been being released once a month so far and it shouldn't increase IMO. The argument is undoubtedly correct - I can take a look at the precompiled bazel approach, but first I'll reach out to bazel team asking about it - here the bazel setup is relatively minimal, just one
Here we can't afford selecting protobuf version - we use exactly the one that TensorFlow uses to avoid version conflicts link. It's 28.3 right now and in some future version TF will surely reach 32. |
In that case, can you look into adding an appropriate upper bound for the |
590c7f7 to
9e7fec6
Compare
@traversaro Sure, I added protobuf generated module import in About an upper bound constraint: I would advocate to keep protobuf version constraint as For runtime dependencies upper bound isn't necessarily suitable:
I think the root cause here is the duality of using bazel within conda-forge build tools/setup which was already painful for array_record. Would you agree to leave it |
Sorry, why would you want to fox protobuf to 5.28 ? From what I understand from https://protobuf.dev/support/cross-version-runtime-guarantee/#python, shouldn't we just constraint protobuf to be <
I think the pro/cons evaluation may be a bit different in conda-forge, where we have repodata patches. On PyPI the metadata is effectively immutable, while in conda-forge we can patch it after the release. |
|
Ah, sorry! I misunderstood your request (for some reason I thought about fixing a specific version for it). Right, this upper bound makes sense - I added it. |
recipes/grain/meta.yaml
Outdated
| - more-itertools >=9.1.0 | ||
| - numpy | ||
| - protobuf >=5.28.3 | ||
| - protobuf >=5.28.3,<8.0.0 |
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.
Can you add a comment pointing to the protobuf docs? Otherwise it is quite difficult to track where this <8.0.0 comes from.
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.
Done!
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.
Not to be super pedantic, but usually these upper bounds are written like <8.0dev0 or similar to avoid RCs and pre-releases getting in the way.
| - protobuf >=5.28.3,<8.0.0 | |
| - protobuf >=5.28.3,<8.0dev0 |
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.
Thanks for the observation! @mtsokol probably it is better to do this before merging so the first build already have the correct metadata, thanks!
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.
Done! I included .rc0 that is the earliest RC which covers all potential protobuf releases of 8.x.
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.
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.
I can make it 8.0dev0, I haven't found any dev releases for protobuf in its conda-forge feedstock or pypi.
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.
Now it also considers dev in the release.
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.
I can make it
8.0dev0, I haven't found any dev releases for protobuf in its conda-forge feedstock or pypi.
That make sense, but please explain changes, as you may imagine reviewers can't read what you have in mind. : )
Anyhow, I guess the @jaimergp suggestion should cover also the rc case, so I think probably sticking to 8.0dev0 make sense.
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.
Sorry about that! And thank you for all your assistance for this PR!
|
Not sure if @jaimergp has more comments, but this is ok for me! If you want to squash and rebase on the latest master that is welcome! |
eea35b5 to
dfce854
Compare
|
Hi! This is the friendly automated conda-forge-linting service. I failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/19272559607. 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/grain/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/19292653517. Examine the logs at this URL for more detail. |
Nothing blocking, go ahead! |
Just a cross-ref to a couple bazel issues that could help in the future:
Even if the protobuf/absl case is definitely a difficult, as it requires somehow to also handle code generators, so probably in any case it would always require some glue code as not all the information required by bazel is present in |
Checklist
url) rather than a repo (e.g.git_url) is used in your recipe (see here for more details).