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.

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

Blacklist pillow 7.1.* #56

Closed
wants to merge 10 commits into from
Closed

Blacklist pillow 7.1.* #56

wants to merge 10 commits into from

Conversation

phue
Copy link
Member

@phue phue commented Apr 21, 2020

There is a regression in the latest Pillow releases

python-pillow/Pillow#4528
The fix has been merged but not released yet

Checklist

  • Used a fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

There is a regression in the latest Pillow releases

python-pillow/Pillow#4528
The fix has been merged but not released yet
@conda-forge-linter
Copy link

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 (recipe) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

@phue
Copy link
Member Author

phue commented Apr 21, 2020

@conda-forge-admin, please rerender

recipe/meta.yaml Outdated Show resolved Hide resolved
Co-Authored-By: Egor Panfilov <egor.v.panfilov@gmail.com>
@jni
Copy link
Contributor

jni commented Apr 22, 2020

Thanks for handling this, @phue!

I must say I don't understand the CI failures... Two appear to be some numerical precision issues that for some reason don't appear in the other builds, and the last one appears to be a dependency conflict.

The numerical precision issues could be easily dealt with with a patch, but I don't know how to deal with the dependency conflicts. @jakirkham? 😬

@phue
Copy link
Member Author

phue commented Apr 22, 2020

@conda-forge-admin, please rerender

@phue
Copy link
Member Author

phue commented Apr 22, 2020

Thanks for handling this, @phue!

I must say I don't understand the CI failures... Two appear to be some numerical precision issues that for some reason don't appear in the other builds, and the last one appears to be a dependency conflict.

The numerical precision issues could be easily dealt with with a patch, but I don't know how to deal with the dependency conflicts. @jakirkham? grimacing

I don't understand them either.
Rerendering with latest conda-smithy made them disappear again. But now there is another error which I also don't understand, this time related to tcl

@hmaarrfk
Copy link
Contributor

I'm not sure if this is going to have the effect you want.

THese kinds of issues are rather typical in rolling releases like conda-forge.

Maybe you can ask the package to be removed if the issue is serious enough?

@jakirkham
Copy link
Member

I'm think we should just do a repodata patch if the issue is this version of pillow has issues. We could then apply this to both the current scikit-image package and older versions. WDYT?

@hmaarrfk
Copy link
Contributor

repodata patch

I'm not sure what this is.

@jakirkham
Copy link
Member

The idea would be to patch repodata for the conda-forge channel to alter the requirements of scikit-image packages. Patching is applied based on code here. Here's a somewhat recent patch that distributed needed as an example ( conda-forge/conda-forge-repodata-patches-feedstock#32 ).

@hmaarrfk
Copy link
Contributor

that sounds harder than simply pulling pillow 7.1

@jakirkham
Copy link
Member

What about applying the upstream fix as a patch to pillow then?

@hmaarrfk
Copy link
Contributor

What about applying the upstream fix as a patch to pillow then?

I'm not super in the patching mood today, but yeah, that would probably be easiest.

I also don't have power on the Pillow feedstock, so :/

@phue
Copy link
Member Author

phue commented Apr 24, 2020

I'm not sure if this is going to have the effect you want.

THese kinds of issues are rather typical in rolling releases like conda-forge.

Could you elaborate more on this? I currently don't understand why blacklisting is not a viable option. And I don't see how the CI failure is related to the change I made

@hmaarrfk
Copy link
Contributor

I currently don't understand why blacklisting is not a viable option.

The solver has been changing recently, so if somebody types conda install pillow then types conda install scikit-image I think they will get pillow 7.1.

And I don't see how the CI failure is related to the change I made

Neither do I. I just skipped the test.

What platform do you use? I could just merge this even if it is failing if you want....

@hmaarrfk
Copy link
Contributor

honestly, i'm not too sure how the solver works out, so i'm willing to try this out, I just wanted to say that typically, the best way is to ping upstream for a fix, or to patch up the offending repo.

@phue
Copy link
Member Author

phue commented Apr 24, 2020

The solver has been changing recently, so if somebody types conda install pillow then types conda install scikit-image I think they will get pillow 7.1.

OK, now I get it that's unfortunate. I think the current situation is even worse though, because anyone typing conda install scikit-image will get a dysfunctional installation

honestly, i'm not too sure how the solver works out, so i'm willing to try this out, I just wanted to say that typically, the best way is to ping upstream for a fix, or to patch up the offending repo.

I fully agree on that, in this case the fix has already been merged upstream so it's just pending a new release. Patching the pillow feedstock is of course an option, I just thought this would be in line with what was done for pip here

@hmaarrfk
Copy link
Contributor

it seems that you are hitting a nasty bug on OSX.. even if i merged, it would give you inconstient result on different platforms

@hmaarrfk
Copy link
Contributor

i'm ok skipping those tk tests, but not so sure about skipping the OSX test.

I'll let @jni review before merging

@phue
Copy link
Member Author

phue commented Apr 24, 2020

Interesting, but this will affect the next scikit-image release as well, regardless of the pillow breakage.

@hmaarrfk
Copy link
Contributor

i totally agree, but that is a "tomorrow" problem ;)

@hmaarrfk
Copy link
Contributor

Pillow 7.1.2 has just been released.

I'm inclined to close this for today. What do you think?

@phue
Copy link
Member Author

phue commented Apr 26, 2020

Absolutely, good that it is now resolved upstream

@phue phue closed this Apr 26, 2020
@hmaarrfk
Copy link
Contributor

Nice. Yeah. Itis really nice when projects can respond to quick bugs like that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants