ENH: use SeedSequence instead of seed() #13780

mattip · 2019-06-14T08:02:38Z

Modify BitGenerators to use a SeedSequence class for seeded entropy, the class also provides a spawned() API to generate a number of independent entropy states for creating independent random streams.

Based on this gist by @rkern, the idea is

create a common base class for all BItGenerators
BitGenerator(seed) accepts an int, a sequence of ints, or a SeedSequence.
There is no longer a seed method to modify an existing BitGenerator, since creation of a new one is cheap. To seed a BitGenerator, simply create a new one with the desired seed value. This may change based on user acceptance of the idea.

The first commit only changes PCG64, as a proof of concept, since the change is rather large, and does not expose the SeedSequence's spawn method to the BitGenerator.

mattip · 2019-06-14T08:08:11Z

xref #13164, #13685, #13635, #13675

numpy/random/tests/test_smoke.py

mattip · 2019-06-14T08:12:17Z

Hmm, python3.5 does not have a secrets module. Some other source of entropy is needed.

bashtage · 2019-06-14T15:05:26Z

Hmm, python3.5 does not have a secrets module. Some other source of entropy is needed.

You can use random_entropy from numpy.random.entropy.

bashtage

Sorry for the multitude of comments.

I think BitGenerator is effectively to defining the interface and so needs to be adaptable to different generators.

numpy/random/bit_generator.pyx

bashtage · 2019-06-14T15:14:43Z

numpy/random/bit_generator.pyx

+        else:
+            raise TypeError('{}(seed) expects int or SeedSequence for seed '
+                             'not {}'.format(type(self).__name__, seed))
+        self._seed = seed


How does the seed get set into the bit generators state? In the old model, you would not call self.seed(self._seed)

One could use a private class self._set_seed(seed) that is requried by children that handles specific seeding.

https://github.com/numpy/numpy/pull/13780/files/82453cdb6cb2d0d99b2394a78e1af13083324f30#diff-81acdee7a38ef6ded447175da539d1a6R105

I don't think I understand. I was thinking that the the BitGenerator class would call self._set_seed(self._seed) (which is provided by PCG64._set_seed(self, value)) at the end of its __init__. Might not be worth the hassle since then PCG64.__init__ would have to ensure that everything existed before the super call (which is fine for PCG64, but not for a generator that needs aligned memory).

The BitGenerators call SeedSequence.generate_state, then use that value to set their seed. in particular PCG64 calls

val = self._seed_seq.generate_state(4, np.uint64); pcg64_set_seed(&self.rng_state, <uint64_t *>np.PyArray_DATA(val), <uint64_t *>np.PyArray_DATA(val) + 2)

numpy/random/bit_generator.pyx

numpy/random/pcg64.pyx

numpy/random/bit_generator.pyx

numpy/random/tests/test_smoke.py

bashtage · 2019-06-14T15:37:10Z

There is no longer a seed method to modify an existing BitGenerator, since creation of a new one is cheap. To seed a BitGenerator, simply create a new one with the desired seed value. This may change based on user acceptance of the idea.

IMO this is not a good idea. Think about testing. Why create many generators when you really want to use the same one with the same state? The cost of creating a generator may also depend on the size of the state. For example, MT19937 allocates 4*624 + 4 bytes of state.

numpy/random/bit_generator.pyx

rkern · 2019-06-14T18:57:54Z

Think about testing. Why create many generators when you really want to use the same one with the same state?

It's actually pretty hard in most test frameworks to make just one object for the test suite and reseed it each test. You have to go out of your way to do it. It's much easier just to make it fresh each time in the setUp(). Tests are not time-critical, in any case, and even "large" PRNGs like MT are only large relative to other PRNGs. They are not large compared to many of the other objects in the tests.

numpy/random/pcg64.pyx

charris · 2019-06-14T21:05:56Z

Doc failure looks legitimate.

bashtage · 2019-06-15T17:14:02Z

It's actually pretty hard in most test frameworks to make just one object for the test suite and reseed it each test. You have to go out of your way to do it. It's much easier just to make it fresh each time in the setUp(). Tests are not time-critical, in any case, and even "large" PRNGs like MT are only large relative to other PRNGs. They are not large compared to many of the other objects in the tests.

What I don't understand is that is the upside to moving seed from being public to private? There is a downside (creating 2 or 3 extra objects, depending on how the seed is implemented).

IME in pytest is easy to create a global automatically reset generator in conftest using a fixture, e.g.,

SEED = 12343928
GENERATOR = np.random.Generator(SEED)
@pytest.fixture()
def reset_generator():
    """
    Fixture that provides a Generator using the same initial state
    """
    GENERATOR.bit_generator.seed(SEED)
    yield GENERATOR

and then

def test_something(reset_generator):
    reset_generator.integers(...)

Of course one can do the same thing by grabbing and then resetting the initial state, so maybe this isn't much of a concern in this context.

The proof is in the pudding I suppose:

In [20]: %timeit rg.Generator(rg.PCG64())
27 µs ± 161 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)

In [21]: %timeit g.seed(12334)
5.49 µs ± 75.2 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

In [22]: state = g.state
In [23]: %timeit g.state = state
2.39 µs ± 25.4 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)

So directly setting the state is fastest, which makes sense since it has the fewest safeties. This said, none are too slow in any important way, and even 10K calls is < 1s.

mattip · 2019-06-15T20:20:40Z

I am aiming for the minimal interface for the first release. We can always add seed back in if the performance hit hurts users.

rkern · 2019-06-16T00:00:07Z

We've seen such great misuse of .seed() in the old design as it's presence suggests to people that "this is the way to provide the seed for the RNG", that micro-optimizations aren't enough, in my mind, to keep it.

numpy/random/bit_generator.pyx

seberg · 2019-06-16T04:37:55Z

I have to agree with being minimal. If there is any other reason except slight optimization, sure. But I doubt few tests even notice that slowdown.

mattip · 2019-06-16T06:36:05Z

Does the design pattern for SeedSequence seem reasonable (create it in the base BitGenerator.__init__, use it in the child PCG64.__init__)? If so, I will convert the other BitGenerators to use this pattern

mattip · 2019-06-19T16:05:11Z

I have moved dSFMT, MT19937 and PCG32 to use seeding via SeedSequence (MT19937 will only use the new SeedSequence if a SeedSequence is provided for legacy reasons). I also updated the documentation.

I don't think it makes sense to use SeedSequence in the counter-based ThreeFry and Philox since they already mix some entropy into their initial state.

rkern · 2019-06-19T16:54:46Z

628C

The use of SeedSequence to convert non-ISeedSequence inputs should be universal in order to enable the later addition of Generator.spawn(). We won't be able to add that later if it's not the default now. It also enables all BitGenerators to accept any of the various forms of entropy that SeedSequence accepts, so it makes the APIs more uniform.

The only thing that needs the old MT19937 seeding is RandomState(seed), not MT19937(seed), so that can be an implementation detail of RandomState.

rkern · 2019-06-19T17:31:25Z

numpy/random/mt19937.pyx

+                        seed = random_entropy(1, 'fallback')
+                    mt19937_seed(&self.rng_state, seed[0])
+                elif isinstance(seed, SeedSequence):
+                    val = self._seed_seq.generate_state(1, np.uint32)


Please get at least 128 bits.

is more better? Should I use RK_STATE_LEN?

Ideally, we'd bypass mt19937_init_by_array() entirely by pulling RK_STATE_LEN uint32 words, then make sure the first word isn't 0 like in the usual routine. Then that's the state.

That will let us replace SeedSequence with a LegacyMTSeedSequence at the Python level that replicates mt19937_init_by_array(). That would be a good way to implement RandomState(seed).

If following this path, should probably disallow BitGenerators from being input into RandomState, so that RandomState(MT19937(seed)) is not allowed since it will be different from RandomState(seed)

I would prefer not to refactor the code from mt19937_init_by_array into a LegacyMTSeedSequence.

Okay, @classmethod it is, then.

Reworked. Now RandomState.seed calls MT19337.legacy_seeded. If RandomState has a different BitGenerator, the call will fail. Also reworked the tests to create new Generators for each test rather than reseed.

Hmm, NEP 19 says (well, I said) that in such cases we should pass the provided seed through to the BitGenerator. Though at the time I was probably still thinking of BitGenerator.seed() being an API that people might use, however disrecommended it was. I think I prefer the semantics you have now, though. If people are calling np.random.seed(), they probably wanted to reproduce the old bitstream, so making this error out when someone has threaded through another BitGenerator should raise the discrepancy up to them.

If needed we can propose a change to the NEP since we now have a better picture of how this all should work

numpy/random/pcg32.pyx

numpy/random/pcg64.pyx

bashtage

Mostly formatting issues.

numpy/random/bit_generator.pyx

numpy/random/bit_generator.pxd

numpy/random/bit_generator.pyx

numpy/random/dsfmt.pyx

bashtage · 2019-06-19T23:02:56Z

numpy/random/mt19937.pyx

+                        seed = random_entropy(1, 'fallback')
+                    mt19937_seed(&self.rng_state, seed[0])
+                elif isinstance(seed, SeedSequence):
+                    val = self._seed_seq.generate_state(1, np.uint32)


If following this path, should probably disallow BitGenerators from being input into RandomState, so that RandomState(MT19937(seed)) is not allowed since it will be different from RandomState(seed)

bashtage · 2019-06-19T23:09:02Z

The use of SeedSequence to convert non-ISeedSequence inputs should be universal in order to enable the later addition of Generator.spawn(). We won't be able to add that later if it's not the default now. It also enables all BitGenerators to accept any of the various forms of entropy that SeedSequence accepts, so it makes the APIs more uniform.

I've been wondering about the API design for this. Is it necessary for a BitGenerator to expose this up to a Generator? In particular, I could imagine another approach that has a signature like

pgen = ParallelGenerator(seed: int, generator:Generator-like, bit_gen:BitGenerator, seed_size: int = 128)

that would be able to enable sequenced creation of any Generator-like object from any BitGenerator-like object as long as the BitGenerator can accept 32-bit unsigned integers. This avoids tight coupling of SeedSequence at the bottom and simplifies writing Generator-like classes that could contain user-provided distributions.

This is orthogonal to the use of SeedSequence internally to the BitGenerators I think.

rkern · 2019-06-19T23:29:58Z

Can you show a usage example? I don't have a clear idea from that constructor what you are proposing.

numpy/random/bit_generator.pyx

mattip · 2019-06-25T08:11:02Z

Let's keep this instancemethod private, either with an underscore ...

Adopting

mattip · 2019-06-25T08:12:24Z

This doesn't work. If this is the method that RandomState.seed(None) ...

What is "This"?

rkern · 2019-06-25T08:17:35Z

You should be testing that legacy_seeding creates an equivalent state.

All the tests in test_random have to use legacy_seeding since they use the old API, I did not change those test results and the tests pass

Well now you have the test's name test_seed_equivalency() opposite to the test's contents. The test was written to exercise certain semantics that now appear under a different API. Test that new API. Or delete the test; what you are testing now has no use.

bashtage · 2019-06-25T08:18:03Z

All the tests in test_random have to use legacy_seeding since they use the old API, I did not change those test results and the tests pass

IIRC all of the tests use a scalar seed. Might be worth triple checking that there is at least one that uses the vector seed path, and adding one with a random-looking seed.

mattip · 2019-06-25T08:25:50Z

TestSeed seems to have the cases you mentioned.

rkern · 2019-06-25T08:30:13Z

This doesn't work. If this is the method that RandomState.seed(None) ...

What is "This"?

Are you able to see the code context of my comment? I made it on the commit, so maybe it's not being displayed to you.

val = self._seed_seq.generate_state(1, np.uint32)

SeedSequence.generate_state() is idempotent. For a given requested length, it returns the same thing every time. That means that 2 calls to .legacy_seeding(None) resets the state to the same thing. This is not the required semantics, which is to get fresh entropy from the OS on each call.
32 bits is too small of a bottleneck. We've been over this before, and it's as true in this context as the previous one.

mattip · 2019-06-25T11:27:45Z

Since it seems Philox is one of the BitGenerators that will be shipped, I refactored it to use SeedSequence

rkern

There are just a few items left that could be addressed in a subsequent PR. Getting this in will help unblock some of the other PRs.

rkern · 2019-06-25T19:49:58Z

doc/source/reference/random/index.rst

+Seeds can be passed to any of the BitGenerators. The provided value is mixed
+via `~.SeedSequence` to spread a possible sequence of seeds across a wider
+range of initialization states for the BitGenerator (except for `~.MT19937`
+which for legacy reasons will use the older initialization methods). Here


This is now untrue.

rkern · 2019-06-25T19:50:17Z

doc/source/reference/random/index.rst

-`RandomState` object.  Random number generation is separated into two
-components, a bit generator and a random generator.
+`RandomState` object.  Random number generation is separated into three
+components, a seed sequencer, a bit generator and a random generator.


Suggested change

components, a seed sequencer, a bit generator and a random generator.

components, a seed sequence, a bit generator and a random generator.

rkern · 2019-06-25T19:51:25Z

doc/source/reference/random/index.rst

@@ -153,6 +170,11 @@ The included BitGenerators are:
 * MT19937 - The standard Python BitGenerator. Produces identical results to
  Python using the same seed/state. Adds a `~mt19937.MT19937.jumped` function


Only true with _legacy_seeding()

rkern · 2019-06-25T19:57:01Z

numpy/random/mt19937.pyx

+        sequence) of unsigned 32-bit integers, a `SeedSequence`, or ``None``
+        (the default).  If `seed` is ``None``, a 32-bit unsigned integer is
+        read from ``/dev/urandom`` (or the Windows equivalent) if available. If
+        unavailable, a 32-bit hash of the time and process ID is used.


This text is out of date. It should be rationalized with the other SeedSequence-using BitGenerators.

rkern · 2019-06-25T20:02:47Z

numpy/random/philox.pyx

-            self.rng_state.ctr.v[i] = counter[i]
-
-        self._reset_state_variables()
-


The ctypes and cffi properties can also be deleted, as they are provided by BitGenerator.

rkern · 2019-06-25T20:06:24Z

numpy/random/tests/test_generator_mt19937_regressions.py

@@ -132,7 +131,7 @@ def test_permutation_subclass(self):
        class N(np.ndarray):
            pass

-        mt19937.bit_generator.seed(1)
+        mt19937 = RandomState(1)


Why is this test using RandomState while all of the others using Generator?

Removing last vestiges of RandomState from this file.

rkern · 2019-06-25T20:12:17Z

Needs a rebase, of course.

mattip · 2019-06-25T20:39:05Z

I will rebase once #13793 is merged

mattip · 2019-06-25T20:51:10Z

There are 31 commits here, should I rebase and squash them down to 1 or leave it be?

charris · 2019-06-25T21:16:32Z

Might as well squash them down, it will make the rebase easier.

charris · 2019-06-25T22:09:55Z

Should be good to rebase now.

charris · 2019-06-25T22:43:48Z

CircleCI has found a couple of nits.

numpy/random/tests/test_direct.py

bashtage · 2019-06-25T22:57:38Z

numpy/random/philox.pyx

-    def __init__(self, seed=None, counter=None, key=None):
+    def __init__(self, seed_seq=None, counter=None, key=None):
+        if seed_seq is not None and key is not None:
+            raise ValueError('seed and key cannot be both used')


Should probably be 'seed_seq and key`

numpy/random/pcg64.pyx

doc/source/reference/random/bit_generators/index.rst

bashtage · 2019-06-25T23:19:49Z

numpy/random/bit_generator.pyx

+    n_children_spawned : {int}, optional
+        The number of children already spawned. Only pass this if
+        reconstructing a `SeedSequence` from a serialized form.
+


Does this need an Attributes section for program_entropy and pool? If these are private should they be _ed?

charris · 2019-06-25T23:23:51Z

Fixed the merge problem, let's see if things go.

@bashtage Lets get this in and then do the fixups. I think it is getting late out your way and Matti will be traveling tomorrow.

charris · 2019-06-25T23:37:33Z

What should be the fix here?

   def test_pickle(self):
        import pickle
    
        bit_generator = self.bit_generator(*self.data1['seed'])
        state = bit_generator.state
        bitgen_pkl = pickle.dumps(bit_generator)
        reloaded = pickle.loads(bitgen_pkl)
        reloaded_state = reloaded.state
        assert_array_equal(Generator(bit_generator).standard_normal(1000),
                           Generator(reloaded).standard_normal(1000))
        assert bit_generator is not reloaded
        assert_state_equal(reloaded_state, state)
    
>       ss = SeedSequence(100)
E       NameError: name 'SeedSequence' is not defined

rkern · 2019-06-25T23:54:56Z

Just import it. With all of the squashing, it's hard to tell what happened.

charris · 2019-06-26T01:53:02Z

I'm putting this in, Shippable is well behind at this point.

@bashtage @rkern Please make further fixups as PRs on master.

Thanks to all the reviewers, and thanks to Matti for hanging in there :)

rkern · 2019-06-26T02:30:42Z

Thank you all for your patience and endurance!

mattip added 01 - Enhancement component: numpy.random labels Jun 14, 2019

mattip commented Jun 14, 2019

View reviewed changes

numpy/random/tests/test_smoke.py Outdated Show resolved Hide resolved

bashtage reviewed Jun 14, 2019

View reviewed changes

numpy/random/bit_generator.pyx Show resolved Hide resolved

rkern reviewed Jun 14, 2019

View reviewed changes

numpy/random/pcg64.pyx Outdated Show resolved Hide resolved

charris added this to the 1.17.0 release milestone Jun 14, 2019

seberg reviewed Jun 16, 2019

View reviewed changes

8000

numpy/random/bit_generator.pyx Show resolved Hide resolved

mattip changed the title ~~WIP, ENH: use SeedSequence instead of seed(), jumped()~~ WIP, ENH: use SeedSequence instead of seed() Jun 16, 2019

mattip force-pushed the seedsequence branch from b5f5e15 to 4724fb0 Compare June 19, 2019 17:22

rkern reviewed Jun 19, 2019

View reviewed changes

numpy/random/pcg32.pyx Outdated Show resolved Hide resolved

rkern reviewed Jun 19, 2019

View reviewed changes

numpy/random/pcg64.pyx Outdated Show resolved Hide resolved

bashtage reviewed Jun 19, 2019

View reviewed changes

bashtage reviewed Jun 20, 2019

View reviewed changes

numpy/random/bit_generator.pyx Show resolved Hide resolved

mattip force-pushed the seedsequence branch from 0585851 to 79f9251 Compare June 25, 2019 19:08

rkern approved these changes Jun 25, 2019

View reviewed changes

ENH: use SeedSequence to generate entropy for seeding

efa35e7

mattip force-pushed the seedsequence branch from ef14924 to f6e8d21 Compare June 25, 2019 22:20

MAINT: remove leftover files, fix docs

c69f90c

mattip force-pushed the seedsequence branch from f6e8d21 to c69f90c Compare June 25, 2019 22:47

rkern reviewed Jun 25, 2019

View reviewed changes

numpy/random/tests/test_direct.py Outdated Show resolved Hide resolved

bashtage reviewed Jun 25, 2019

View reviewed changes

MAINT: Fix bad merge.

0487fb0

BUG: Fix missing SeedSequence import.

8a0c54a

charris merged commit 4668b46 into numpy:master Jun 26, 2019

mattip deleted the seedsequence branch October 13, 2020 13:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: use SeedSequence instead of seed() #13780

ENH: use SeedSequence instead of seed() #13780

	components, a seed sequencer, a bit generator and a random generator.
	components, a seed sequence, a bit generator and a random generator.

		@@ -153,6 +170,11 @@ The included BitGenerators are:
		* MT19937 - The standard Python BitGenerator. Produces identical results to
		Python using the same seed/state. Adds a `~mt19937.MT19937.jumped` function

		self.rng_state.ctr.v[i] = counter[i]

		self._reset_state_variables()

ENH: use SeedSequence instead of seed() #13780

ENH: use SeedSequence instead of seed() #13780

Conversation

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment