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

DEV/CI: use scipy-openblas32 wheels in dev.py and some CI jobs #19380

Merged
merged 3 commits into from Oct 15, 2023

Conversation

rgommers
Copy link
Member

Addresses most of gh-19269. The newer OpenBLAS version seems to hang on 32-bit Linux in scipy.cluster tests, I'll open an issue for that. Switching over the wheel build jobs will be a follow-up to this PR, to not do too much at once.

The dev.py usage is slightly more polished than what numpy has, because there is no need to set PKG_CONFIG_PATH - that's taken care of from within dev.py. To use this locally:

$ pip install scipy-openblas32
$ python dev.py build --use-scipy-openblas   # in a clean env, not with an already configured build dir

After that, you can use all the dev.py commands without having to pass --use-scipy-openblas again (until you do git clean -xdf of course).

In CI, this gets rid of having to deal with manually locating the OpenBLAS shared library, something copying it over, putting it on LD_LIBRARY_PATH, etc.

@rgommers rgommers added the CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure label Oct 13, 2023
@rgommers rgommers added this to the 1.12.0 milestone Oct 13, 2023
.github/workflows/linux_meson.yml Show resolved Hide resolved
dev.py Show resolved Hide resolved
dev.py Show resolved Hide resolved
dev.py Show resolved Hide resolved
dev.py Show resolved Hide resolved
@@ -8,3 +8,8 @@
The SciPy standard source distribution will not put code in this file, so you
can safely replace this file with your own version.
"""

try:
Copy link
Contributor

@andyfaff andyfaff Oct 13, 2023

Choose a reason for hiding this comment

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

We say above that we're not going to put code in this file, but then we do.

Is this import only relevant in a dev.py process? What happens when we start to delvewheel/delocate/auditwheel in a wheel build?

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 say above that we're not going to put code in this file, but then we do.

Good point. I'm improving the description in the docstring.

Is this import only relevant in a dev.py process

Yes - or in editable/pip installs where we rely on the scipy-openblas wheels (not implemented yet.

What happens when we start to delvewheel/delocate/auditwheel in a wheel build?

Nothing different from what happens now. The _local file will not exist then, and the name mangling will still be the same.

Nicer than using the older tarballs, easier to understand and
maintain. The tarballs will also be dropped at some point, so
this is more future-proof.

[skip cirrus] [skip circle]
@rgommers
Copy link
Member Author

This should be good to go now, however the manylinux Docker images are not available right now (major outage says https://status.quay.io/), hence the two failed jobs.

@andyfaff
Copy link
Contributor

I might just test locally first before merge. Were you going to fix the nightly openblas wheel first?

@rgommers
Copy link
Member Author

Were you going to fix the nightly openblas wheel first?

It's already fixed, it's downloading from the scientific-python bucket. From the CI log of the pre-release job:

Collecting scipy-openblas32
  Downloading https://pypi.anaconda.org/scientific-python-nightly-wheels/simple/scipy-openblas32/0.3.24.52/scipy_openblas32-0.3.24.52-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (11.2 MB)

...

Run-time dependency scipy-openblas found: YES 0.3.24.dev

Copy link
Contributor

@tylerjereddy tylerjereddy left a comment

Choose a reason for hiding this comment

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

Nothing too surprising in the diff from my read through. I did check locally on an i9-13900K Linux box that a 32-process run of the full testsuite performs about the same on this branch with the OpenBLAS wheel as with spack-installed OpenBLAS (about 50 seconds or so).

I noted that if you don't have threadpoolctl installed, the punishment is larger with the distributed wheel than with a default spack install of a similar version of OpenBLAS, probably because the latter has threading disabled by default. I suppose that's what we'd expect though.

@andyfaff
Copy link
Contributor

Works well for me on macOS Sonoma.

@andyfaff
Copy link
Contributor

Thanks @rgommers for these changes. It's going to make developing a lot easier.

@andyfaff andyfaff merged commit d720703 into scipy:main Oct 15, 2023
21 checks passed
@rgommers rgommers deleted the use-openblas32-wheels branch October 15, 2023 11:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Items related to the CI tools such as CircleCI, GitHub Actions or Azure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants