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

Build wheels with numpy 1.25+ #4198

Merged
merged 17 commits into from
Aug 15, 2023
Merged
15 changes: 13 additions & 2 deletions .github/actions/build-src/action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ inputs:
description: 'build MDA docs'
required: true
default: false
isolation:
Copy link
Member Author

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.

Copy link
Member

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:

  • default (i.e. isolate) installs new build-time dependencies that can be separate from runtime dependencies or previously installed packages
  • --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 versions

If 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?

Copy link
Member Author

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.

Copy link
Member Author

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.

descriptions: 'use build isolation for pip installs'
required: true
default: false


runs:
Expand All @@ -19,6 +23,7 @@ runs:
run: |
echo ${{ inputs.build-tests }}
echo ${{ inputs.build-docs }}
echo ${{ inputs.isolation }}

- name: check_setup
shell: bash -l {0}
Expand All @@ -34,14 +39,20 @@ runs:
- name: build_mda_main
shell: bash -l {0}
run: |
if [ "${{ inputs.isolation }}" == "false" ]; then
BUILD_FLAGS="--no-build-isolation"
fi
# install instead of develop would reduce coverage (for .pyx files)
cd package/ && python setup.py develop
python -m pip install ${BUILD_FLAGS} -v -e ./package

- name: build_mda_tests
if: ${{ inputs.build-tests == 'true' }}
shell: bash -l {0}
run: |
cd testsuite/ && python setup.py install
if [ "${{ inputs.isolation }}" == "false" ]; then
BUILD_FLAGS="--no-build-isolation"
fi
python -m pip install ${BUILD_FLAGS} -v -e ./testsuite

- name: build_docs
if: ${{ inputs.build-docs == 'true' }}
Expand Down
10 changes: 5 additions & 5 deletions .github/workflows/deploy.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The 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
4 changes: 4 additions & 0 deletions .github/workflows/gh-ci-cron.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ jobs:
with:
build-tests: true
build-docs: false
isolation: false
Copy link
Member Author

Choose a reason for hiding this comment

The 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: |
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

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

build with the deps you have installed


- name: run_tests
run: |
Expand Down Expand Up @@ -164,6 +167,7 @@ jobs:
with:
build-tests: true
build-docs: false
isolation: false
Copy link
Member Author

Choose a reason for hiding this comment

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

As above


- name: run_tests
run: |
Expand Down
7 changes: 7 additions & 0 deletions .github/workflows/gh-ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -152,6 +158,7 @@ jobs:
with:
build-tests: true
build-docs: true
isolation: false
Copy link
Member Author

Choose a reason for hiding this comment

The 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'
Expand Down
10 changes: 4 additions & 6 deletions azure-pipelines.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Copy link
Member Author

Choose a reason for hiding this comment

The 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
Expand Down
3 changes: 3 additions & 0 deletions package/CHANGELOG
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ Enhancements
(Issue #3546)

Changes
* Package builds are now made using NumPy 1.25 or higher. This provides
backwards runtime compatibility with older NumPy builds within the limit
of NEP29 (PR #4108).
* Removed `-ffast-math` compiler flag to avoid potentially incorrect
floating point results in MDAnalysis *and* other codes when
MDAnalysis shared libraries are loaded --- see
Expand Down
19 changes: 5 additions & 14 deletions package/pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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'",
Copy link
Member Author

Choose a reason for hiding this comment

The 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",
]
Expand Down
Loading