move Random to stdlib #24874

rfourquet · 2017-12-01T12:49:31Z

This is just an experiment at this point, and I don't know if we want this.
One motivation is that almost no code in base depends on Random, so could be stripped out if needed by some installations, another motivation is if this would allow realeasing new versions of Random faster than Base (which would allow faster release of improvements which change the generated random streams (which is "breaking")). I'm happy to discuss here alternative plans for those breaking changes, like have MersenneTwister without reprocucibility guarranty, and having MersenneTwister1 frozen to its state as of Julia version 1.0.

I had to leave some functions (e.g. rand) and AbstractRNG in Base because sprand needs it (I may move sprand to Random eventually, if sparse code stays in base), but also I think it's nice to have at least rand, rand! and maybe srand available from Base directly without importing Random.

TODOS:

decide which functions stay in Base
update docs accordingly
update test accordingly, and "choosetests.jl"
fix the use of randstring in "distributed/" once it's moved to "stdlib" (Move Distributed to stdlib #24443)
maybe use precompile like other bits moved to stdlib ?

ViralBShah · 2017-12-01T17:26:42Z

Overall, I think this is the right direction.

ViralBShah · 2017-12-12T20:59:10Z

Bump. would be great to get this done. I think we should do only the excision from Base into stdlib for now - and once done, figure out the fate of MersenneTwister separately.

rfourquet · 2017-12-12T21:39:46Z

Sure; but this more or less depends on "distributed" being moved to stdlib first, as it uses randstring. If @amitmurthy doesn't come to it, I will see how I can work around.

ViralBShah · 2017-12-12T22:47:55Z

We could just patch distributed for now to call randstring with the new incantation. I think there is a bit more work there to be done before it can be moved.

rfourquet · 2017-12-13T09:38:37Z

We could just patch distributed for now to call randstring with the new incantation.

But randstring is called in init_parallel itself in the sysimg.jl __init__, at which time I think stdlib modules are not loaded yet?

StefanKarpinski · 2017-12-13T16:30:53Z

Presumably, distributed just needs an unpredictable path name that won't collide and doesn't need to hook into our "official" random infrastructure. So maybe we could decouple it by just calling C rand directly, e.g.:

function randslug(n::Int=8)
    digits = ['0':'9'; 'a':'z'; 'A':'Z']
    s = sprint() do io
        while io.size < n
            r = ccall(:rand, Cuint, ())
            write(io, digits[(r % length(digits)) + 1])
        end
    end
    return s
end

It's not very efficient, but it's just being used for random path names.

rfourquet · 2017-12-13T20:09:29Z

So maybe we could decouple it by just calling C rand directly, e.g.:

Yes I had had the same thought, but got lazy as soon as trying to remember how we do things in C 😁 thanks for having done the work, it helps a lot!

rfourquet · 2017-12-14T17:40:35Z

So this should be ready. I kept a referencence in "base/sparse/linalg.jl" to the Base._rand_pm1! function, which is implemented in Random, with the idea that sparse will be moved in stdlib too eventually, so it's OK to depend on Random. On the other hand, a couple of functions in Base have switched to using C's rand function, in "error.jl" and mktempdir, as it seems that a bad RNG is enough for those.
All random related functions are moved to the stdlib, plus sprand and sprandn which were not in Base.Random. So even for rand(), you get to using Random.
If this is fine, I think this is good to go (will review one more time now).

rfourquet · 2017-12-15T09:18:57Z

As I said, the current state of the PR is to not export by default any rand function when starting the REPL, and I don't even know how to do that. But as this is a merge-conflict magnet, I will merge shortly after CI turns green, and it will be easy to adjust to a different decision if one is taken.

rfourquet · 2017-12-16T11:35:46Z

Help! After many attemps, I'm still unable to find what the error is in Appveyor, while it builds the doc. There is no information shown, and I don't have Windows, so I'm really helpless :( It may be a stupid error, but hard to say when not able to debug...

rfourquet · 2018-01-08T13:30:46Z

Thanks for checking the box! Updated accordingly. Unless requested to change something, I would like to merge by tomorrow if CI turns green. The details can easily be adjusted later, but this tends to accumulate merge conflicts rapidly.

We do have the option of putting using Random in the default juliarc

That would be my preference too, or rather using Random: rand, randn.

fredrikekre · 2018-01-08T13:58:50Z

stdlib/Random/test/OAs.jl

@@ -0,0 +1,124 @@
+# This file is a part of Julia. License is MIT: https://julialang.org/license
+
+# copied verbatim from julia/test/TestHelpers.jl


Why not just use what is in test/TestHelpers.jl? Seems annoying to maintain two exact copies...

Because I couldn't find the correct path to put in the include invocation. My short term plan was to move OffsetArrays from test/TestHelpers.jl to stdlib/Test as I think it's generally useful beyond Base, so it wouldn't be annoying to maintain two copies for a long time.

Perhaps

isdefined(Main, :TestHelpers) || @eval Main include(joinpath(@__DIR__, "../../../test/TestHelpers.jl")) using Main.TestHelpers.OAs

Great, this worked! thanks

In the end it didn't work, I had forgotten that the problem arises with on CI (with some invocation of the tests), not with julia stdlib/Random/test/runtests.jl. I will just do with the duplication for now.

This worked for the REPL to stdlib: https://github.com/JuliaLang/julia/pull/25544/files#diff-7b80498051b14c15cd26dd98fcc8e5acR12

I had noticed your solution earlier today, but I don't see how it is different from what Fredrik proposed above ?

fredrikekre · 2018-01-08T14:00:36Z

base/abstractarray.jl

@@ -361,6 +361,8 @@ See also [`checkindex`](@ref).

 # Examples
 ```jldoctest
+julia> using Random


This (and most other doc examples) don't actually need to use rand so we should replace those calls with something like fill instead since random just poisons the example (and even more now with a required using Random.

Thanks for your review! Actually with the new change (rand is in Base), using Random isn't necessary anymore.

fredrikekre · 2018-01-08T14:01:29Z

base/docs/basedocs.jl

@@ -988,6 +988,8 @@ callable with no arguments). The task exits when this function returns.

 # Examples
 ```jldoctest
+julia> using Random


rand -> fill

fredrikekre · 2018-01-08T14:01:54Z

base/essentials.jl

@@ -575,6 +575,8 @@ if the index is out of bounds, or has an undefined reference.

 # Examples
 ```jldoctest
+julia> using Random


rand -> fill

fredrikekre · 2018-01-08T14:02:20Z

base/event.jl

@@ -118,6 +118,8 @@ If a second argument `val` is provided, it will be passed to the task (via the r
 the woken task.

 ```jldoctest
+julia> using Random


rand -> fill

fredrikekre · 2018-01-08T14:04:29Z

base/permuteddimsarray.jl

@@ -28,6 +28,8 @@ See also: [`permutedims`](@ref).

 # Examples
 ```jldoctest
+julia> using Random


rand -> fill

fredrikekre · 2018-01-08T14:05:18Z

base/task.jl

@@ -61,6 +61,8 @@ Wrap an expression in a [`Task`](@ref) without executing it, and return the [`Ta
 creates a task, and does not run it.

 ```jldoctest
+julia> using Random


rand -> fill

fredrikekre · 2018-01-08T14:05:46Z

doc/src/stdlib/arrays.md

@@ -164,16 +164,16 @@ Base.mapslices
 ## Combinatorics

 ```@docs
-Base.Random.randperm
-Base.Random.randperm!
+Random.randperm


Shouldn't these move to the random docs instead?

I don't know. As long as Random is bundled with Base, it may be useful to have that here as before?

I ended up following your suggestion :)

fredrikekre · 2018-01-08T14:06:00Z

doc/src/stdlib/strings.md

@@ -54,7 +54,7 @@ Base.chomp
 Base.thisind
 Base.nextind
 Base.prevind
-Base.Random.randstring
+Random.randstring


To random docs?

fredrikekre · 2018-01-08T14:07:09Z

stdlib/Random/src/Random.jl

@@ -359,4 +368,16 @@ true
 """
 srand(rng::AbstractRNG, ::Nothing) = srand(rng)

+
+## deprecations


The other stdlibs have an deprecated.jl file that they include.

KristofferC · 2018-01-08T14:09:11Z

I think putting random stuff (pun intended) into .juliarc.jl is more likely to be confusing than helpful.

fredrikekre · 2018-01-08T14:11:05Z

Why is sprand is moved. I find it weird that Random essentially has a dependency on SparseArrays. Isn't it more natural the other way around, that SparseArrays implements sprand with Random as a dependency?

rfourquet · 2018-01-08T14:57:25Z

Why is sprand is moved.

I agree it would better together with the sparse functionality, but it's in Base for the moment, so not really possible (without complications)... or do you mean putting sprand and friends in stdlib/SuiteSparse ? (I know very little about the sparse stuff).

fredrikekre · 2018-01-08T14:59:45Z

So we should just move it back to SparseArrays after #25249 then?

ViralBShah · 2018-01-09T05:45:15Z

Yes, we should move sprand into the sparse matrix implementation.

rfourquet · 2018-01-09T13:19:15Z

I updated according to @fredrikekre comments, and kept sprand and sprandn "grandfathered" in Base, which can be moved in SparseArrays with #25249 (or the other way around...)

KristofferC · 2018-01-09T14:24:55Z

This needs to be rebased after #24461. I can do it if you are tired of rebasing this :)

rfourquet · 2018-01-11T08:08:24Z

I can do it if you are tired of rebasing this :)

Thanks so much for the offer! It was an easy one this time so I just did it :)

rfourquet · 2018-01-15T01:00:29Z

All green, except what seems to be a timeout on AppVeyor, time to merge has finally come 😌

ViralBShah · 2018-01-15T18:43:59Z

Thank you!

ararslan · 2018-01-15T22:53:42Z

stdlib/Random/src/Random.jl



 ## general definitions

 abstract type AbstractRNG end

+defaultRNG() = GLOBAL_RNG


This shouldn't be camel-cased; it violates the style used everywhere else in Base.

Also if this is just reading from a global const, why not just export the const rather than hiding it behind a function call?

This shouldn't be camel-cased;

I didn't really sea this as CamelCased, more as "initials get capitalized"; what would be prefered here? default_rng ? (defaultrng is un-readable).

why not just export the const rather than hiding it behind a function call

If I remember correctly, it was needed at one point in this PR, because sprand had GLOBAL_RNG in its signature, but was in base and couldn't reference a not-yet created global object. Now that SparseArrays is in stdlib, it should be fine to use GLOBAL_RNG again but didn't think about it. But I have an impression that for our rand API, it would be better to recommend using a function than a global variable, this gives more freedom evolving the API without breaking change (e.g. defaultRNG could give a different object depending on the current thread in the future). See also #23205. But this was not the intent in this current PR to push this change, and now that it's not needed anymore, I should probably revert this bit.

I didn't really sea this as CamelCased, more as "initials get capitalized"

We never capitalize anything in function names, only in type names and consts.

what would be prefered here? default_rng ? (defaultrng is un-readable).

I'd say defaultrng, though I think just using the const is fine; I don't see how changing the return value of the function would be any more or less breaking than changing the value of a const variable.

I don't see how changing the return value of the function would be any more or less breaking than changing the value of a const variable.

It's less breaking in that if defaultRNG() returns a different object depending on the thread (in which case GLOBAL_RNG may be deleted), then it won't be necessary to update rand-related functions (like sprand) to default to defaultRNG() instead of GLOBAL_RNG. But we are not there yet (quite some uncertainty remains regarding the future of GLOBAL_RNG), and needing to access GLOBAL_RNG directly should be quite rare anyway, so I will revert this part.

This function is an artifact of an intermediate state of PR #24874, but is not needed anymore.

rfourquet added kind:excision Removal of code from Base or the repository domain:randomness Random number generation and the Random stdlib stdlib Julia's standard library labels Dec 1, 2017

rfourquet added this to the 1.0 milestone Dec 13, 2017

rfourquet force-pushed the rf/stdlib-random branch from a4a3785 to b6f6cbc Compare December 14, 2017 17:38

rfourquet force-pushed the rf/stdlib-random branch 2 times, most recently from 10eb971 to f42b73d Compare December 15, 2017 08:28

rfourquet changed the title ~~WIP/RFC: move Random to stdlib~~ move Random to stdlib Dec 15, 2017

rfourquet force-pushed the rf/stdlib-random branch 6 times, most recently from 3834399 to aef1c9f Compare December 16, 2017 09:19

rfourquet closed this Dec 16, 2017

rfourquet reopened this Dec 16, 2017

rfourquet force-pushed the rf/stdlib-random branch 3 times, most recently from aef1c9f to 6af87f4 Compare December 16, 2017 11:06

rfourquet force-pushed the rf/stdlib-random branch from 6af87f4 to b00ef11 Compare December 16, 2017 15:34

fredrikekre reviewed Jan 8, 2018

View reviewed changes

rfourquet force-pushed the rf/stdlib-random branch 2 times, most recently from e452c5f to fb2585e Compare January 9, 2018 13:18

rfourquet force-pushed the rf/stdlib-random branch from fb2585e to 960e30b Compare January 9, 2018 13:29

rfourquet force-pushed the rf/stdlib-random branch 2 times, most recently from 132d491 to 62da12c Compare January 11, 2018 08:03

rfourquet force-pushed the rf/stdlib-random branch 2 times, most recently from 3c62d1f to 84b5fb8 Compare January 14, 2018 10:06

move Random to stdlib

28c1e6f

rfourquet force-pushed the rf/stdlib-random branch from 84b5fb8 to 28c1e6f Compare January 14, 2018 13:47

rfourquet merged commit 60cd7cf into master Jan 15, 2018

rfourquet deleted the rf/stdlib-random branch January 15, 2018 01:01

rfourquet mentioned this pull request Jan 15, 2018

move TestHelpers.OAs -> Test.OAs #25568

Closed

ararslan reviewed Jan 15, 2018

View reviewed changes

rfourquet added a commit that referenced this pull request Jan 20, 2018

remove Random.defaultRNG()

a80e80a

This function is an artifact of an intermediate state of PR #24874, but is not needed anymore.

rfourquet mentioned this pull request Jan 20, 2018

remove Random.defaultRNG() #25650

Merged

ararslan pushed a commit that referenced this pull request Jan 21, 2018

remove Random.defaultRNG() (#25650)

c546f03

This function is an artifact of an intermediate state of PR #24874, but is not needed anymore.

stevengj mentioned this pull request Aug 14, 2019

bugfixes in Libc.rand for Windows #32895

Merged

Keno pushed a commit that referenced this pull request Jun 5, 2024

move Random to stdlib (#24874)

5438aef

		@@ -0,0 +1,124 @@
		# This file is a part of Julia. License is MIT: https://julialang.org/license

		# copied verbatim from julia/test/TestHelpers.jl

move Random to stdlib #24874

move Random to stdlib #24874

Conversation

rfourquet commented Dec 1, 2017 • edited Loading

ViralBShah commented Dec 1, 2017

ViralBShah commented Dec 12, 2017

rfourquet commented Dec 12, 2017

ViralBShah commented Dec 12, 2017

rfourquet commented Dec 13, 2017

StefanKarpinski commented Dec 13, 2017

rfourquet commented Dec 13, 2017

rfourquet commented Dec 14, 2017

rfourquet commented Dec 15, 2017

rfourquet commented Dec 16, 2017

rfourquet commented Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristofferC Jan 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KristofferC commented Jan 8, 2018 • edited Loading

fredrikekre commented Jan 8, 2018

rfourquet commented Jan 8, 2018

fredrikekre commented Jan 8, 2018

ViralBShah commented Jan 9, 2018

rfourquet commented Jan 9, 2018

KristofferC commented Jan 9, 2018

rfourquet commented Jan 11, 2018

rfourquet commented Jan 15, 2018

ViralBShah commented Jan 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfourquet commented Dec 1, 2017 •

edited

Loading

rfourquet commented Jan 8, 2018 •

edited

Loading

KristofferC Jan 14, 2018 •

edited

Loading

KristofferC commented Jan 8, 2018 •

edited

Loading