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

apply OpenBLAS_jll v0.3.23+4 patch #53074

Merged

Conversation

pablosanjose
Copy link
Contributor

@pablosanjose pablosanjose changed the title Backport OpenBLAS_jll v0.3.23+3 patch to 1.10 apply OpenBLAS_jll v0.3.23+4 patch Jan 26, 2024
@pablosanjose
Copy link
Contributor Author

Two questions:
(1) is CI working correctly? If so, I cannot make heads or tails of the test failures
(2) should we add the MWE of JuliaLang/LinearAlgebra.jl#1051 as a test?

@giordano
Copy link
Contributor

giordano commented Jan 26, 2024

First of all, have you verified the bug is indeed gone on this branch? Regarding a regression test, that'd be a good idea, but a test only on this branch would be quite useless as it'd be lost forever in future releases. You want to submit a separate PR for master, perhaps to be backported to 1.10 (but the backport doesn't seem massively useful).

Copy link
Member

@ViralBShah ViralBShah left a comment

Choose a reason for hiding this comment

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

On 1.10 branch, we also need to add the patch for a source based build. So the patch should also get added to deps/patches, and it should be applied in deps/openblas.mk.

@ViralBShah ViralBShah added the backport 1.10 Change should be backported to the 1.10 release label Jan 26, 2024
@ViralBShah ViralBShah added this to the 1.10 milestone Jan 26, 2024
@ViralBShah
Copy link
Member

@imciner2
Copy link
Contributor

(2) should we add the MWE of JuliaLang/LinearAlgebra.jl#1051 as a test?

I'd say the regression test should at a minimum go into master (we added one for the other OpenBLAS patch we are using for M1 just recently in #53059). That should be a separate PR though.

@pablosanjose
Copy link
Contributor Author

First of all, have you verified the bug is indeed gone on this branch?

I can confirm that for me this branch builds from source using BinaryBuilder, and the result is free of the bug.

@pablosanjose
Copy link
Contributor Author

Regarding the test failures, at least some of them are also appearing in buildkite runs of other branches, such as avi/53062, so they should be unrelated to the patch.

Regarding a regression test, that'd be a good idea, but a test only on this branch would be quite useless as it'd be lost forever in future releases. You want to submit a separate PR for master, perhaps to be backported to 1.10 (but the backport doesn't seem massively useful).

I'd say the regression test should at a minimum go into master (we added one for the other OpenBLAS patch we are using for M1 just recently in #53059). That should be a separate PR though.

I'll add a test here and submit it also to master as a separate PR.

@pablosanjose
Copy link
Contributor Author

pablosanjose commented Jan 27, 2024

There is a potential issue I'm trying to understand: this patch is for OpenBLAS (JuliaRegistries/General#99646) which is indeed v0.3.23+4, but the OpenBLAS32 version (JuliaRegistries/General#99647) is v0.3.23+3. Is this a problem?

@giordano
Copy link
Contributor

Is this a problem?

No.

@KristofferC
Copy link
Member

So this should be merged in addition to 3c73873?

@imciner2
Copy link
Contributor

So this should be merged in addition to 3c73873?

Yes, because this updates the local build for OpenBLAS in case people are not using the BinaryBuilder version.

@KristofferC
Copy link
Member

Test error is for another change on the backport branch.

@KristofferC KristofferC merged commit 3f5f846 into JuliaLang:backports-release-1.10 Jan 29, 2024
4 of 6 checks passed
@pablosanjose pablosanjose deleted the openblas-patch branch January 29, 2024 14:58
giordano pushed a commit that referenced this pull request Jan 30, 2024
Tests issue #53054. The same test is included in backport #53074.
KristofferC pushed a commit that referenced this pull request Jan 30, 2024
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label Feb 6, 2024
Drvi pushed a commit to RelationalAI/julia that referenced this pull request Jun 7, 2024
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.

Non-deterministic mul! of ComplexF32 arrays under Apple Silicon
5 participants