-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Bump OpenBLAS 0.3.9 #35113
Bump OpenBLAS 0.3.9 #35113
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, but I know Keno was working on the win32 patch very recently, so let's see what he says. Once he weighs in, we can merge the Yggdrasil PR, get binaries uploaded, update the BB release and checksums, then merge this.
On that note, from #34923 (comment)
I think the Yggdrasil and Julia base patches are still not in sync at the moment. |
109aaf7
to
0f30c45
Compare
Seems like the tests still pass on Ubuntu 18.04 with this bumped version of OpenBLAS:
so that looks promising. Can we trigger CI to not use binary builder btw? |
Not the case. Part of it is definitely still needed. |
Ok, so now the Windows patch is back and is identical to the one at JuliaPackaging/Yggdrasil#621 @staticfloat last difference between here and Yggdrasil is this change JuliaPackaging/Yggdrasil@b572419. Should the "Patch out |
Yes, we still need to carry that patch both here and in Yggdrasil. |
130c733
to
78fbde8
Compare
Ok, so now everything is in sync with the Yggdrasil PR |
81f9b71
to
f5197a9
Compare
Ok, so now that the BB OpenBLAS version is bumped we can finally see if it works with Julia 🤞 I've also bumped LAPACK to 3.9.0, but I don't really understand why it's still in the Makefile since it is almost always shipped with OpenBLAS? Edit: files changed 166 🙄 |
Thank you @haampie |
LAPACK is separately in the Makefile for the use case where you don't use it bundled with OpenBLAS. Increasingly a rare use case but useful for architectures where OpenBLAS may not be available. It should usually be the version of LAPACK that ships with the OpenBLAS we are using. |
tester_linuxaarch64 has linalg errors:
Seems like rounding errors mostly, but |
Stating the obvious. If its only on aarch64, that suggests possible openblas bugs. |
Is there any way to debug this easily? aarch64 docker image? |
I think @staticfloat may have something for you. |
You.... can.... do an aarch64 docker image, but running everything inside of QEMU can be error-prone for things like this. I suggest remoting into the aarch64 buildbot. PM me on Slack with your SSH public key and I'll give you temporary access to an environment where I have a |
Any chance we can figure out what the issue is and get this in before code freeze? |
Of course, I have a bad feeling it is an openblas bug, and we'll need a new release or pull in some patches... |
OpenBLAS 0.3.9 is much faster than earlier versions on systems with AVX512 in my benchmarks, so I hope these issues can be solved. Anyone have aarch64 to test? |
Best option is to send your ssh key to @staticfloat and he can give you access to the buildbot. |
Using the julia nightly on aarch64 from yesterday, and doing a few different runs, most of the errors seem to be related to tolerance and I am unable to reproduce all. The only one that seems worrisome as @haampie pointed out is:
The failure is clearly in the Float32 version of the hilbert matrix test case. However, it only gets triggered if you run the whole script. I am unable to reproduce it by itself.
|
@haampie Can you rebase this to master? |
It only fails the first time when run in tests. Can't reproduce it manually or in the debugger.
|
@chriselrod Would you be able to take a look? Maybe I am doing something wrong. Can you email/slack me your ssh public key if you have some time to look into this? |
Bumped to BB_REL 4. Let's see how this goes. |
I think you also need to regenerate the checksum files? |
Sigh. Yes. |
|
I think this is good news for linuxaarch64:
So there's a test failing, but it is not openblas-related. |
@haampie why not using |
I will from now on 😆 |
Let's try rebasing on master again to see if we get a green on CI. |
c4eb178
to
bbd1c5b
Compare
Tests are green 🎉 |
Hooray! Thank you everyone! This currently has the backport 1.5 label, does that mean it won't be in 1.5.0? Forgive my ignorance |
That means it will be in the 1.5 release (we also haven't branched yet). |
That was a lot of work for a version bump. Thanks @haampie! Taking off the label, since we have not yet branched. Is an openblas version update something we can expect in a 1.4 backport? |
Following @ViralBShah's comment in the linear algebra channel on Slack, it might be the right time to bump OpenBLAS to 3.9.0 for the Julia 1.5 release.
USE_BLAS_FFLAGS
toOPENBLAS_FFLAGS
. This addresses an issue where OpenBLAS does not build LAPACK with-fdefault-integer-8
and causes a segfault when running the OpenBLAS tests during the build (ref INTERFACE64 builds segfault with kernel_regress:skx_avx OpenMathLib/OpenBLAS#2429)Add theActually, I'm inclined to drop this... from the comments there they mention that-frecursive -fno-optimize-sibling-calls
flags toUSE_BLAS_FFLAGS
. I'm not 100% sure why this would be necessary, but read INTERFACE64 builds segfault with kernel_regress:skx_avx OpenMathLib/OpenBLAS#2429 where they include those flags.-frecursive
cannot hurt but does not seem to be mandatory, and-fno-optimize-sibling-calls
sounds very unrelated. The important bit is passing-fdefault-integer-8
to LAPACK I figure.Todo:
New todo: