patch ray-default and ray-client to avoid 2.9.0 + grcpio1.58 #638
Conversation
|
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 ( |
Signed-off-by: mattip <matti.picus@gmail.com>
|
@mattip in my testing, 1.58 was broken against basically all. So I would do when ray <= 2.9 so folks on all previous versions are guarded |
|
For the record, as I stated in a comment on the original issue which @timkpaine deleted, there is no point patching this metadata until the ray feedstock itself is updated to prevent solving with grpcio 1.58. If you do not do this, we will be right back here again in a broken state as soon as the next PR is merged to the ray feedstock. |
|
@apmorton that's not true, actually the opposite. Changing it in the feedstock on the existing version does nothing at all as the solver can skip the patched build and choose the unpatched release. So this change has to happen if anything is to be done about existing versions. |
I fully understand this - but you cannot patch existing builds and leave the feedstock unchanged. If you do not apply this grpcio pin in the ray-packages feedstock the next build of ray will incorrectly be able to solve against grpcio 1.58 |
|
no reason to patch, just wait for the next release. it happens relatively frequently, I expect a 2.9.1 shortly anyway. Certainly no reason to wait for that to do this PR, this PR should be done now to fix the current state. |
|
And in the next build of the ray feedstock you will pin grpcio to prevent this from being reintroduced? That is certainly not the impression I get based on your replies in the original issue. |
|
@apmorton of course, but there are also some conversations over on the original ray project RE grpc compatibility/performance/long term path, and we should make sure to not just pin this and forget as it will likely crop up again. |
|
Hmm. I guess I misunderstood how the There is this comment in the PR template about future package resolutions:
So if I don't add that condition will the patch apply to future packages? Edit: uncomment the html comment in the quote |
|
@mattip future versions can have the pin added (e.g. the next ray release, assuming we're confident this is a one-off grpc problem), but modifying an existing release to have more restrictive pinning and incrementing a build doesn't really do anything. So in this repo, you can override resolution of those past releases to force a new version resolution. Here you'd say "ray less than or equal to 2.9.0, make sure grpc != 1.58" and the solver will respect that as if the pin had been in place in those prior releases. |
|
You are supposed to check that box to indicate your patch WON'T impact future builds - which is generally accomplished by adding a |
|
OK, I inverted the condition. Now the result of `$ python ./recipe/show_diff.py</details. |
|
I think
EDIT: the better way is |
|
I would merge a patch that pins exactly to 1.57. But something that allows new versions of grpcio to be installed would cause the problem to reappear in the future. |
| then: | ||
| - replace_depends: | ||
| old: grpcio | ||
| new: grpcio !=1.58 |
There was a problem hiding this comment.
| new: grpcio !=1.58 | |
| new: grpcio =1.57 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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....
There was a problem hiding this comment.
I think a pin to both 1.57 and 1.59 might be acceptable. However. I haven’t tested.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
so long as some upper bound on the version exists.
|
see other thread, I would not hard pin for now as we want to maintain as broad compatibility as we can, even at the introduction of some flakiness (which may need to be resolved with additional repodata patches when discovered). |
|
I see three choices:
Personally, closing this PR and not dealing with older versions makes the most sense to me. Thoughts? |
|
@timkpaine thoughts on @mattip's comment above? |
|
I don't like the idea of leaving older versions broken, but at the same time I don't want to be overly broad in potentially breaking older environments that don't notice a defect. I like @mattip's current changes without the suggested changes (e.g. mark known bad instead of forcing known good). |
|
I think the most correct thing to do at this point is update all previous versions of ray to add (the current version of this patch won't do anything since it says I will draft a new patch. |
Checklist
generate_patch_json.pyif absolutely necessary.pre-commit run -aand ensured all files pass the linting checks.python show_diff.pyand posted the output as part of the PR.This is my first PR to this repo. What I want to do is prevent the use of grpcio 1.58 with ray version2.9.0 and greater. Fixes conda-forge/ray-packages-feedstock#136 and conda-forge/grpc-cpp-feedstock#343.
I am not sure why
show_diffrepeats osx64 twice. Note that ray 2.9.0 is not yet supported on win64, that will be done once I fix conda-forge/ray-packages-feedstock#137.