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

Updated numpy to 1.22 in CIs #105

Closed
wants to merge 2 commits into from
Closed

Updated numpy to 1.22 in CIs #105

wants to merge 2 commits into from

Conversation

radarhere
Copy link
Contributor

Checklist

  • Used a personal 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.

I presume most of the above checklist is not relevant to this PR, since I'm not modifying the package itself in any way. This is just a suggestion to improve the automatic testing.

@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.

Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

As noted in python-pillow/Pillow#5860 this is (AFAIU) just for testing, but just to avoid misunderstandings: this change doesn't do anything to the current CI and should not be merged.

@radarhere
Copy link
Contributor Author

Oh? I'm totally fine if this PR is rejected, but I would have thought it was better to test with newer versions of numpy.
Or have I missed something, and these version numbers are generated somehow?

@h-vetinari
Copy link
Member

h-vetinari commented Jan 3, 2022

I would have thought it was better to test with newer versions of numpy. Or have I missed something, and these version numbers are generated somehow?

You were changing things in migrators, which (aside from not being the right way to do it) would need a "rerender" of the feedstock to actually influence anything.

However, it's almost certainly not correct to build against numpy 1.22, which translates into a hard lower bound for runtime. Conda-forge takes care of this by having a global pinning at build time, but a newer-than-build-version at runtime, i.e. 1.22 will already be used.

However, I don't even see numpy in the list of dependencies here. As far as I can tell from upstream, it is an optional dependency that will be used when available?

@h-vetinari
Copy link
Member

h-vetinari commented Jan 4, 2022

If you tell me what you're trying to achieve, I might be able to help... (it's not obvious to me from your commits)

@radarhere
Copy link
Contributor Author

Just using trial and error to try and find some hint as to how to resolve the failure. If you would rather find a solution yourself, or if running your CI many times is a problem, I can stop.

@h-vetinari
Copy link
Member

If you would rather find a solution yourself, or if running your CI many times is a problem, I can stop.

I appreciate your help, and running CI is not a problem. Your changes just seem a bit... inexperienced with the conda-forge infrastructure, hence my offer to help.

@h-vetinari
Copy link
Member

h-vetinari commented Jan 4, 2022

If we're talking about the failures for py37/py38 on windows, then you seem to have found the source for them already. This then runs into some integrity check upon importing that doesn't strip quotes when comparing the versions (perhaps this is a recent change?).

@h-vetinari
Copy link
Member

If we're talking about the failures for py37/py38 on windows, then you seem to have found the source for them already.

We could also try patching out these extra quotes on the conda-forge side and see if that works (not necessarily something that needs to be upstreamed back to pillow).

@h-vetinari
Copy link
Member

Just using trial and error to try and find some hint as to how to resolve the failure.

As a high-level view, you should (almost) never have to edit anything outside the recipe folder.

@radarhere
Copy link
Contributor Author

If we're talking about the failures for py37/py38 on windows, then you seem to have found the source for them already. This then runs into some integrity check upon importing that doesn't strip quotes when comparing the versions (perhaps this is a recent change?).

That code hasn't changed since python-pillow/Pillow#4794, over a year ago, so not recently no.

As a high-level view, you should (almost) never have to edit anything outside the recipe folder.

I recognise that my changes aren't solutions. Pillow 8.4.0 previously passed in this repository, but now it is failing. So the cause is some other dependency that has upgraded in the meantime. My thinking was to try and figure out what the dependency was that changed, so that then the root cause of the problem could be identified.

We could also try patching out these extra quotes on the conda-forge side and see if that works (not necessarily something that needs to be upstreamed back to pillow).

If you don't think this needs to be upstreamed, then ok, sure, my investigation into the root cause can be stopped. Patching should absolutely solve the immediate problem.

@h-vetinari
Copy link
Member

My thinking was to try and figure out what the dependency was that changed, so that then the root cause of the problem could be identified.

That is a more tractable problem. We can have a look at the last passing CI run and the current ones, and then compare the environments (even with something as basic as diff). Assuming it is due to newer versions and not to some ambient changes (e.g. windows image has been updated), then it would be a simple matter of adding constraints in meta.yaml to return to prevent certain newer versions.

@h-vetinari
Copy link
Member

That code hasn't changed since python-pillow/Pillow#4794, over a year ago, so not recently no.

I meant whether the code that does the comparison leading to:

ImportError: The _imaging extension was built for another version of Pillow or PIL:
Core version: "9.0.0"
Pillow version: 9.0.0

had been changed recently...

@h-vetinari
Copy link
Member

I meant whether the code that does the comparison leading to [ImportError] had been changed recently...

I think this is the issue, tbh. python-pillow/Pillow#5776 removed PILLOW_VERSION but it's still "used" in that check.

@radarhere
Copy link
Contributor Author

I think this is the issue, tbh. python-pillow/Pillow#5776 removed PILLOW_VERSION but it's still "used" in that check.

core refers to our C code, and PILLOW_VERSION is still defined there.

And to answer your other question, the check code hasn't changed in two years.

@h-vetinari
Copy link
Member

core refers to our C code, and PILLOW_VERSION is still defined there.

I saw that it's coming from another module, but it still seems at least possible to me that the removal of PILLOW_VERSION affects this (even though I acknowledge that this PR runs into the same issue on 8.4.0, indicating perhaps that it's somehow compilation-related).

Following your lead, core.PILLOW_VERSION comes from here, which is passed through the already-mentioned section in setup.py, itself coming through src/PIL/_version.py.

To be very honest, I cannot discern what the extra quotes for py37&py38 should do, but it seems to go back to python-pillow/Pillow#2517.

@h-vetinari
Copy link
Member

Thanks a lot for the patch! I picked up a rebased version of this into #104

@radarhere radarhere closed this Jan 4, 2022
@radarhere radarhere deleted the numpy branch January 4, 2022 03:50
@h-vetinari
Copy link
Member

Thanks. From the logs there:

src/_imaging.c(4124): error C2440: 'type cast': cannot convert from 'double' to 'char *'
src/_imaging.c(4124): error C2143: syntax error: missing ';' before 'constant'
src/_imaging.c(4124): error C2224: left of '.dev0' must have struct/union type

(taking into account -DPILLOW_VERSION="9.1.0.dev0" further up).

So AFAICT, 9.1.0.dev0 gets chopped up into 9.1 (as a double) and the rest, causing a compilation error. So it must be a difference in the compiler setup (flags, versions, etc.) that causes this to be treated differently in conda-forge.

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

3 participants