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
45 changes: 43 additions & 2 deletions .github/actions/build-src/action.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,40 @@ 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.

# Details on build isolation can be found here:
# https://pip.pypa.io/en/stable/reference/build-system/pyproject-toml/#build-isolation
#
# A short description of how this works in pratice is provided here:
# * If build isolation is on, pip will create wheels for MDAnalysis in a
# fully isolated temporary environment hence making sure that build
# requirements are handled independently of the runtime environment
# (whatever is already in your Python PATH at the time of install).
# For example, with build isolation on, you could have an old version
# of NumPy available in your current Python environment, but build with
# a much newer one.
# * If build isolation is off (through the --no-build-isolation flag), the
# pip build system depends on what is currently avaiable in your Python
# environment. So if NumPy is necessary and you have a version already
# installed, it will just use that directly. There is a massive potential
# for breaking an environment if misused, so this is very much advised
# only in cases where you know exactly what your enviroment does / has
# (such as the tests we run here).
#
# In practice we use build isolation in MDAnalysis CI when:
# * We are trying to test MDAnalysis with a different / older runtime
# version of a key build dependency (e.g. NumPy).
# We don't use build isolation when:
# * We know we have already installed the required dependencies earlier
# and we want to make sure that we use those dependnecies for the build
# (e.g. we want to check that the packages we pulled for conda-forge
# work for the build, not the ones from PyPi).
# * We want to test the build & runtime with a sepecial version of a
# build dependency that won't get picked up by the pip install. For
# example, when we use nightly wheels of NumPy.
descriptions: 'Use build isolation for pip installs, if false `--no-build-isolation` is used. '
required: true
default: false


runs:
Expand All @@ -19,6 +53,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 +69,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
10 changes: 10 additions & 0 deletions .github/workflows/gh-ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,15 @@ jobs:
with:
build-tests: true
build-docs: false
# If we specifically define an old NumPy version then we isolate the
# the build, i.e. make sure that we build with NumPy 1.25+ but we keep
# the old version of NumPy as what we use for runtime testing
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 +161,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