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 latest OpenBLAS #2

Merged
merged 1 commit into from
May 3, 2019

Conversation

tylerjereddy
Copy link
Collaborator

Build the latest stable release of OpenBLAS, v0.3.6, as requested by @charris based on evidence of Skylake-related issues reported downstream in NumPy, thanks to some debugging help from @matthew-brett:

The v0.3.6 release notes explicitly mention:

fixed building on SkylakeX systems when either the compiler or the (emulated) operating
environment does not support AVX512

@rgommers we may also want to consider bumping OpenBLAS for upcoming SciPy release if we think the tradeoff for changing versions from the rc to the final release is worth it for this possible issue?

@charris
Copy link
Collaborator

charris commented Apr 30, 2019

Who needs to merge this, @matthew-brett ?

@tylerjereddy
Copy link
Collaborator Author

Yes, and the results will not get uploaded to rackspace for the ecosystem to use until the build happens from master because of the API keys.

I think Matthew was open to giving me the appropriate permissions to merge at some point.

@matthew-brett
Copy link
Contributor

Certainly am - I've added you and Chuck as admins on the repo.

@charris
Copy link
Collaborator

charris commented Apr 30, 2019

Hmm, I wonder if the Python update affected the linux builds?

@tylerjereddy
Copy link
Collaborator Author

I wonder if the Python update affected the linux builds?

Seems consistent with the warning banner Travis produces for those jobs, and the logs from previous vs. current version bump.

I've amended the commit with an attempt to pin to Python 2.7 for now. If that works, maybe I'll open an issue here to remind us to bump to a more modern Python for the image build env eventually.

The 32-bit appveyor failure looked like some kind of download issue for mingw? Hopefully that goes away with the recent push.

@tylerjereddy
Copy link
Collaborator Author

Pinning Python version to 2.7 had no effect on Travis Linux error.

The first error is:

memory.c: In function ‘get_num_procs’:
memory.c:1775:10: error: invalid type argument of ‘->’ (have ‘cpu_set_t’)
      if (CPU_ISSET(i,cpuset)) n++;
          ^

The first point of substantial divergence with previous successful builds looks like the following, though it may not matter that much:

getarch.c:96:0: warning: "NO_AVX512" redefined [enabled by default]
 #define NO_AVX512
 ^
<command-line>:0:0: note: this is the location of the previous definition

@charris
Copy link
Collaborator

charris commented Apr 30, 2019

Yeah, that sounds like an OpenBLAS error, although I'd need to look at the source to verify.

@tylerjereddy
Copy link
Collaborator Author

I double checked the origin as the OpenBLAS source code and opened the cross-referenced issue upstream.

@rgommers
Copy link
Collaborator

@rgommers we may also want to consider bumping OpenBLAS for upcoming SciPy release if we think the tradeoff for changing versions from the rc to the final release is worth it for this possible issue?

If you want to do that, I would definitely do an rc2, it's a significant change. I'm not fully up to speed on why this is needed, you can make that call as release manager.

@charris
Copy link
Collaborator

charris commented Apr 30, 2019

I'm not fully up to speed on why this is needed

Buggy implementations for the SkyLake processor line. It can be fixed with an environmental variable specifying the processor, but I'd rather not burden naive users with that. I'd at least like to have the latest release tested for 1.17, and am thinking of using it for 1.16.4 with an rc1 first. But it looks like there is a bug in the last release of OpenBLAS, so we will see...

@tylerjereddy
Copy link
Collaborator Author

Yeah, looks like a typo upstream. We could circumvent by bumping GNU libc version apparently, but that seems like asking for trouble compared to waiting for a typo fix.

* build the latest OpenBLAS develop
branch commit; v0.3.6 can't be used
because of an issue with glibc compatibility
in older manylinux1 CentOS version

* apparently, latest OpenBLAS contains
fixes for Skylake-related
issues reported downstream in NumPy
@tylerjereddy
Copy link
Collaborator Author

This PR has been revised in the following ways:

  • now points to latest commit on OpenBLAS develop branch because the fix for the recently-encountered glibc issue was merged upstream
  • I removed the pinning to Python 2.7 in Travis--there was no evidence we needed this, and it seems like a liability long-term if we don't need to
  • adjusted commit message accordingly

@tylerjereddy
Copy link
Collaborator Author

CI is all green with latest dev version of OpenBLAS -- I've made a note to merge tomorrow (May 3/ 2019) if there are no objections before then, so that the ecosystem has access to updated binaries for trickle down/ testing.

@charris
Copy link
Collaborator

charris commented May 2, 2019

Sounds good.

@charris charris changed the title BLD: use latest stable OpenBLAS BLD: use latest OpenBLAS May 2, 2019
@tylerjereddy
Copy link
Collaborator Author

Ok, let's put this in so we have access to the binaries.

I'm also thinking about playing a little with the Intel SDE noted here when I asked OpenBLAS team about testing / CI: OpenMathLib/OpenBLAS#2108 (comment)

@tylerjereddy tylerjereddy merged commit 7d2e011 into MacPython:master May 3, 2019
@tylerjereddy tylerjereddy deleted the OpenBLAS-v0.3.6 branch May 3, 2019 14:53
mattip added a commit that referenced this pull request Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants