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

Change handling of numpy x.x so that it is only needed as a build requirement #653

Closed
wants to merge 2 commits into from

Conversation

mwcraig
Copy link
Contributor

@mwcraig mwcraig commented Oct 28, 2015

I just noticed #650, which goes in the opposite direction to some extent. GIven that, let me start with an overview of what I see as the problem and solution...

The released implementation of numpy x.x (a desirable feature) is, arguably, broken because it is very easy to build a package which does not match its runtime dependencies as listed in info/index.json for the recipe, or has the wrong build id, or has a build id which indicate it was built against a different version of numpy than it was actually built against.

Some examples, organized by the numpy build/run requirements in meta.yaml:

  • Build requirement: numpy x.x; runtime requirement: numpy (or numpy >1.7 or anything except numpy x.x)
    • Builds against the numpy version in CONDA_NPY.
    • build_id is _pyX.Y_0 and requirement in info/index.json is whatever the runtime numpy requirement was listed as (numpy or numpy >=1.7 or whatever).
    • Result is probably not what the builder intended.
  • Build requirement: numpy; runtime requirement: numpy x.x
    • Builds against the latest numpy, or whatever numpy is matches the build requirement, which may not be CONDA_NPY
    • build_id is _npA.BpyX.Y_0 and requirement in info/index.json is numpy A.B, where the numpy version is CONDA_NPY.
    • Definitely NOT what the builder intended -- a package whose numpy requirement does not match the build version of numpy.

I don't know if this second case is addressed in #650, but would argue for this alternative syntax (convention?) instead, regardless of whether this PR is the right way to implement it:

  • If you want to build against a specific version of numpy that is determined by CONDA_NPY then put numpy x.x in the build requirement. This makes sense to me because CONDA_NPY is about how you build, not how you run.
  • Use the runtime numpy dependency specification to indicate which version(s) of numpy the package is compatible with, just as is done with other python packages. If numpy x.x is used as the runtime build spec then there is no way to express in the recipe what versions of numpy the package is, in principle, compatible with.
  • conda build must enforce CONDA_NPY in both build and run....this PR is one imlementation of that.

Pinging a few people on this who have comment on/raised issues related to the original numpy x.x (which is a good idea!): @pelson @Juanlu001 @seibert @rmcgibbo (please also look at #650).

Also, because I know they spend lots of time building: @ChrisBarker-NOAA @ocefpaf

@pelson
Copy link
Contributor

pelson commented Oct 28, 2015

Build requirement: numpy x.x; runtime requirement: numpy
... BROKEN...

I agree. My PR (#650) addresses this by preventing a definition of the build environment using `x.x``

Build requirement: numpy; runtime requirement: numpy x.x
...BROKEN...

The intention, at least from my perspective, is to state that I can build against any numpy (in this case, whatever you define in CONDA_NPY or else the latest numpy) and that at runtime you use the same MAJOR.MINOR version of numpy. Logically this is how I would expect to encode my recipe.

The alternative, that is the proposal put forwards here, which uses numpy x.x in the build dependency does not indicate to me that there is a hard runtime dependency on a specific numpy version. If we were to use the x.x notation in the build dependencies, where would we put the limiting factors, such as which numpy versions we could build with?

There are three types of built package - the ones which do not depend on numpy, the ones that depend on the Python interface, and the ones that depend on the numpy ABI (C interface). I think it is worth enumerating the usecases:

  1. a package which has no numpy runtime dependency, but needs numpy to build
  2. a package which has numpy python runtime dependency, but does not need numpy to build
  3. a package which has numpy python runtime dependency, and needs numpy to build
  4. a package which has numpy C runtime dependency, and needs numpy to build

If we change the numpy x.x in the build requirements to mean there is a runtime dependency on this exact version of numpy we:

a) lose the ability to control the numpy versions that we can build against
b) lost the ability to define a specific runtime version without specifying numpy as a build dependency (pretty minor)
c) end up having to implement another special case to deal with a pinned numpy in the dependencies (this one I like least of all, as I fully intend to roll the x.x concept out beyond Python and numpy in the future)

@mwcraig - is there still a facet of #650 which is broken? If so, would you be willing to iterate on that with me?

@mwcraig
Copy link
Contributor Author

mwcraig commented Oct 28, 2015

@pelson -- happy to! I thought it would be useful to lay out both possibilities for where the x.x goes (run or build) and see what arguments there are for each.

In the end as long as one of them gets merged and the docs get clarified I'm happy.

Looks like there is still an issue with #650, comments/suggestions will be made over there.

@mwcraig
Copy link
Contributor Author

mwcraig commented Nov 3, 2015

Closing this because I now think #650 is a better approach.

@mwcraig mwcraig closed this Nov 3, 2015
@github-actions github-actions bot added the locked [bot] locked due to inactivity label May 18, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked [bot] locked due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants