Add fixes for numpy >= 1.19.

[jnphilipp]

Update randomness to respect new numpy.random API.

[thequilo]

Why did you add this check? The seed should certainly be set for all versions of sacred, not only for the new one.

[jnphilipp]

Sry, wrong direction. np.random.seed is depreciated / legacy in 1.19.

[thequilo]

Can you run black to reformat the code? (Here: use double quotes)

[jnphilipp]

Done.

[thequilo]

The checks (still on an older version of numpy) fail with AttributeError: module 'numpy.random' has no attribute 'Generator' where you do the type check. Is there another way to detect that? Don't know which variant is the best, but you could use hasattr('integers') or try/except

[thequilo]

I believe the failing checks are not caused by your changes. There is still one thing with numpy 1.19: currently, the default random number generator is not seeded by sacred, but it should be seeded for reproducibility. I don't see a way of setting a global seed for the default RNG in numpy 1.19. Also, I don't know if using rnd.integers(*SEEDRANGE, dtype=int) for seeding is the best way to go. It might be better to use a SeedSequence, though I haven't looked into the details yet.

[jnphilipp]

As far as I understand the new api there is no longer a default rng. For the new api you need a rng and you only get it with default_rng. For the moment we can still seed the old api with np.random.seed(seed) which for the time being might be a good idea.
I think SeedSequence might not be the right think for what get_seed is supposed to do.

[thequilo]

I think SeedSequence might not be the right think for what get_seed is supposed to do.

That's right, but I think to use the new random API correctly, the code should generate one seed for the experiment and then use the seed sequence to directly generate the random generators like so:

import numpy as np
# Create a new seed sequence. Store its entropy as the seed
seed_sequence = np.random.SeedSequence()

# In get_rng:
rng = np.random.default_rng(seed_sequence.spawn(1)[0])

This creates a deterministic sequence of random number generators that is "good" (in the sense that numpy says good RNG states should look like, which is not guaranteed by random integers from SEEDRANGE) and that is not influenced by other libraries (like MongoDB #573 #783).

We would have to rework the structure of the random code a little bit, but the code could need some polishing anyway.

Another point: If we change get_rng to return a np.random.default_rng, current code that uses the _rng breaks on numpy 1.19. We could add an option to switch between old and new rng behavior.

@jnphilipp what do you think about this?

[jnphilipp]

As far as I understand the new API and have tested we don't need to call spawn:

import numpy as np

seed = 123456 # for reproducibility
seed_sequence = np.random.SeedSequence(seed)

rng = np.random.default_rng(seed_sequence)

An option to switch is a good idea, we should also display a warning that this a legacy API, which support will end.

[thequilo]

I think we have to call spawn to get a new seed:

import numpy as np
sq = np.random.SeedSequence()
# Without spawn: Creates RNG with same seed:
np.random.default_rng(sq).integers(100)
Out[5]: 90
np.random.default_rng(sq).integers(100)
Out[6]: 90
np.random.default_rng(sq).integers(100)
Out[7]: 90
np.random.default_rng(sq).integers(100)
Out[8]: 90
# With spawn: Creates RNGs with different seeds but in a reproducible way
np.random.default_rng(sq.spawn(1)[0]).integers(100)
Out[9]: 83
np.random.default_rng(sq.spawn(1)[0]).integers(100)
Out[10]: 22
np.random.default_rng(sq.spawn(1)[0]).integers(100)
Out[11]: 92
np.random.default_rng(sq.spawn(1)[0]).integers(100)
Out[12]: 43

[jnphilipp]

Yeah I think you're right.

[jnphilipp]

Changed the create_rnd function to use SeedSequence, but in that case it ignores the given seed, which I think is not expected behavior, need to rethink that.
I'm not sure about the option to switch between old/new rnd, global cmd argument passed to create_rng and a warning message?

[thequilo]

Right, that would need a little more thinking I believe. The current processing does this:

1. Generate a global seed (or get from config), set global seeds of packages (np.random.seed, random.seed, ...)
2. Create one rng for each captured function, where the seed of those rngs is generated based on the global seed
3. On every invocation of a captured function: Get a new seed for this invocation from the function's rng and initialize a new rng with that seed

With this processing, a captured function always gets the same sequence of seeds when called multiple times.

For the new random API it could look like this:

1. Create a SeedSequence (randomly or given the seed from the config)
2. Spawn one child sequence for every captured function
3. Generate a new seed and rng from the seed sequence on every invocation of a captured function

The second variant (new API) is way cleaner than the old one, but I have no idea how to combine those two without overcomplicating everything. The only way I can think of right now would be to write a wrapper that behaves similarly to the SeedSequence that abstracts away the usage of the old and new API.

For the option: I would add an option to sacred.SETTINGS, something like "NUMPY_RANDOM_API_VERSION". This is the current way of handling such settings. It is globally accessible, but unfortunately not from the CLI (#771).

[jnphilipp]

@thequilo if you think this is good, I'll add the settings stuff.

[thequilo]

Hmm, I'm not sure what to do exactly here. I initially thought about something like a RngSequence that directly generates rngs. We would then need one class that generates random.Random, one for random.RandomState and one for random.Generator. This would make the code that uses the rng straightforward, and it would be trivial to add a different random generator, e.g., some custom implementation. But it is not so easy to rewrite the code so that it doesn't break anything. The main problem I see here is that the SeedSequence doesn't actually generate a sequence of seeds (as I thought) and that it is not reproducible by its entropy, so generating seeds to pass to "_seed" and store in the config is not trivial with the SeedSequence.

I don't see much advantages of your code over the previous one and it has the side-effect that older experiments are not reproducible because random.Random uses a different rng than np.random.RandomState.

I don't want to bother you too much with this, so to quickly get a running version of sacred with support for the new random API, we could go back to the beginning and don't use the SeedSequence. I think I now better understand how the SeedSequence works. Your initial implementation should work fine and generate "good" starting points for the rngs, while in the beginning, I thought it wouldn't.

[thequilo]

This breaks previous experiments. Previously, np.random.RandomState was used to create seeds which uses a different random number generator than random.Random, so with this change older experiments would not be reproducible

[thequilo]

Here, you can simply pass the seed_sequence to np.random.default_rng because it is only used for this one rng.

[thequilo]

When does this fail?

[jnphilipp]

When the legacy random API gets removed.

[jnphilipp]

I first started by writing something like a RngSequence, but then I went through the code to see how it would be used and since apart from the captured function that actually want a rng every were else it is just about seeds. That is why I settled for the SeedIterator. Maybe this can be something for a later version were we have breaking changes.

I noticed that SeedSequence.spawn returns SeedSequence with the same entropy just with different spawn_key, that is why I settled for random.Random.

I had another look at the default_rng function and noticed that it is in numpy since v1.17 and with v1.19 came the legacy mark.

I'll revert and add the settings stuff.

[thequilo]

You can directly access the setting from SETTINGS.CONFIG.NUMPY_RANDOM_LEGACY_API here. Is there a reason for the argument? If I saw this correctly, it is always set to SETTINGS.CONFIG.NUMPY_RANDOM_LEGACY_API. Or do you want to allow setting the API version independently for every captured function?

[thequilo]

I like the name!

[thequilo]

Okay, now I fully understand why you implemented it as it is. It makes sense that you didn't go for the RngSequence and it definitely requires more thought. It also makes sense that you didn't use the SeedSequence, but using random.Random, while in general, it would work, it breaks past experiments. We have to make sure here that experiments stay reproducible even after updating sacred.

I think now the only thing that's missing is a test case for the new setting.

[thequilo]

Something is wrong with the tensorflow 1.14 and 2 test cases. There isn't even a console output on azure. This is not caused by your changes, so I'd say this is fine.

There is only one thing left that I'm unsure about: Currently, NUMPY_RANDOM_LEGACY_API is set to False by default, which could break code if people pull the master. How about setting it to True for numpy < 1.19 to follow their deprecation scheme?

[jnphilipp]

About the test in general: Python 3.8 and 3.9 are not tested and 3.5 has no longer support and 3.6 has its EOL in December. Also currently supported Tensorflow version are 1.15, 2.0, 2.1, 2.2, 2.3 and 2.4. I think we should change the tests accordingly.

[thequilo]

Yes, fixing these test issues is on my to-do list, but I didn't have time to fix them yet. They got neglected over the past year or so because nobody actively maintained sacred. If you want to, you can adjust the test to support the newer python versions and remove unsupported versions of tensorflow, but better in a separate PR. Removing Python 3.5 has the advantage that we can use f-strings.

[jnphilipp]

K, will do. Merge this first though?