-
Notifications
You must be signed in to change notification settings - Fork 652
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
Build wheels with numpy 1.25+ #4198
Changes from 16 commits
d54db2f
21cecd1
ebc8b95
0926a7c
6f93755
e5aa75c
d4abcb3
2c48989
e00333a
520da67
717a6ce
edf9cb8
2ebccb5
bb4b019
2d04c4c
2bd32ca
a33cbd9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -241,18 +241,18 @@ jobs: | |
micromamba: true | ||
full-deps: true | ||
|
||
- name: install_min_deps | ||
if: "matrix.type == 'MIN'" | ||
run: | | ||
pip install pytest pytest-xdist pytest-timeout | ||
|
||
- name: pip_install_mda | ||
run: | | ||
awk '/__version__ =/ {print $3; exit}' package/MDAnalysis/version.py | tr -d \" > version.dat | ||
ver=$(python maintainer/norm_version.py --file version.dat) | ||
pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple MDAnalysis==$ver | ||
pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple MDAnalysisTests==$ver | ||
|
||
- name: install_min_deps | ||
if: "matrix.type == 'MIN'" | ||
run: | | ||
pip install pytest-xdist pytest-timeout | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moving this here because pip installing MDAnalysisTests should have already gotten pytest, you just need ot add pytest-xdist and pytest-timeout |
||
|
||
- name: run_tests | ||
run: | | ||
pytest --timeout=200 -n auto --pyargs MDAnalysisTests |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,7 @@ jobs: | |
with: | ||
build-tests: true | ||
build-docs: false | ||
isolation: false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. turn off isolation here because we want to build with the nightly wheels |
||
|
||
- name: run_tests | ||
run: | | ||
|
@@ -112,11 +113,13 @@ jobs: | |
run: | | ||
sed -i "s/#extra_cflags =/extra_cflags = -march=native -mtune=native/g" package/setup.cfg | ||
cat package/setup.cfg | ||
|
||
- name: build_srcs | ||
uses: ./.github/actions/build-src | ||
with: | ||
build-tests: true | ||
build-docs: false | ||
isolation: false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. build with the deps you have installed |
||
|
||
- name: run_tests | ||
run: | | ||
|
@@ -164,6 +167,7 @@ jobs: | |
with: | ||
build-tests: true | ||
build-docs: false | ||
isolation: false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As above |
||
|
||
- name: run_tests | ||
run: | | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -88,6 +88,12 @@ jobs: | |
with: | ||
build-tests: true | ||
build-docs: false | ||
isolation: ${{ matrix.numpy != '' }} | ||
IAlibay marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- name: check_deps | ||
run: | | ||
micromamba list | ||
pip list | ||
|
||
- name: run_tests | ||
if: contains(matrix.name, 'asv_check') != true | ||
|
@@ -152,6 +158,7 @@ jobs: | |
with: | ||
build-tests: true | ||
build-docs: true | ||
isolation: false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no need for isolation if we have all the dependencies here |
||
|
||
- name: doctests | ||
if: github.event_name == 'pull_request' | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -120,14 +120,12 @@ jobs: | |
pip list | ||
displayName: 'List of installed dependencies' | ||
- powershell: | | ||
cd package | ||
python setup.py install | ||
cd .. | ||
cd testsuite | ||
python setup.py install | ||
cd .. | ||
python -m pip install ./package | ||
python -m pip install ./testsuite | ||
IAlibay marked this conversation as resolved.
Show resolved
Hide resolved
|
||
displayName: 'Build MDAnalysis' | ||
condition: and(succeeded(), eq(variables['BUILD_TYPE'], 'normal')) | ||
- powershell: pip list | ||
displayName: 'Check installed packages' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Keeping this around so we can get an idea of what's in the environment ahead of testing |
||
- powershell: | | ||
cd testsuite | ||
pytest MDAnalysisTests --disable-pytest-warnings -n auto --timeout=200 -rsx --cov=MDAnalysis | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,20 +3,11 @@ | |
requires = [ | ||
"Cython>=0.28", | ||
"packaging", | ||
# lowest NumPy we can use for a given Python, | ||
# In part adapted from: https://github.com/scipy/oldest-supported-numpy/blob/main/setup.cfg | ||
# except for more exotic platform (Mac Arm flavors) | ||
# aarch64, AIX, s390x, and arm64 all support 1.21 so we can safely pin to this | ||
# Note: MDA does not build with PyPy so we do not support it in the build system | ||
# Scipy: On windows avoid 1.21.6, 1.22.0, and 1.22.1 because they were built on vc142 | ||
# Let's set everything to this version to make things clean, also avoids other issues | ||
# on other archs | ||
"numpy==1.22.3; python_version=='3.9' and platform_python_implementation != 'PyPy'", | ||
"numpy==1.22.3; python_version=='3.10' and platform_python_implementation != 'PyPy'", | ||
"numpy==1.23.2; python_version=='3.11' and platform_python_implementation != 'PyPy'", | ||
# For unreleased versions of Python there is currently no known supported | ||
# NumPy version. In that case we just let it be a bare NumPy install | ||
"numpy<2.0; python_version>='3.12'", | ||
# NumPy 1.25+ offers backwards compatibility that tracks with greater | ||
# than NEP29 (see: https://numpy.org/doc/stable/dev/depending_on_numpy.html#adding-a-dependency-on-numpy) | ||
# We pin our wheel installation to that and a maximum of numpy 2.0 since | ||
# this will likely involve several breaking changes | ||
"numpy>=1.25,<2.0; python_version>='3.9'", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NumPy 1.25+ is backwards compatible with its C/C++ API, so (as shown in the CI tests) we can build with it and then use an older version of NumPy at runtime and things just work. |
||
"setuptools", | ||
"wheel", | ||
] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're adding an option to trigger the pip --no-build-isolation flag. This will allow us to avoid
pip
trying to have its own isolated build & overidding already installed dependencies.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a couple readings of this comment and the pip documentation to understand (if I have this correctly) that:
--no-build-isolation
does not override previously installed packages (I think?) but makes use of what is currently installed, which is why we added this flag here to test different build package versionsIf that's right, could you please add that to the description of this flag, so later maintainers understand the use of the flag in our CI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly I'm not really 100% sure I know exactly how build isolation works, my take is that it creates an isolated environment with brand new everything to create wheels and then it gives you the wheels back in the world you first started in.
I'll try to add something sensible here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I've added a longer explanation and justification for how build isolation works, why we need it, etc...
I'm going to go ahead with the merge, but if you have the time to review what I wrote here and call out anything that's unclear in an issue that'd be great.