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

BLD: use scipy-openblas wheel #24839

Merged
merged 4 commits into from
Oct 4, 2023
Merged

BLD: use scipy-openblas wheel #24839

merged 4 commits into from
Oct 4, 2023

Conversation

mattip
Copy link
Member

@mattip mattip commented Oct 1, 2023

EDIT (Ralf): follow-up to gh-24749, landing most of it and postponing the more hairy bit - see #24749 (comment)

Follow on for MacPython/openblas-libs#87 which creates OpenBLAS wheels, available as scipy-openblas32 and scipy-openblas64. The wheel provides facilities for generating a scipy-openblas.pc file. Importing scipy-openblas will inject the dll/so into the process namespace and make the interfaces available. In order to use the wheel you must do one of these to build NumPy:

pip install scipy-openblas32 -r build_requirements.txt
PKG_CONFIG_PATH=./.openblas
spin build --with-scipy-openblas=32

or

pip install scipy-openblas64 -r build_requirements.txt
PKG_CONFIG_PATH=./.openblas
spin build --with-scipy-openblas=64

You can also use a non-spin pip install route:

pip install scipy-openblas32 spin
spin config-openblas --with-scipy-openblas=32
export PKG_CONFIG_PATH="${PWD}/.openblas"
pip install .

Significant changes:

  • use spin config-openblas across CI instead of the tools/openblas_support.py script
  • change the detection code in numpy/meson.build to:
    • check for scipy-openblas first by default (the default is openblas)
    • if scipy-openblas is found, use it as blas_name and skip cblas and lapack detection
  • add a PKG_CONFIG_PATH env variable so that the scipy-openblas.pc file can be found. Theoretically this could be added to the set of project defaults in the top-level meson.build via pkg_config_path, but I don't know if it has implications when setting PKG_CONFIG_PATH outside of meson. This can be cleaned up in a future PR?

Edit: PKG_CONFIG_PATH must be set, and link to the pkg_config_path meson build variable

@github-actions github-actions bot added the 36 - Build Build related PR label Oct 1, 2023
@mattip mattip force-pushed the openblas-wheel2 branch 4 times, most recently from e5f6475 to da7d8e9 Compare October 1, 2023 19:32
@mattip
Copy link
Member Author

mattip commented Oct 2, 2023

Hmm. The sdist build is failing to find _distributor_init_local.py when running python -m build. Since build copies only controlled source code to a temporary location, and since that file is excluded, build cannot find it.

@mattip
Copy link
Member Author

mattip commented Oct 2, 2023

This leads to a bit of a conceptual rabbit hole. Say I move the creation of _distributor_init_local.py into numpy/meson.build. Then the sdist will depend on scipy-openblasXX. But now there is no explicit dependency on scipy-openblasXX in the metadata. Installing a wheel built from that source in a new virtualenv would not know about scipy-openblasXX.

@rgommers
Copy link
Member

rgommers commented Oct 2, 2023

The revert in the last commit looks like the right way to handle this. python -m build by design builds sdist + wheels as they are released. There's no real need to use it for development purposes in combination with scipy-openblas.

@mattip
Copy link
Member Author

mattip commented Oct 2, 2023

CI is passing.

@rgommers rgommers added this to the 2.0.0 release milestone Oct 3, 2023
Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

This looks quite good, thanks @mattip! A few comments regarding reverting leftover changes from your initial attempt to also make wheel builds use scipy-openblas.

@@ -1,10 +1,12 @@
""" Distributor init file

Distributors: you can add custom code here to support particular distributions
of numpy.
Distributors: you can add a _distributor_init_local.py file to support
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to revert this doc change, since it's better for distributors to overwrite this file, for two reasons:

  1. no need to change existing build scripts
  2. keeps python -m build working, while adding an untracked _distributor_init_local.py file would not work since that file will not be included in the sdist (and wheel is built from sdist)

Copy link
Member Author

Choose a reason for hiding this comment

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

The original reason I made a change at all here is that it was too easy to run the scipy-openblas workflow locally (which modifies this file) and then mistakenly check the modified file in to VCS along with a bunch of other changes. If we revert this, perhaps we should add a check as part of the smoke test that the file has not been modified?

Copy link
Member

Choose a reason for hiding this comment

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

No need I think - we always had this file, and it's just not changing. This is no longer used by scipy-openblas (that uses _local) and for redistributions things work exactly like before.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, got it, just revert the doc change, leave the contents. OK. Will adopt the other suggestions here too.

Copy link
Contributor

Choose a reason for hiding this comment

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

This occurs with submodules and licence files frequently. I wish git had some facility that would complain if you tried to commit to a specified set of files, that you would have to give a force flag to overcome that resistance.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that would be nice. Too many contributors make mistakes with git submodules. And I even do it myself on occasion, since git commit -a is kinda muscle memory.

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 could add a check as part of smoke_test that all the files in a do_not_change.txt (including submodules) are not part of a PR. Then disable that check if the PR has the "allow_changed_immutable_state" label. The check would only run on PRs, not on merge to main.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I'm not so sure if for either of those the cure isn't as problematic as the disease. If I had to choose, I think the one from @andyfaff's link is nicer. The downside is:

If you set ignore = all, to get sane behavior with git commit -a, it will ALSO ignore the submodule in git show/diff when you EXPLICITLY add them. The only way to work-around the latter is using the command line option --ignore-submodule=none.

I'd probably leave it alone in NumPy; in SciPy it seems more like a toss-up.

tools/openblas_support.py Outdated Show resolved Hide resolved
tools/ci/cirrus_arm.yml Outdated Show resolved Hide resolved
tools/wheels/cibw_test_command.sh Outdated Show resolved Hide resolved
@rgommers
Copy link
Member

rgommers commented Oct 4, 2023

The one CI failure is on the PyPy on Windows job: TestBinop.test_ufunc_binop_interaction. That's showing up on other PRs too, so it should be unrelated.

Copy link
Member

@rgommers rgommers left a comment

Choose a reason for hiding this comment

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

LGTM now, let's give this a go! Thanks Matti, this makes working with OpenBLAS in CI quite a bit more smooth.

@rgommers rgommers merged commit 21d29c5 into numpy:main Oct 4, 2023
48 of 50 checks passed
@rgommers
Copy link
Member

rgommers commented Oct 4, 2023

So out of the plan we had discussed, which was:

  1. Make CI jobs use wheels
  2. Uploading more nightly wheels to the scientific-python bucket
  3. Allowing contributors and/or end users to use the openblas wheels
  4. Depending on the new openblas wheels in our own releases (last, likely blocked)

This takes care of (1) and (3) I believe. For (2), I see that the wheels have started landing at https://anaconda.org/scientific-python-nightly-wheels/scipy-openblas64/files and https://anaconda.org/scientific-python-nightly-wheels/scipy-openblas32/files, that looks like it's in good shape as well (right?).

So what's left is (4). The status of that was discussed in this thread: #24749 (comment). @mattip maybe you can open a new issue to summarize that, and then we can have a chat about how to best handle that? E.g., do nothing, wait for a proper solution connected to PEP 725, or have a more bespoke solution somehow. And whether this makes sense to do with ILP64 if SciPy remains on LP64 (that is fixable and of interest to have it move to ILP64, but more work).

@andyfaff
Copy link
Contributor

andyfaff commented Oct 4, 2023

Is there instructions on how to use wheels in downstream CI?

@rgommers
Copy link
Member

rgommers commented Oct 4, 2023

Is there instructions on how to use wheels in downstream CI?

For SciPy, the exact same as in this PR I think? And I'm not sure anyone else needs them.

@mattip
Copy link
Member Author

mattip commented Oct 4, 2023

Right, 1, 3 are complete. As for 2: currently there are weekly wheels for OpenBLAS HEAD for only a few select platforms. Theoretically we could build more if needed. I intend to update the workflow in linux_blas.yml to use the wheel-based workflow.

Is there instructions on how to use wheels in downstream CI

@andyfaff since the wheels building has not changed, downstream CI cannot currently use the scipy-openblas wheels: the openblas so/dlls are embedded in the wheel and RPATH / add_dll_path are used to find them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
36 - Build Build related PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants