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

remove pin_run_as_build #44

Merged
merged 1 commit into from Apr 19, 2018

Conversation

msarahan
Copy link
Member

It is not good to do numpy here. Better to do it on the recipe level, not the variant level, as documented at https://conda.io/docs/user-guide/tasks/build-packages/variants.html#pinning-at-the-recipe-level

The reason is that when numpy is purely used for its python interface, not its C interface, we don't want to pin it in run depends at all. If we define the pin_run_as_build value, rather than using pin_compatible, it will always have a run constraint, even when we don't want it (i.e. with numpy's python interface only)

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

@ocefpaf ocefpaf merged commit bfff192 into conda-forge:master Apr 19, 2018
@ocefpaf ocefpaf mentioned this pull request Apr 19, 2018
@isuruf
Copy link
Member

isuruf commented Apr 19, 2018

You told me that ignore_run_exports is supposed to be used in that case. Usually numpy is not in the build section unless there's an extension being built. How about run_exports in the numpy package?

@msarahan
Copy link
Member Author

You also don't want run_exports in numy for exactly the same reason. You don't always want to impose its pin on things that don't need to be pinned.

@msarahan msarahan deleted the remove_pin_run_as_build branch April 19, 2018 15:47
@msarahan
Copy link
Member Author

PS: numpy should essentially never be in the build section. It may be in the host section.

Each of these are valid solutions for numpy: either (run_exports in numpy or pin_run_as_build for numpy, ignore_run_exports where it need not be pinned) or (no global pinning for numpy, explicit pin_compatible in recipes that need it).

We do the latter in defaults.

@isuruf
Copy link
Member

isuruf commented Apr 19, 2018

I think run_exports in numpy or pin_run_as_build for numpy, ignore_run_exports where it need not be pinned is better, since most maintainers are not aware of pinning stuff. For rare cases where you don't need a pinning, ignore_run_exports can be used. @conda-forge/core what do you think?

@ocefpaf
Copy link
Member

ocefpaf commented Apr 19, 2018

I think run_exports in numpy or pin_run_as_build for numpy, ignore_run_exports where it need not be pinned is better, since most maintainers are not aware of pinning stuff. For rare cases where you don't need a pinning, ignore_run_exports can be used. @conda-forge/core what do you think?

If I understand your rationale you want to let accidental pins happen instead of not pinning when it is not needed, right?

If so, I understand the logic but we currently have docs that tell users how to pin numpy. Wouldn't it be a matter of updating those docs?

@isuruf
Copy link
Member

isuruf commented Apr 19, 2018

PS: numpy should essentially never be in the build section. It may be in the host section.

Thanks. I'm still not used for the cb3 naming. 😄

If I understand your rationale you want to let accidental pins happen instead of not pinning when it is not needed, right?

Exactly. We are planning to do this for boost, right? boost need not be pinned when using header only parts. (This is what defaults does for boost)

If so, I understand the logic but we currently have docs that tell users how to pin numpy

We make mistakes in the reviews. For example, the brand new package https://github.com/conda-forge/pyprism-feedstock/blob/master/recipe/meta.yaml

@msarahan
Copy link
Member Author

we already expect users to do something like numpy x.x. It is reasonable to ask users to do this pinning.

If you choose the former option here, you are imposing a large cost to us, since all of our recipes already use the latter option. If you want to make that switch, I would ask that you help submit PRs to all of our recipes that use the latter option.

@jakirkham
Copy link
Member

Sorry Mike, didn't follow that last part, which is the option you are preferring? numpy x.x?

@msarahan
Copy link
Member Author

explicit pinning in recipes. Not using pin_run_as_build or run_exports in cases where there is dual use for a given package.

{{ pin_compatible('numpy') }}

That's ~equivalent to the older numpy x.x syntax.

@ocefpaf
Copy link
Member

ocefpaf commented Apr 19, 2018

We make mistakes in the reviews. For example, the brand new package https://github.com/conda-forge/pyprism-feedstock/blob/master/recipe/meta.yaml

Indeed we do, but like @msarahan said we chose that kind of error in lieu of the high CI cost that numpy x.x imposed to us. Although, our current numpy pinning brings the best of both worlds. I'm not sure how that fits in here.

@jakirkham
Copy link
Member

So, Mike, is that opening the door to NumPy matrix builds again or does it behave differently with cb3?

@isuruf
Copy link
Member

isuruf commented Apr 19, 2018

Recipes not in conda-forge, but in defaults are CVXcanon and mxnet right? Former is incorrect as it is using the C API, but no pin_compatible is used. Latter doesn't use the C API and needs ignore_run_exports if my proposal was used. recipes in conda-forge we can collaborate to fix.

Indeed we do, but like @msarahan said we chose that kind of error in lieu of the high CI cost that numpy x.x imposed to us.

No, I'm not suggesting we go to numpy x.x. I'm just saying use run_exports or `pin_run_as_build.

@ocefpaf
Copy link
Member

ocefpaf commented Apr 19, 2018

No, I'm not suggesting we go to numpy x.x

I did not mean to say you want us to go there, but that is what would happen in that case if I understand it correctly.

@isuruf
Copy link
Member

isuruf commented Apr 19, 2018

I did not mean to say you want us to go there, but that is what would happen in that case if I understand it correctly.

No. what would happen is that if the package was built with numpy 1.14, then it will only be usable with numpy >=1.14. If you want it usable with numpy 1.11, fix the recipe if the package uses the C API or add ignore_run_exports if it doesn't use the C API.

@msarahan
Copy link
Member Author

no, what I meant with numpy x.x is that users are manually specifying something that signifies that binary compatibility matters for numpy.

pin_compatible('numpy') will evaluate to something like numpy >=1.9,<2.0a0

@ocefpaf
Copy link
Member

ocefpaf commented Apr 19, 2018

TL;DR what @msarahan is proposing maps to our current pinning in https://conda-forge.org/docs/meta.html#building-against-numpyv then, right?

@msarahan
Copy link
Member Author

msarahan commented Apr 19, 2018

Yes. It replaces

build:
  - numpy 1.8.*  # [not (win and (py35 or py36))]
  - numpy 1.9.*  # [win and py35]
  - numpy 1.11.*  # [win and py36]
run:
  - numpy >=1.8  # [not (win and (py35 or py36))]
  - numpy >=1.9  # [win and py35]
  - numpy >=1.11  # [win and py36]

with

meta.yaml:

build:
  - numpy
run:
  - {{ pin_compatible('numpy') }}

and conda_build_config.yaml:

numpy:
  - 1.8   # [not (win and (py35 or py36))]
  - 1.9   # [win and py35]
  - 1.11  # [win and py36]

@isuruf
Copy link
Member

isuruf commented Apr 19, 2018

What happens is exactly the same in both proposals. The difference is what happens by default. pin by default or don't pin by default.

@msarahan's proposal special cases numpy, by making it not pin by default whereas others like openblas, zlib, etc are pin by default.
My proposal is to pin by default for all.

@msarahan
Copy link
Member Author

No, it is not pin by default. Your examples are poor. Boost is the only comparable package. Sometimes you want a pin, sometimes you don't. These packages are special, and a special case is appropriate.

@jakirkham
Copy link
Member

Could I ask that we add this as agenda item for the next meeting (if we haven't already)?

@mingwandroid
Copy link
Contributor

It seems to me the crux of this is that the C-level libs need different pinning than the python modules so why not use split packages? libnumpy for the C-level libs and numpy for the python parts?

@ocefpaf
Copy link
Member

ocefpaf commented Apr 19, 2018

It seems to me the crux of this is that the C-level libs need different pinning than the python modules so why not use split packages? libnumpy for the C-level libs and numpy for the python parts?

Even though that makes sense in the packaging world we would have a huge resistance from the python folks b/c they want to declare the dependencies as close as possible to their setup.py. Also, this kind of change is what PiPA uses to make the point against conda :-(

@jjhelmus
Copy link
Contributor

libnumpy for the C-level libs and numpy for the python parts?

This would be idea, unfortunately NumPy does not off a method to easily split itself into the C and Python parts. For better or worse, NumPy (and the numpy conda package) refers to both the C API and the Python API. The former needs to be pinned in a compatible way where as the later can be left as a unconstrained dependency.

My feeling is that there are far more packages that depend on the NumPy Python API and therefore do not need any pinning. Therefore an unpinned numpy should be the default. That said, when the C ABI is used and NumPy is not pinned bad things can happen and so I can understand the desire to pin by default.

@ocefpaf
Copy link
Member

ocefpaf commented Apr 19, 2018

@jjhelmus good summary! Let's pause this discussion here and move this to the meeting.

At the end of the day the choices are:

  1. pin by default
  2. do not pin by default but invest in docs and fix bad merges when the ABI pin was needed.

The pros and cons are:

  1. no need to pay attention is the package needs or not the pin / having a slightly more complex pinning scheme.
  2. alleviate an unnecessary / some packages may need to be fixed it they pass the review.
  3. Anaconda's existing recipes would need to be changed to remove {{pin compatible('numpy') }} over time / nil

@isuruf and others please edit my comment here at your leisure to add/remove whatever you want. We can take this message as a topic for the meeting.

@msarahan edited to add downside that pinning by default will require changes to Anaconda's recipes. This need not be a blocking change, it can be done over time. However, having both styles in recipes may confuse people.

@mingwandroid
Copy link
Contributor

Even though that makes sense in the packaging world we would have a huge resistance from the python folks b/c they want to declare the dependencies as close as possible to their setup.py. Also, this kind of change is what PiPA uses to make the point against conda :-(

I cannot see this as a valid complaint. Split packages are implementation details. No one should really care so long as the top-level, user-facing package isn't something they strongly dislike, e.g. "numerical-python" instead of "numpy".

I believe not doing split packages in this case adds technical debt to our ecosystem.

@ocefpaf
Copy link
Member

ocefpaf commented Apr 19, 2018

@mingwandroid would that require people to add libnumpy to their recipes that depends on the numpy ABI? If so, that is the complaint I imagine we would get from the python folks that would submit recipes to conda-forge.

@jjhelmus
Copy link
Contributor

Moving to using conda_build_config.yaml should avoid some of the pain when a package uses the C API and forgets to add pin_compatible in the recipe. Previously the latest version of NumPy would be used in the build phase so the undeclared compatibility would be numpy >= 1.14 (assuming 1.14 is the latest version of NumPy). With conda build 3, the NumPy version declared in conda_build_config.yaml is used which makes the undeclared compatibility much wider, either numpy >= 1.9 or numpy >= 1.11 depending on the platform. It is still possible to create a broken environment in these cases by explicitly installing an ancient version of NumPy, but it is much harder.

@jakirkham
Copy link
Member

Splitting numpy into C and Python APIs seems like an intractable problem. Haven't people tried to do that at the development level and essentially given up/written new things from scratch? As to doing this at the package level, am not convinced that is feasible either (though feel free to take that as a challenge and prove me wrong). That said, the idea is certainly intriguing!

Back to @ocefpaf's point though, think we have a lot to mull over before our next meeting when we can dig into this a bit more.

@isuruf
Copy link
Member

isuruf commented Apr 19, 2018

Some statistics (list of feedstocks might be a few hours old)

Recipes with numpy: 667
Recipes with only run: 374
Recipes with build correctly pinned: 180
Recipes with x.x: 29 (['asap', 'pyedflib', 'pygmo_plugins_nonfree', 'python-primesieve', 'fretbursts', 'sep', 'extinction', 'bob.math', 'pyopcode', 'qutip', 'gala', 'pynfft', 'pykrige', 'mpifft4py', 'pygypsy', 'pyhmc', 'scikit-bio', 'kapteyn', 'pyxdameraulevenshtein', 'megaman', 'gridfill', 'bob.blitz', 'deepgraph', 'pyhrf', 'msmbuilder', 'sklearn-contrib-lightning', 'cmt', 'limix-legacy', 'pyabel'])

Other pinnings. Except for the first line others are broken if they use the numpy C API because even with the conda_build_config.yaml they will have numpy 1.14 in the build.

no pins             ['weave', 'fastparquet', 'chemfiles-python', 'emcee', 'slepc4py', 'limix', 'reproject', 'jplephem', 'epic2cf', 'bob.ap', 'genpy', 'obspy', 'pyferret', 'sk-video', 'pymatbridge', 'caffe', 'pyepics', 'cis', 'dipy', 'astro-gala', 'lmfit', 'textadapter', 'nibabel', 'pytest-arraydiff', 'pyagrum', 'quantities', 'esmpy', 'wradlib', 'asteval', 'bob.learn.boosting', 'nilearn', 'forestci', 'pyopencl', 'glymur', 'pysurfer', 'esmp', 'iris', 'xtensor-python', 'mtspec', 'numpy-indexed', 'h5io', 'otwrapy', 'alpenglow', 'cfchecker', 'flopy', 'pyfmmlib']
>=1.10              ['autograd', 'somoclu']
>=1.13              ['batman', 'quaternion']
>=1.11.3,<1.12      ['cyminmax']
>=1.5               ['deap']
>=1.7               ['gpy', 'openpiv', 'emperor', 'zwatershed', 'pystan']
>=1.11              ['sunpy', 'pygypsy', 'mdsrv', 'pyuvdata', 'pyvtk', 'mpl-scatter-density', 'specutils', 'gala']
>=1.9               ['imbalanced-learn', 'pycalphad', 'openmc', 'galpy', 'ginga']
>=1.11.3            ['gensim']
>=1.10.0            ['ibis-framework']
>=1.8.0             ['resampy', 'spectral-cube', 'pyprism', 'librosa']
>=1.12              ['scikit-procrustes', 'limix-legacy']
>=1.14.1            ['numpy_sugar']
>=1.9.0             ['pvlib-python']
>=1.6               ['pydstool']
>=1.6,<1.12         ['pyhrf']
>=1.3.0             ['python-neo']
>=1.14.0            ['pytim']
>=1.9.2             ['scikit-bio']
>=1.11.0            ['stingray', 'hendrics']
>=1.11|!=1.14.0     ['sunpy']
>=1.8               ['trackpy']

Incorrect pins (start with >=) = 43
No pins = 46. Out of these 46, I checked a few and some do have np.get_include() in their setup.py or don't need numpy in build.

@ocefpaf
Copy link
Member

ocefpaf commented Apr 19, 2018

Recipes with x.x: 29

These are either abandoned or they did not have a new releases in ages. Not sure it is worth fixing those.

Incorrect pins (start with >=) = 43
No pins = 46. Out of these 46, I checked a few and some do have np.get_include() in their setup.py or don't need numpy in build.

Some of may be worth fixing.

@CJ-Wright
Copy link
Member

(I should fix pyxdameraulevenshtein the bot PR'ed it but it needs some work 🐑)

@mwcraig
Copy link
Contributor

mwcraig commented Apr 20, 2018

I'll fix up sep and extinction this weekend (they are in the numpy x.x pile)

@jjhelmus
Copy link
Contributor

Incorrect pins (start with >=) = 43

Not all of these are incorrect. If a packages does not use the NumPy C API but depends on a feature or bug fix from a particular NumPy version then having a lower bound on the version is perfectly reasonable. Such a requirement is typically not needed in the build section although there are odd setup.py files that check dependencies at build time in which case these would be required.

@mingwandroid
Copy link
Contributor

Such a requirement is typically not needed in the build section although there are odd setup.py files that check dependencies at build time in which case these would be required.

You mean host section here I think?

maresb pushed a commit to maresb/conda-forge-pinning-feedstock that referenced this pull request Nov 1, 2022
Add script to check proposed changes to repodata
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

9 participants