Skip to content

so long and thanks for all the fish numpy x.x#415

Merged
ocefpaf merged 1 commit intoconda-forge:masterfrom
ocefpaf:numpy
Jul 27, 2017
Merged

so long and thanks for all the fish numpy x.x#415
ocefpaf merged 1 commit intoconda-forge:masterfrom
ocefpaf:numpy

Conversation

@ocefpaf
Copy link
Copy Markdown
Member

@ocefpaf ocefpaf commented Jul 21, 2017

@isuruf as requested here are some docs on the numpy pinning. Can you take a look?

@jjhelmus I would like your input too since you are the most experienced on this topic 😉

Comment thread src/meta.rst
conda-forge builds against.
If you have a package which links\* against numpy you need to build against the oldest possible version of numpy that is forwards compatible.
That can be achieved by pinning the build requirements and letting "free" the run requirements.
At the moment these are the oldest available numpy versions in conda-forge that you can use:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

These are not the oldest available versions in conda-forge right? They are pulled in from defaults, right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

FWICT it looks like they have been added as branches to the feedstock. So they are coming from conda-forge.

ref: https://github.com/conda-forge/numpy-feedstock/tree/numpy17
ref: https://github.com/conda-forge/numpy-feedstock/tree/numpy19

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

These are not the oldest available versions in conda-forge right? They are pulled in from defaults, right?

We added some LTS branches just for this.

Thanks for the review @isuruf!

@ocefpaf
Copy link
Copy Markdown
Member Author

ocefpaf commented Jul 21, 2017

One thing we should consider is maybe use only numpy 1.9 to simplify this. (I could not get numpy 1.7 to build with python >=3.5). What do you think @jjhelmus?

@mwcraig
Copy link
Copy Markdown
Contributor

mwcraig commented Jul 26, 2017

@conda-forge/core -- are we sure we want to do this? I agree that having fewer builds is better, but if I remember correctly, the original motivation for numpy pinning was that the ABI is not always compatible across releases.

If that has changed, great...and I know 100% for sure that I'm not up-to-date with the latest on numpy pinning. Mostly I'm writing based painful memories of about 4 years ago... 😰

@ocefpaf
Copy link
Copy Markdown
Member Author

ocefpaf commented Jul 26, 2017

This is the same strategy used for wheels and we need to watch for future numpy releases that may break the ABI, but we hope those will be well documented. For more info see @jjhelmus original PR to the pandas feedstock: conda-forge/pandas-feedstock#32

@mwcraig
Copy link
Copy Markdown
Contributor

mwcraig commented Jul 26, 2017

Thanks for the reference, @ocefpaf. A few more questions:

  • How will this interact with defaults? For example, is conda-forge has astropy 2.0.1 with python, but not numpy, pinned, and defaults has both numpy and python pinned, which gets installed? Does that break anything?
  • Will this change be compatible with what continuum plans to do in the future? That is not a show-stopper, but if we are trying to move towards using the same recipe for both we should make sure they are on board (I know @jjhelmus is at continuum now, so maybe this is already done).
  • Can we ask for feedback on the conda-forge list and/or open a CFEP to get feedback (and advertise the change)?

This seems like a big enough change in how we build that we should commit to it cautiously, and maybe ask for helping in additional testing.

@ocefpaf
Copy link
Copy Markdown
Member Author

ocefpaf commented Jul 26, 2017

For example, is conda-forge has astropy 2.0.1 with python, but not numpy, pinned, and defaults has both numpy and python pinned, which gets installed? Does that break anything?

In theory it should not b/c the numpy chosen would still be compatible as long as (a) it is within the version range and, (b) the mkl vs openblas linkages are properly resolved. The latter is the same situation we have now BTW.

Will this change be compatible with what continuum plans to do in the future?

We discussed this extensively at SciPy and I believe that is the direction defaults is going. Still, once we move to conda-build 3, this will be relatively easier to do.

Can we ask for feedback on the conda-forge list and/or open a CFEP to get feedback (and advertise the change)?

The changes are actually more trivial than they seem. Not sure we need a CFEP, but if you really want we can compile the various texts in the issue and submit one.

@mwcraig
Copy link
Copy Markdown
Contributor

mwcraig commented Jul 26, 2017

Thanks for the additional explanation!

The changes are actually more trivial than they seem. Not sure we need a CFEP, but if you really want we can compile the various texts in the issue and submit one.

Given your comments then maybe just announce on the conda-forge mailing list. If this is an opt-in thing (i.e. we are not going to run a script to rewrite recipes), then agree that it is not very disruptive.

@ocefpaf
Copy link
Copy Markdown
Member Author

ocefpaf commented Jul 26, 2017

If this is an opt-in thing (i.e. we are not going to run a script to rewrite recipes), then agree that it is not very disruptive.

It is opt-in but highly recommended, particularly for big python x numpy matrices that take too long to build. Still, there is nothing preventing you to use numpy x.x for as long as you want.

Thanks for looking at this @mwcraig!

@jakirkham
Copy link
Copy Markdown
Member

As noted in the pandas thread, I think trying to support all the way back to NumPy 1.7 is too ambitious. Currently we are unable to get such builds to link against OpenBLAS on macOS and I expect it will be a lot of work to make them work correctly. Only NumPy 1.9 seems to be built correctly. Would recommend that we simply drop NumPy 1.7. Then go with NumPy 1.9 and up for Python 2.7 instead. As a separate point, I think we should pull the broken NumPy 1.7 builds on macOS. ( conda-forge/numpy-feedstock#57 )

@ocefpaf
Copy link
Copy Markdown
Member Author

ocefpaf commented Jul 27, 2017

As noted in the pandas thread, I think trying to support all the way back to NumPy 1.7 is too ambitious.

I disagree that is ambitious b/c is works fine with all packages that supports np17, like matplolitb and pandas, and that does not links to openblas.

With that said I already mentioned that we should consider np19 as the lower bound for simplicity.

Currently we are unable to get such builds to link against OpenBLAS on macOS an I expect it will be a lot of work to make them work correctly. Only NumPy 1.9 seems to be built correctly. Would recommend that we simply drop NumPy 1.7. Then go with NumPy 1.9 and up for Python 2.7 instead. As a separate point, I think we should pull the broken NumPy 1.7 builds on macOS. ( conda-forge/numpy-feedstock#57 )

That is not an issue. See the logs of those builds for more information.

@ocefpaf
Copy link
Copy Markdown
Member Author

ocefpaf commented Jul 27, 2017

There is a concern regarding other blas than openblas. The only numpy without openblas is np17 and I could not find a single feedstock that could be built with np17 and links to openblas. Meaning that the use of np17 is safe to all who wants to support np17 and np18. (Also remember, conda-inspect is your best friend here!)

There seems to be some confusion on how this is supposed to work. This docs are examples only! And the guideline is "oldest possible numpy. So if your package does not support np17 go for np19, etc. (I am considering creating an np18 branch BTW. So maybe we could flatten the recommendation to a single numpy version.)

Merging this so people can modify the lingo here to fit their interpretations.

@ocefpaf ocefpaf merged commit e8b3de3 into conda-forge:master Jul 27, 2017
@ocefpaf ocefpaf deleted the numpy branch July 27, 2017 16:03
@kain88-de
Copy link
Copy Markdown
Contributor

How does conda smithy rerender work with this? It shouldn't do anything since this only depends on the meta.yaml right?

Also how can I best test a package when I want to convert it to use this scheme?

@ocefpaf
Copy link
Copy Markdown
Member Author

ocefpaf commented Aug 6, 2017

How does conda smithy rerender work with this? It shouldn't do anything since this only depends on the meta.yaml right?

We do need to re-render with conda-smithy after removing the numpy x.x just to remove the numpy versions from the build matrix. See https://github.com/conda-forge/pandas-feedstock/pull/32/commits for an example.

Also how can I best test a package when I want to convert it to use this scheme?

Using that building scheme the package will be built with the older numpy and run against the latest so, you need to come up with tests that would fail in that scenario. Usually just importing or running some basic stuff is enough, but you can try to run the package test suit too.

PS: here is the latest version of this docs https://github.com/conda-forge/conda-forge.github.io/pull/421/files

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.

5 participants