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

Add a test to avoid #53054 regressions #53079

Merged
merged 13 commits into from
Jan 30, 2024
Merged

Conversation

pablosanjose
Copy link
Contributor

Tests issue JuliaLang/LinearAlgebra.jl#1051. The same test is included in backport #53074.

@KristofferC
Copy link
Member

KristofferC commented Jan 27, 2024

Should this ideally be a test in openblas? It somehow feels excessive if we should test bugs of dependencies here.

@pablosanjose
Copy link
Contributor Author

I also thought so initially, but seeing there was just a lone test in there I thought it might be reserved for other things. But yeah, let me move it.

@pablosanjose
Copy link
Contributor Author

So, @giordano argues here that "I don't think this test should be here, this is meant to test only very basic functionalities of the jll, like opening the library and such. If we ever switch to a different default blas backend, the test would still be useful, it isn't specific to openblas."
After thinking it over, I agree with him. This test may seem a bit trivial on its own, but it is definitely not tied to OpenBLAS, it is a general test of deterministic behavior. So, absent an agreement between you two guys in a different direction, I'll revert the last commit in both PRs

@giordano giordano merged commit 3f468cd into JuliaLang:master Jan 30, 2024
7 checks passed
@giordano
Copy link
Contributor

@pablosanjose I think the master branch in your fork isn't in-sync with master of this repository, so the history of your PRs is a bit messed up and always includes unrelated commit messages.

@pablosanjose
Copy link
Contributor Author

Sorry about that, I'll try to fix it

@KristofferC
Copy link
Member

This test may seem a bit trivial on its own, but it is definitely not tied to OpenBLAS, it is a general test of deterministic behavior.

Eh, I mean, it's not like we run every test a million times like this does to test "deterministic behavior". With "should this ideally be a test in openblas" I meant that they shouldn't be in this repo at all but be tested upstream in openblas itself to prevent the regression at the earliest stage possible.

@pablosanjose
Copy link
Contributor Author

Oh, I fully appreciate the point of Kirstoffer. When it doesn't fail, this test takes a full 0.15s on my machine. Maybe it is too excessive? The problem is that a lower number of iterations made it fail too infrequently to be useful. And of course his other argument that this would be more useful upstream is also totally clear. What do you think @giordano?

If you guys agree this is too much overhead for a test that is likely never going to fail again, please do use your authority to revert this PR.

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.

3 participants