Skip to content
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.

Sign up for GitHub

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

patch ray-default and ray-client to avoid 2.9.0 + grcpio1.58 #638

Closed
wants to merge 2 commits into from
Closed

patch ray-default and ray-client to avoid 2.9.0 + grcpio1.58 #638

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0c19f0b
patch ray-default and ray-client to avoid 2.9.0 + grcpio1.58
mattip Jan 18, 2024
6e754e3
fix condition
mattip Jan 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions recipe/patch_yaml/ray-grpcio.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Prevent ray2.9.0 and grpcio 1.58 (https://github.com/conda-forge/ray-packages-feedstock/issues/136)
if:
name: ray-default
version_le: "2.9.0"
then:
- replace_depends:
old: grpcio
new: grpcio !=1.58
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
new: grpcio !=1.58
new: grpcio =1.57

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve seen and participated in the above discussion by asking relevant questions in the feedstock PR.

You asked why this is taking too long to merge in. Making patches that just have to be patched again later is not a sustainable strategy.

I personally don’t have enough knowledge of the details of grpcio to guarantee that 1.57 will work with 1.60 for example. If you want to allow 1.57 and 1.59 I might consider it. But following the grpcio pinnings, it doesn’t seem safe to leave the upper bound of grpcio unbound.

If an other core member agrees with your strategy and disagrees with mine they are entirely free to merge this. I am just applying conda-forge best practices to help review this request and get your package running.

please don’t outright dismiss the real concerns I raised in the feedstock PR.

Copy link
Member

@timkpaine timkpaine Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not "outright dismissing", please review all of the background linked previously for context that you might've missed with a quick screen. Hard pinning to a specific version will immediately break a lot of people who are otherwise fine. And thats certainly not great from a best practices or community standpoint! But feel free to not merge if you're not comfortable with the changes, there are other folks in core.

Copy link
Member

@timkpaine timkpaine Jan 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You asked why this is taking too long to merge in.

I did not, not sure where you got that from...I am a maintainer trying to get the right outcome with the smallest blast radius

get your package running

Not my package of course, I am just a volunteer....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a pin to both 1.57 and 1.59 might be acceptable. However. I haven’t tested.

Copy link
Member

@timkpaine timkpaine Jan 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apmorton has already shown 1.56 works as well. so the conservative approach is to disable known non-working versions, which I think is least disruptive. disabling all except for 1 (or 2) known-working version seems to be overly disruptive.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so long as some upper bound on the version exists.

---
if:
name: ray-client
version_le: "2.9.0"
then:
- replace_depends:
old: grpcio !=1.56.0
new: grpcio !=1.58,!=1.56.0
hmaarrfk marked this conversation as resolved.
Show resolved Hide resolved
- replace_depends:
old: grpcio
new: grpcio !=1.58
hmaarrfk marked this conversation as resolved.
Show resolved Hide resolved