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.

Sign up for GitHub

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

Jump to bottom

@testset: with Xoshiro, restore Random.GLOBAL_SEED #43188

Merged
merged 1 commit into from
Nov 23, 2021
Merged

@testset: with Xoshiro, restore Random.GLOBAL_SEED #43188

merged 1 commit into from
Nov 23, 2021

Conversation

rfourquet
Copy link
Member

A @testset is supposed to restore the "global RNG state" as
it was before execution (so that they can be re-ordered easily, etc.)
Also, before a testset starts, the default RNG is re-seeded with the
"global seed" (to help reproduce test failures).

Before Xoshiro as the default RNG, the "global seed" was stored
within a MersenneTwister object. It was enough for a testset to
copy the default RNG at the start, and copy it back at the end.
But now the global seed is stored outside of the RNG, so it should
also be restored separately.

@rfourquet rfourquet added backport 1.7 kind:bugfix This change fixes an existing bug testsystem The unit testing framework and Test stdlib labels Nov 22, 2021
@Seelengrab
Copy link
Contributor

Does this possibly fix #42752?

@rfourquet
Copy link
Member Author

Does this possibly fix #42752?

Probably not, but who knows?... I noticed the bug fixed by this PR while looking at that issue, but I don't understand the cause of it.

@rfourquet
@testset: with Xoshiro, restore Random.GLOBAL_SEED
e53b1b9 
A `@testset` is supposed to restore the "global RNG state" as
it was before execution (so that they can be re-ordered easily, etc.)
Also, before a testset starts, the default RNG is re-seeded with the
"global seed" (to help reproduce test failures).

Before `Xoshiro` as the default RNG, the "global seed" was stored
within a `MersenneTwister` object. It was enough for a testset to
copy the default RNG at the start, and copy it back at the end.
But now the global seed is stored outside of the RNG, so it should
also be restored separately.
jonas-schulze
jonas-schulze reviewed Nov 22, 2021
View reviewed changes
stdlib/Test/test/runtests.jl
@test GLOBAL_SEED_orig == Random.GLOBAL_SEED
@testset "GLOBAL_SEED is also preserved (beginend)" begin
@test refvalue == rand(Int)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
end
end
@test GLOBAL_SEED_orig == Random.GLOBAL_SEED

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, yes this could be added, but this is also tested after the first testset of the series, which is also "beginend" (actually, this last testset really is testing that the forloop one correctly restored the global seed by checking rand(Int) within it, but it's a bit redundant with @test GLOBAL_SEED_orig == Random.GLOBAL_SEED just before it; but as we don't call seed! there, there is not much need to recheck this equality after it).

@vtjnash
Copy link
Sponsor Member

vtjnash commented Nov 22, 2021

Would it be better if we called seed! here instead? (possibly without the copy, or do the copy next)

@rfourquet
Copy link
Member Author

Would it be better if we called seed! here instead? (possibly without the copy, or do the copy next)

It would work too, but we would indeed need to still make the copy to restore the global rng, which doesn't necessarily correspond to the pristine state after seed!(GLOBAL_SEED). I prefer set_global_seed! as it's no less cheap than needed, compared to seed!, and more to the point. To avoid introducing this function, making GLOBAL_SEED a Ref{Any} might work, but I'm not sure whether this would be safe w.r.t. multithreading.

@rfourquet rfourquet added the domain:randomness Random number generation and the Random stdlib label Nov 23, 2021
@vtjnash vtjnash merged commit 08ea2d8 into master Nov 23, 2021
@vtjnash vtjnash deleted the rf/testset-GLOBAL_SEED branch November 23, 2021 15:41
@vtjnash
Copy link
Sponsor Member

vtjnash commented Nov 23, 2021

Does this bug need to be fixed inside Test.guardseed too?

@rfourquet
Copy link
Member Author

No, guardseed is dead code, I've been meaning to remove it. But it's interesting that the "bug" in #42752 was found thanks to guardseed's tests.

@KristofferC KristofferC mentioned this pull request Nov 25, 2021
Merged
29 tasks
KristofferC pushed a commit that referenced this pull request Nov 25, 2021
@rfourquet @KristofferC
@testset: with Xoshiro, restore Random.GLOBAL_SEED (#43188)
e1c7cd4 
A `@testset` is supposed to restore the "global RNG state" as
it was before execution (so that they can be re-ordered easily, etc.)
Also, before a testset starts, the default RNG is re-seeded with the
"global seed" (to help reproduce test failures).

Before `Xoshiro` as the default RNG, the "global seed" was stored
within a `MersenneTwister` object. It was enough for a testset to
copy the default RNG at the start, and copy it back at the end.
But now the global seed is stored outside of the RNG, so it should
also be restored separately.

(cherry picked from commit 08ea2d8)
KristofferC pushed a commit that referenced this pull request Nov 26, 2021
@rfourquet @KristofferC
@testset: with Xoshiro, restore Random.GLOBAL_SEED (#43188)
5a99507 
A `@testset` is supposed to restore the "global RNG state" as
it was before execution (so that they can be re-ordered easily, etc.)
Also, before a testset starts, the default RNG is re-seeded with the
"global seed" (to help reproduce test failures).

Before `Xoshiro` as the default RNG, the "global seed" was stored
within a `MersenneTwister` object. It was enough for a testset to
copy the default RNG at the start, and copy it back at the end.
But now the global seed is stored outside of the RNG, so it should
also be restored separately.

(cherry picked from commit 08ea2d8)
KristofferC pushed a commit that referenced this pull request Nov 26, 2021
@rfourquet @KristofferC
@testset: with Xoshiro, restore Random.GLOBAL_SEED (#43188)
bf8d60b 
A `@testset` is supposed to restore the "global RNG state" as
it was before execution (so that they can be re-ordered easily, etc.)
Also, before a testset starts, the default RNG is re-seeded with the
"global seed" (to help reproduce test failures).

Before `Xoshiro` as the default RNG, the "global seed" was stored
within a `MersenneTwister` object. It was enough for a testset to
copy the default RNG at the start, and copy it back at the end.
But now the global seed is stored outside of the RNG, so it should
also be restored separately.

(cherry picked from commit 08ea2d8)
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
@rfourquet @LilithHafner
@testset: with Xoshiro, restore Random.GLOBAL_SEED (JuliaLang#43188)
cd6c3a0 
A `@testset` is supposed to restore the "global RNG state" as
it was before execution (so that they can be re-ordered easily, etc.)
Also, before a testset starts, the default RNG is re-seeded with the
"global seed" (to help reproduce test failures).

Before `Xoshiro` as the default RNG, the "global seed" was stored
within a `MersenneTwister` object. It was enough for a testset to
copy the default RNG at the start, and copy it back at the end.
But now the global seed is stored outside of the RNG, so it should
also be restored separately.
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
@rfourquet @LilithHafner
@testset: with Xoshiro, restore Random.GLOBAL_SEED (JuliaLang#43188)
b4f7da0 
A `@testset` is supposed to restore the "global RNG state" as
it was before execution (so that they can be re-ordered easily, etc.)
Also, before a testset starts, the default RNG is re-seeded with the
"global seed" (to help reproduce test failures).

Before `Xoshiro` as the default RNG, the "global seed" was stored
within a `MersenneTwister` object. It was enough for a testset to
copy the default RNG at the start, and copy it back at the end.
But now the global seed is stored outside of the RNG, so it should
also be restored separately.
vchuravy pushed a commit to JuliaLang/Test.jl that referenced this pull request Oct 7, 2023
@rfourquet @KristofferC
@testset: with Xoshiro, restore Random.GLOBAL_SEED (JuliaLang/julia…
a1f51b3 
…#43188)

A `@testset` is supposed to restore the "global RNG state" as
it was before execution (so that they can be re-ordered easily, etc.)
Also, before a testset starts, the default RNG is re-seeded with the
"global seed" (to help reproduce test failures).

Before `Xoshiro` as the default RNG, the "global seed" was stored
within a `MersenneTwister` object. It was enough for a testset to
copy the default RNG at the start, and copy it back at the end.
But now the global seed is stored outside of the RNG, so it should
also be restored separately.

(cherry picked from commit 6b48dc1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonas-schulze jonas-schulze jonas-schulze left review comments

@vtjnash vtjnash vtjnash approved these changes

Assignees
No one assigned
Labels
domain:randomness Random number generation and the Random stdlib kind:bugfix This change fixes an existing bug testsystem The unit testing framework and Test stdlib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rfourquet @Seelengrab @vtjnash @jonas-schulze @KristofferC