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

math/rand: deprecate Seed #56319

Closed
rsc opened this issue Oct 19, 2022 · 20 comments
Closed

math/rand: deprecate Seed #56319

rsc opened this issue Oct 19, 2022 · 20 comments

Comments

@rsc
Copy link
Contributor

rsc commented Oct 19, 2022

Proposal #54880, accepted last week, seeds math/rand randomly at startup. Part of the rationale is that programs that expect a specific determinstic sequence from the global rand functions (like rand.Int) are inherently fragile, since any package can change the number of times it calls those functions, which changes the results from all future calls by other packages. So programs should not depend on any kind of determinism from them.

For the same reason, I suggest we deprecate math/rand.Seed (the global seeder). Programs that need a specific deterministic sequence can do that more reliably using NewRand(NewSource(seed)), which they can be sure no other packages are consuming values from.

@rsc rsc added the Proposal label Oct 19, 2022
@rsc rsc added this to the Proposal milestone Oct 19, 2022
@chrisguiney
Copy link
Contributor

What are the implications of deprecation? Does it simply do nothing? Is it just marked as deprecated so that IDEs/linters can usher users to the suggested usage?

@FiloSottile
Copy link
Contributor

What are the implications of deprecation? Does it simply do nothing? Is it just marked as deprecated so that IDEs/linters can usher users to the suggested usage?

The latter. The function continues to work like before, but users are warned about it in linting, IDEs, and docs.

@rsc
Copy link
Contributor Author

rsc commented Oct 20, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@magical
Copy link
Contributor

magical commented Oct 20, 2022

I disagree with #54880. But i agree that, with that proposal accepted, deprecating the global Seed function is a logical step.

@btracey
Copy link
Contributor

btracey commented Oct 20, 2022

The latter. The function continues to work like before, but users are warned about it in linting, IDEs, and docs.

So it will work into the future, just users will be warned about it? Or it will work in the near-term future, but no promises in the long run?

programs that expect a specific determinstic sequence from the global rand functions (like rand.Int) are inherently fragile

In scientific simulation, there are lots of cases where it is very useful to be able to produce a deterministic result, especially for debugging purposes. Right now, this is easy to do in Go. The author writes code that calls the math/rand functions, and can get reproducible behavior by setting the seed. This is incredibly useful for debugging code, for example to reliably follow a specific codepath or to check a computation by hand from some known results. It's true that this is inherently fragile, but that's not important, because during debugging I'm not changing all of that code. For working code, I do want the result to be random (and thus subject to that fragility), but for non-working code it's very useful to easily control the randomness.

It is true that an alternative is to call rand.NewSource, but this is invasive. This call has to happen near the top of main, and then all subsequent functions have to pass around the *rand.Source. In Gonum we allow this behavior because it's intended to be a base library, but for a lot of my exploratory code I just use the default generator. If Seed no longer works to generate a deterministic stream, then this is effectively also deprecating rand.Float64() and friends for my code (as again, the deterministic behavior is really important for debugging).

I appreciate the story is different in Go than in other languages because concurrency is so prevalent and this interacts poorly with determinism from a global generator. That said, I've had a lot of success with code that generates the necessary randomness, and then does deterministic concurrent computation given those results.

@chrisguiney
Copy link
Contributor

If Seed no longer works to generate a deterministic stream, then this is effectively also deprecating rand.Float64() and friends for my code (as again, the deterministic behavior is really important for debugging).

I think the implication here is that if you need deterministic behavior, then you need to create a new RNG with rand.New()

It's not really possible to ensure deterministic behavior to the global RNG, because there may be a goroutine started by a library that re-seeds it.

I'm not sure if the go team ensures that the RNG will produce the same series for the same seed between go versions, so even if you ensure that there is no competing seed for the global RNG, the series it produces may change in the future.

If you need a truly stable deterministic series that will hold up to changes, it should be generated, stored in the project, and read at runtime.

That all said, while I believe the intention is that the package will continue behaving as it has, and there's just more emphasis on using it correctly. I don't know what the long term plan for the package is though, so please take my words lightly.

@Merovius
Copy link
Contributor

Personally, the main reason I want to deprecate Seed is so that I can run a static analyzer for anything in my transitive dependency graph calling it and filing bugs that they shouldn't do that anymore. Having the function deprecated is a great argument for that.

It's not really possible to ensure deterministic behavior to the global RNG, because there may be a goroutine started by a library that re-seeds it.

It doesn't even have to be a goroutine. It can just be a package you import which uses init or a package you import which uses one of the global math/rand functions.

It is true that an alternative is to call rand.NewSource, but this is invasive. This call has to happen near the top of main, and then all subsequent functions have to pass around the *rand.Source.

Personally, as I have the opposite problem, I tend to have an internal/rnd package which has a global locked source which is properly seeded and can't be reseeded. Apart from that it exports the same package-scoped functions that math/rand does. Whenever I want to get "just a random integer", I do rnd.Intn(n), instead of rand.Intn(n) and can rely on having a properly non-deterministic integer.

You can do the same concept, but the other way around: Make an internal/rnd package with a global locked source which is deterministically seeded to 1, can't be re-seeded and which exports the same package-scoped functions math/rand does. Then replace math/rand with internal/rnd in any package you control and want to behave deterministically.

This, of course, only works if all the relevant package are controllled by you, so rogue RNG usage doesn't change your output. But that's already the situation you're in.

So, IMO 1. it's not actually invasive, really, if you don't want it to be. And 2. you should probably do the invasive thing anyway, as that's just the right thing to do.

@btracey
Copy link
Contributor

btracey commented Oct 20, 2022

Whenever I want to get "just a random integer", I do rnd.Intn(n), instead of rand.Intn(n) and can rely on having a properly non-deterministic integer.

I think that's a reasonable solution. I do want to say though that this workaround/solution is still implicitly deprecating rand.Intn and friends.

@Merovius
Copy link
Contributor

Merovius commented Oct 20, 2022

is still implicitly deprecating rand.Intn and friends.

I strongly disagree. On the contrary. It makes them, for most usecases - those which actually want non-deterministic random numbers - usable, as opposed to the current situation, where they are pointless for all usecases.. Currently you can't rely on them to be non-deterministic, because some library might've called rand.Seed with a bad seed. You can't rely on them being deterministic for the same reason.

If finally no one calls rand.Seed anymore, you can finally rely on them being actually random.

@rsc
Copy link
Contributor Author

rsc commented Oct 26, 2022

So it will work into the future, just users will be warned about it? Or it will work in the near-term future, but no promises in the long run?

In Go, deprecated means only that you should probably not use it anymore. We won't remove it, because compatibility.

@rsc
Copy link
Contributor Author

rsc commented Oct 26, 2022

In scientific simulation, there are lots of cases where it is very useful to be able to produce a deterministic result, especially for debugging purposes. Right now, this is easy to do in Go. ...

It will remain easy to do. IDEs will just flag that rand.Seed use. Nothing will actually stop you from using it though.

@FiloSottile
Copy link
Contributor

It is true that an alternative is to call rand.NewSource, but this is invasive. This call has to happen near the top of main, and then all subsequent functions have to pass around the *rand.Source. In Gonum we allow this behavior because it's intended to be a base library, but for a lot of my exploratory code I just use the default generator. If Seed no longer works to generate a deterministic stream, then this is effectively also deprecating rand.Float64() and friends for my code (as again, the deterministic behavior is really important for debugging).

@btracey You can also put var rand = rand.New(rand.NewSource(99)) in your package's global scope, and use it as the global RNG. It will work approximately the same with the advantage of not being fragile. It won't work across packages, but if you have a design that spans multiple packages you probably should be passing around the *Rand, as you pointed out.

@rsc
Copy link
Contributor Author

rsc commented Oct 26, 2022

The 'var rand = ' trick is also not safe for concurrent use, but if you think you're reproducing an answer, you probably don't have concurrency anyway.

@rsc
Copy link
Contributor Author

rsc commented Nov 2, 2022

There were some concerns about existing uses being flagged, but they will keep compiling, and they can be made more robust and avoid the flag by using var rand = rand.New(rand.NewSource(99)) as @FiloSottile pointed out.

Are there any remaining objections to accepting this proposal?

@rsc
Copy link
Contributor Author

rsc commented Nov 9, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor Author

rsc commented Nov 16, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: math/rand: deprecate Seed math/rand: deprecate Seed Nov 16, 2022
@rsc rsc modified the milestones: Proposal, Backlog Nov 16, 2022
@gopherbot
Copy link

Change https://go.dev/cl/451375 mentions this issue: math/rand: deprecate Seed

@vcschapp
Copy link

Does the proposal cover what to do with rand.Shuffle?

It's always a bit frustrating when one part of the standard library specifically documents reliance on a feature marked deprecated by the other part.


@ianlancetaylor
Copy link
Contributor

rand.Shuffle works as before.

If you want a repeatable shuffle, use New(NewSource(seed)) with your desired seed value to get a math.Rand value, and call its Shuffle method.

@vcschapp
Copy link

Thanks @ianlancetaylor. In retrospect I should have RTFM'd harder! I do appreciate your advice.

ajm188 added a commit to planetscale/vitess that referenced this issue Aug 30, 2023
…stant

See golang/go#56319

Signed-off-by: Andrew Mason <andrew@planetscale.com>
aruneshpa added a commit to aruneshpa/vm-operator that referenced this issue Sep 25, 2023
controller-runtime v15 has a min Golang version requirement that
breaks the container builds. This change bumps up the Go version used
to build the container. Starting 1.21 Golang, go.mod requires the Go
version and treats that as min required go version to compile your
binary. So, also bump up the Go version for local builds.

Additional changes:

- As per golang/go#56319
and golang/go#54880, global rand is
automatically, randomly seeded. So we don't need to call `Seed`
anymore.

- Package ioutil has been deprecated. Port the code to use replacement
methods.
aruneshpa added a commit to vmware-tanzu/vm-operator that referenced this issue Sep 25, 2023
controller-runtime v15 has a min Golang version requirement that
breaks the container builds. This change bumps up the Go version used
to build the container. Starting 1.21 Golang, go.mod requires the Go
version and treats that as min required go version to compile your
binary. So, also bump up the Go version for local builds.

Additional changes:

- As per golang/go#56319
and golang/go#54880, global rand is
automatically, randomly seeded. So we don't need to call `Seed`
anymore.

- Package ioutil has been deprecated. Port the code to use replacement
methods.
georgezgeorgez added a commit to zenon-network/go-zenon that referenced this issue Sep 28, 2023
Determinstic random generator should not be set by rand.Seed
https://pkg.go.dev/math/rand#Seed
golang/go#56319
georgezgeorgez added a commit to hypercore-one/go-zenon that referenced this issue Sep 28, 2023
Determinstic random generator should not be set by rand.Seed
https://pkg.go.dev/math/rand#Seed
golang/go#56319
georgezgeorgez added a commit to zenon-network/go-zenon that referenced this issue Sep 28, 2023
georgezgeorgez added a commit to hypercore-one/go-zenon that referenced this issue Sep 28, 2023
georgezgeorgez added a commit to hypercore-one/go-zenon that referenced this issue Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

10 participants