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.

Sign up for GitHub

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

Jump to bottom

Unpin numpy<2 in build requirements #121

Closed
wants to merge 1 commit into from
Closed

Unpin numpy<2 in build requirements #121

wants to merge 1 commit into from

Conversation

pllim
Copy link
Contributor

@pllim pllim commented Oct 18, 2023

I need this to test astropy/astropy#15495

pllim added a commit to pllim/astropy that referenced this pull request Oct 18, 2023
@pllim
DO NOT MERGE: Try with liberfa/pyerfa#121
1710319
@pllim pllim mentioned this pull request Oct 18, 2023
Closed
1 task
@mhvk
Copy link
Contributor

mhvk commented Oct 18, 2023

@astrofrog, @saimn - see also #119 - is there any actual harm in just removing the numpy constraint altogether? As is, it seems to cause pain without a clear benefit.

@saimn
Copy link
Contributor

saimn commented Oct 18, 2023

>=1.25 is needed to build wheels that are compatible with numpy >= 1.21. Before that we were using oldest-supported-numpy which was using the oldest version of numpy for a given python version.
<2 is - I think - just a safety measure, we don't know if a release made currently (e.g. pyerfa 2.0.1) will build correctly with numpy 2.0 once it is released. This pin is needed when releasing but it can be removed in main. But I don't know how you can tell setuptools to use a numpy dev version when building (in the normal "isolated" mode).

@pllim
Copy link
Contributor Author

pllim commented Oct 18, 2023

Me neither... And tox does not really tell us in the log. @astrofrog said he has a cunning plan though. 🤞

@mhvk
Copy link
Contributor

mhvk commented Oct 18, 2023

Those constraints are (very) reasonable for building the wheels, but that does not mean they should be used in build_requirements generally - I think that should just exclude things that we know fail (which currently would seem to imply no constraint!).

So, it seems to me it is (right now), completely safe to remove the requirement as long as we can ensure that the build_and_publish part of ci_workflows.yml uses latest numpy. I guess one way of finding out is just to try: #122.

@astrofrog
Copy link
Contributor

See astropy/astropy#15506 for a possible way of testing the build with dev version of dependencies

pllim added a commit to pllim/astropy that referenced this pull request Oct 19, 2023
@pllim
DO NOT MERGE: Unpin pyerfa in install req
e73ef52 
because I want to install a version from my fork without tags

and try with liberfa/pyerfa#121
@pllim
Copy link
Contributor Author

pllim commented Oct 27, 2023

astropy no longer needs this after astropy/astropy#15524

@pllim pllim closed this Oct 27, 2023
@pllim pllim deleted the patch-1 branch October 27, 2023 14:21
lpsinger added a commit to lpsinger/ligo.skymap that referenced this pull request Nov 21, 2023
@lpsinger
Adjust macros and typedefs for Numpy C API
77eab45 
- Define `Py_LIMITED_API` and `NPY_TARGET_VERSION` macros in
  setup.py rather than in the C source code.
- Don't define Numpy's `struct _typeobject` if we are building for
  PyPy because PyPy doesn't implement Python's limited C API
  (see liberfa/pyerfa#120).
- Define `struct _typeobject` workaround after including Python.h,
  not before, because on PyPy, Python.h defines the macros that we
  depend on to detect whether we are building under PyPy.
- Use `NPY_TARGET_VERSION` for builds that are backwards-compatible
  with old versions of Numpy.
- Build with any version of Numpy >=1.25 and <2
  (see liberfa/pyerfa#121).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@pllim @mhvk @saimn @astrofrog