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

Test: Child tasks have identical RNG streams after a testset in 1.10.0-beta2 #51255

Closed
nhz2 opened this issue Sep 9, 2023 · 2 comments · Fixed by #51332
Closed

Test: Child tasks have identical RNG streams after a testset in 1.10.0-beta2 #51255

nhz2 opened this issue Sep 9, 2023 · 2 comments · Fixed by #51332
Labels
randomness Random number generation and the Random stdlib regression Regression in behavior compared to a previous version testsystem The unit testing framework and Test stdlib

Comments

@nhz2
Copy link
Contributor

nhz2 commented Sep 9, 2023

          So that might be an issue with `Test.guardseed` now then. That `copy` seems to be supposed to work.

Originally posted by @vtjnash in #51225 (comment)

Here is a MWE that works in 1.9 but fails in 1.10.

using Test

@testset "top test" begin
    random_nums = Float64[]
    for i in 1:5
        @testset "inner test" begin
            @test true
        end
        push!(random_nums, fetch(@async(rand())))
    end
    @test !allequal(random_nums)
end
@brenhinkeller brenhinkeller added testsystem The unit testing framework and Test stdlib randomness Random number generation and the Random stdlib labels Sep 9, 2023
@Keno Keno added the regression Regression in behavior compared to a previous version label Sep 9, 2023
@rfourquet
Copy link
Member

This is solved by something like:

@eval Random begin
struct TaskLocalCopy <: AbstractRNG
    xo::Xoshiro
    s4::UInt64
end

function _copy(::TaskLocalRNG)
    t = current_task()
    TaskLocalCopy(Xoshiro(t.rngState0, t.rngState1, t.rngState2, t.rngState3), t.rngState4)
end

function copy!(dst::TaskLocalRNG, src::TaskLocalCopy)
    t = current_task()
    xo = src.xo
    setstate!(dst, xo.s0, xo.s1, xo.s2, xo.s3, src.s4)
    return dst
end

end

And then in Test use Random._copy instead of copy in the implementation of @testset.

The question is: should this be what copy does directly on TaskLocalRNG (instead of calling it _copy), in which case should we bother to implement rand(::TaskLocalCopy) (this feels overkill...) ? It looks like this would address the concerns of #51225.

@nhz2
Copy link
Contributor Author

nhz2 commented Sep 11, 2023

I found an example https://github.com/RelationalAI-oss/XUnit.jl/blob/1f422fa22c8f3f570691cd5737990c250f8074b1/test/base-tests.jl#L718-L736 that depends on rand(::TastLocalCopy) being defined, so maybe that is a required method for the thing returned by copy

vtjnash pushed a commit that referenced this issue Sep 15, 2023
This PR adds an optional field to the existing `Xoshiro` struct to be
able to faithfully copy the task-local RNG state.

Fixes #51255
Redo of #51271

Background context: #49110 added an additional state to the task-local
RNG. However, before this PR `copy(default_rng())` did not include this
extra state, causing subtle errors in `Test` where `copy(default_rng())`
is assumed to contain the full task-local RNG state.
NHDaly pushed a commit that referenced this issue Sep 20, 2023
This PR adds an optional field to the existing `Xoshiro` struct to be
able to faithfully copy the task-local RNG state.

Fixes #51255
Redo of #51271

Background context: #49110 added an additional state to the task-local
RNG. However, before this PR `copy(default_rng())` did not include this
extra state, causing subtle errors in `Test` where `copy(default_rng())`
is assumed to contain the full task-local RNG state.
KristofferC pushed a commit that referenced this issue Oct 3, 2023
This PR adds an optional field to the existing `Xoshiro` struct to be
able to faithfully copy the task-local RNG state.

Fixes #51255
Redo of #51271

Background context: #49110 added an additional state to the task-local
RNG. However, before this PR `copy(default_rng())` did not include this
extra state, causing subtle errors in `Test` where `copy(default_rng())`
is assumed to contain the full task-local RNG state.

(cherry picked from commit 41b41ab)
nalimilan pushed a commit that referenced this issue Nov 5, 2023
This PR adds an optional field to the existing `Xoshiro` struct to be
able to faithfully copy the task-local RNG state.

Fixes #51255
Redo of #51271

Background context: #49110 added an additional state to the task-local
RNG. However, before this PR `copy(default_rng())` did not include this
extra state, causing subtle errors in `Test` where `copy(default_rng())`
is assumed to contain the full task-local RNG state.

(cherry picked from commit 41b41ab)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
randomness Random number generation and the Random stdlib regression Regression in behavior compared to a previous version testsystem The unit testing framework and Test stdlib
Projects
None yet
4 participants