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

Upgrade to OpenBLAS 0.3.13 #39216

Merged
merged 5 commits into from
Feb 20, 2021
Merged

Upgrade to OpenBLAS 0.3.13 #39216

merged 5 commits into from
Feb 20, 2021

Conversation

omus
Copy link
Member

@omus omus commented Jan 12, 2021

Fixes #39201. Requires JuliaPackaging/Yggdrasil#2385.

Possibly this PR also should include the neoverse-generic-kernels.patch used with the BinaryBuilder version. (the additional patches are for GCC and don't apply here)

@omus omus added the external dependencies Involves LLVM, OpenBLAS, or other linked libraries label Jan 12, 2021
@omus
Copy link
Member Author

omus commented Jan 12, 2021

I'll mention that the patches here and in the BB version have minor differences but should be functionally identical

@dkarrasch
Copy link
Member

Should close #36976 and #38865.

@omus omus changed the title Upgrade to OpenBLAS 0.3.13 WIP: Upgrade to OpenBLAS 0.3.13 Jan 13, 2021
@omus
Copy link
Member Author

omus commented Jan 16, 2021

I've successfully ran the Julia test suite locally with OpenBLAS 0.3.13 using dependencies built from source. I just need to get the BinaryBuilder version working.

@omus omus changed the title WIP: Upgrade to OpenBLAS 0.3.13 Upgrade to OpenBLAS 0.3.13 Jan 21, 2021
@omus
Copy link
Member Author

omus commented Jan 21, 2021

Testers seem to have this error (I just spot checked a couple):

precompile                         (1) |        started at 2021-01-21T15:46:04.840
julia: /workspace/srcdir/llvm-project/llvm/lib/IR/Value.cpp:460: void llvm::Value::doRAUW(llvm::Value*, llvm::Value::ReplaceMetadataUses): Assertion `New->getType() == getType() && "replaceAllUses of value with new value of different type!"' failed.
signal (6): Aborted
in expression starting at /buildworker/worker/tester_linux64/build/share/julia/test/precompile.jl:850

@omus
Copy link
Member Author

omus commented Jan 21, 2021

I had some strange building issues locally that have since disappeared. I'll try a CI re-run as I'm unable to reproduce locally

@omus omus closed this Jan 21, 2021
@omus omus reopened this Jan 21, 2021
@omus
Copy link
Member Author

omus commented Jan 27, 2021

I've been waiting on JuliaPackaging/Yggdrasil#2431 before investigating these CI issues further.

@pablosanjose
Copy link
Contributor

pablosanjose commented Jan 29, 2021

There is a fix to a LAPACK-netlib bug for OpenBLAS here. Is there any chance it could be included here as a patch? Or do we need to wait for OpenBLAS 0.3.14 to be released?

@omus
Copy link
Member Author

omus commented Jan 29, 2021

There is a fix to a LAPACK-netlib bug for OpenBLAS here. Is there any chance it could be included here as a patch? Or do we need to wait for OpenBLAS 0.3.14 to be released?

It's fine to include the patch here.

@omus
Copy link
Member Author

omus commented Jan 29, 2021

CI error seems to be the same as: #27055. Unfortunately I can't trigger the issue locally by changing the optimization level

@pablosanjose
Copy link
Contributor

pablosanjose commented Jan 30, 2021

It's fine to include the patch here.

Should I do a PR onto this PR? The patch is quite simple, just this

EDIT: updated the gist above with a typo correction, as noted here, after confirming with an OpenBLAS maintainer.

@omus
Copy link
Member Author

omus commented Feb 1, 2021

Just providing the patch is good enough. I can incorporate the patch here but I may start with it disabled until I sort out the CI issue.

@pablosanjose
Copy link
Contributor

Many thanks! The issue addressed by the patch is making my life complicated, given that it produces convergence errors that are difficult to predict and are platform-dependent. I really appreciate this!

@omus
Copy link
Member Author

omus commented Feb 1, 2021

Disabling the test that seems to cause the failure on CI just to see if that's the only issue

@omus
Copy link
Member Author

omus commented Feb 1, 2021

The patch is quite simple, just this

The patch you provided appears to be corrupt. I've regenerated the patch. If you could make sure it still is correct that would be great

@pablosanjose
Copy link
Contributor

pablosanjose commented Feb 1, 2021

Uh, apologies, I actually tried to adapt the full patch at Reference-LAPACK/lapack#477 into an OpenBLAS patch (the relevant files are identical), but I guess that's not how this is done (I've never done this before, sorry). Unfortunately, the OpenBLAS PR OpenMathLib/OpenBLAS#3091 you took the patch from was merged before the full fix was made upstream in Reference-LAPACK/lapack#477, so that patch you posted is actually incomplete... Is there a way to adapt Reference-LAPACK/lapack#477 into an OpenBLAS patch?

EDIT: If that is not actually possible, I would just use the patch you've generated, as I think it is enough to solve the core of the convergence problem

@pablosanjose
Copy link
Contributor

My confusion above was due to the fact that the original fix in Reference-LAPACK/lapack#477 has been ported to OpenBLAS in two parts, OpenMathLib/OpenBLAS#3087 and OpenMathLib/OpenBLAS#3091. When you corrected my corrupted patch I believe you made a patch with only the latter PR, but not the former. I'm pretty sure they are both necessary. The question is how to do a single patch that applies two PRs. I'm afraid I don't know how to do that, but perhaps you do?

@pablosanjose
Copy link
Contributor

pablosanjose commented Feb 11, 2021

Is it clear what is the cause of the CI failure here?

[The failure ([1] pipeline_error @ ./process.jl:525 [inlined]) reminds me of this issue, if that's any help]

Bumping to support xcode/clang 12 which was addressed in OpenBLAS 0.3.11
@omus
Copy link
Member Author

omus commented Feb 14, 2021

Patch should now include OpenMathLib/OpenBLAS#3087 and OpenMathLib/OpenBLAS#3091.

@omus
Copy link
Member Author

omus commented Feb 14, 2021

After applying the revised patch and rebasing I am unable to reproduce the doctest failure or the issue with opaque closures locally.

@omus
Copy link
Member Author

omus commented Feb 14, 2021

Looks like the opaque closure failures were fixed somewhere else. I've managed to reproduce the docstrings failures in a Linux environment but oddly they don't fail on macOS

@ViralBShah ViralBShah added the linear algebra Linear algebra label Feb 20, 2021
@omus
Copy link
Member Author

omus commented Feb 21, 2021

Probably should have removed the WIP from the commit message. What's our feeling on changing it?

@ViralBShah
Copy link
Member

I was mostly looking at the diff and not the commit message. I'm not sure if things can be(?) changed on master - but there are a few other wip commit messages in the log. I'll leave it to the git specialists to decide.

I am removing the WIP from the PR title though.

@ViralBShah ViralBShah changed the title WIP: Upgrade to OpenBLAS 0.3.13 Upgrade to OpenBLAS 0.3.13 Feb 21, 2021
sthagen added a commit to sthagen/JuliaLang-julia that referenced this pull request Feb 21, 2021
@KristofferC KristofferC added the backport 1.6 Change should be backported to release-1.6 label Mar 22, 2021
KristofferC pushed a commit that referenced this pull request Mar 23, 2021
* Use OpenBLAS 0.3.13

Bumping to support xcode/clang 12 which was addressed in OpenBLAS 0.3.11

* Use OpenBLAS 0.3.13+1

* Add openblas-exshift patch for src build

* Update LinearAlgebra doctests for Linux

* non-ambiguous ordering in eigen and eigvals test (#39767)

add missing sortby's

Co-authored-by: Pablo San-Jose <lekand@gmail.com>
(cherry picked from commit 3129a5b)
@KristofferC KristofferC mentioned this pull request Mar 23, 2021
10 tasks
KristofferC pushed a commit that referenced this pull request Mar 26, 2021
* Use OpenBLAS 0.3.13

Bumping to support xcode/clang 12 which was addressed in OpenBLAS 0.3.11

* Use OpenBLAS 0.3.13+1

* Add openblas-exshift patch for src build

* Update LinearAlgebra doctests for Linux

* non-ambiguous ordering in eigen and eigvals test (#39767)

add missing sortby's

Co-authored-by: Pablo San-Jose <lekand@gmail.com>
(cherry picked from commit 3129a5b)
@KristofferC KristofferC mentioned this pull request Mar 26, 2021
33 tasks
@KristofferC
Copy link
Member

Based on #40279 I will take off the backport label here. It can be put back until it is resolved.

@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Apr 4, 2021
@ViralBShah ViralBShah added the backport 1.6 Change should be backported to release-1.6 label Apr 4, 2021
@ViralBShah
Copy link
Member

ViralBShah commented Apr 4, 2021

Fixed through new binaries in JuliaPackaging/Yggdrasil#2769. But also needs #40343 to go with it.

KristofferC pushed a commit that referenced this pull request Apr 6, 2021
* Use OpenBLAS 0.3.13

Bumping to support xcode/clang 12 which was addressed in OpenBLAS 0.3.11

* Use OpenBLAS 0.3.13+1

* Add openblas-exshift patch for src build

* Update LinearAlgebra doctests for Linux

* non-ambiguous ordering in eigen and eigvals test (#39767)

add missing sortby's

Co-authored-by: Pablo San-Jose <lekand@gmail.com>
(cherry picked from commit 3129a5b)
@KristofferC KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Apr 6, 2021
@KristofferC
Copy link
Member

KristofferC commented Apr 6, 2021

#40361 (removing backport label again)

@ViralBShah
Copy link
Member

Wow I can't imagine what is causing that.

@KristofferC
Copy link
Member

Me neither, it was discussed a bit in the #binarybuilder channel in Slack but nothing concrete came out of it.

@omus
Copy link
Member Author

omus commented Apr 8, 2021

#40361 has been addressed (details: #40361 (comment)). Should the backport label be added again?

@ViralBShah
Copy link
Member

ViralBShah commented Apr 8, 2021

Given that 1.7 is not too far away (timed releases going forward), and we've found various openblas related issues, it might be best not to backport.

ElOceanografo pushed a commit to ElOceanografo/julia that referenced this pull request May 4, 2021
* Use OpenBLAS 0.3.13

Bumping to support xcode/clang 12 which was addressed in OpenBLAS 0.3.11

* Use OpenBLAS 0.3.13+1

* Add openblas-exshift patch for src build

* Update LinearAlgebra doctests for Linux

* non-ambiguous ordering in eigen and eigvals test (JuliaLang#39767)

add missing sortby's

Co-authored-by: Pablo San-Jose <lekand@gmail.com>
antoine-levitt pushed a commit to antoine-levitt/julia that referenced this pull request May 9, 2021
* Use OpenBLAS 0.3.13

Bumping to support xcode/clang 12 which was addressed in OpenBLAS 0.3.11

* Use OpenBLAS 0.3.13+1

* Add openblas-exshift patch for src build

* Update LinearAlgebra doctests for Linux

* non-ambiguous ordering in eigen and eigvals test (JuliaLang#39767)

add missing sortby's

Co-authored-by: Pablo San-Jose <lekand@gmail.com>
@vchuravy vchuravy added the backport 1.6 Change should be backported to release-1.6 label Jan 12, 2022
@vchuravy
Copy link
Member

We should backport the fix to the LTS #40492 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.6 Change should be backported to release-1.6 external dependencies Involves LLVM, OpenBLAS, or other linked libraries linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenBLAS 0.3.10 fails to compile from source with Xcode 12
8 participants