-
Notifications
You must be signed in to change notification settings - Fork 268
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
Implement reproducible seeding #1288
Conversation
Wow, thanks a lot for working on this @marcharper it looks like it was a chunk of work to get to the bottom of. I've read your explanation/solution proposal and it all looks excellent to me. I don't think this only fixes #1277 but it also makes the whole seeding much nicer from a user point of view as well . Let me know if you'd like to share out some of the remaining work (or if I can help in any other way). |
I sat down thinking it wouldn't be that difficult and then it keep growing. I can handle the rest of the changes -- there are a few thorny cases to handle like making some of the transformers play nicely with deterministic (non-seeded players) but probably easier for me to tackle now that I'm pretty far in. Do you have any opinions on the names of the new classes or variables? I used Perhaps you can take a look at the tests and look for more hypothesis opportunities or entirely new tests (maybe after this PR is finished). The axelrod-fortran update should be easy, just need to reuse the new RNG classes. Folding in the evolutionary algorithms should happen soon after and then I think we're in good shape. |
I am happy with the names you got so far. Forgive me in advance if I make suggestions for changing one or two when I review more carefully but I don't think that'll happen and I doubt I'll feel strongly. In this specific case I think the only preference I'd have is
Yeah good idea. I'll pick that up afterwards. |
Is this so? The way I'm reading it, both _run_serial and _run_parallel start by building chunks using self.match_generator, which should have the seed in the state you left it at the previous run. Am I misunderstanding? |
I think you're right, we'd need to have match generator reuse its seed if we want the second run to match the prior run. But running two tournaments in row (e.g. calling play() twice) with different Tournament instances (and the same seed) should generate the same pair of results since the match generator is exhausted by Tournament before batching out matches. |
Making a note here to make sure we don't forget to modify the documentation if #1329 goes in before. I'd recommend we still give hypothesis an upper bound as in the past they've developed things so quickly it's often broken things but at least the new cron CI builds would catch that. |
…er classes to use new seeding methods.
Update: Since this is taking a while I decided to go ahead and finish it rather than fold in the rest of the dojo and #1282. This branch was rebased and many tests fixed. Files still containing failing tests:
|
This is a big piece of work 👍 Let me know if I can assist in any way. |
Let me give you another idea. I am aware that the problem of thread schedulers and reproducible "randomness" has been tackled by using a different design from traditional PRNGs. Notably, so-called "splittable" PRNGs have been developed for use in "isolated parallel computations that may generate subtasks", such as event processing that could be done by individual threads. See also freeorion/freeorion#2856 (comment), the JAX PRNG Design Notes, and Java's SplittableRandom documentation. |
Hi @peteroupc thanks for the links. I'm a fan of Jax :) IIUC what we're doing here is quite similar in that each task ends up with it's own PRNG (in fact each match and player, though they are all deterministically linked in a tree depending on the splitting path), so in this case I'm not sure that we gain much from using splittable PRNGs. Since we're going for 100% reproducibility, we'll still have the same issue in that we would need to split out all the PRNGs initially just as we're generating seeds here because we can't guarantee that the order of dispatch will be the same in the distributed case. Am I missing another advantage of using splittable PRNGs that's relevant? |
axelrod/tests/unit/test_match.py
Outdated
rg = RandomGenerator(seed) | ||
r = rg.random() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rg = RandomGenerator(seed) | |
r = rg.random() | |
random = RandomGenerator(seed) | |
r = random.random() |
To be consistent with changes elsewhere?
I'd actually suggest rag = RandomerGenerator(seed)
and using that elsewhere.
Not at all vital to change: whatever you prefer @marcharper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I've been using the RandomGenerator
object as a drop-in replacement for the standard random
module, but it's not quite the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to rng
for now. We can decide on a universal approach throughout pending your documentation effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I've gone through
tests/unit
: have some minor suggestions/questions.
I'm really enjoying reviewing this PR @marcharper: such a nice piece of work 👍
That's all I'm going to have time to do today I'm afraid. I'll be getting back to this on Friday when I'll go over the implemented functionality itself (I might circle back to the tests/docs): really impressive work @marcharper :) 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone through everything now @marcharper. Impressive piece of work.
I wonder if writing up (I'm mainly thinking of a diagram) "how parallelisation works" in the docs somewhere (perhaps in the reference pages) could be a helpful resource. I also wonder if it could be beneficial if someone that's not you writes that and you review. I'd particularly like that person to be me 😆.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have now gone over all the changes. I have left some minor comments. Really nice piece of work 😄 💪
What an awesome piece of work @marcharper 👏 👏 👏 |
This is actually no longer correct since #1288
* Write a new documentation page with branch info This is related to #1352. Once this PR is merged we should: - Change github default branch to `dev` - Delete `master` - Confirm that read the docs is looking at `release` Am I missing anything? We should also remove the release process information from the wiki (assuming we're happy with what I've written here). Finally, once #1360 is done we should make sure we update the docs with the relevant information. * Remove ambiguous `very`. * Remove hypothesis version specification in docs. This is actually no longer correct since #1288 * Test properties not affected by floating point error This build found a particular failing example of `TestTournament.test_seeding_equality` https://github.com/Axelrod-Python/Axelrod/pull/1368/checks?check_run_id=975415322 Upon closer investigation it looks like that was not due to seeding but due to the floating point error of some calculations made by the result set. I investigated using: ``` import axelrod as axl import numpy as np seed = 2 repetitions = 10 rng = axl.RandomGenerator(seed=seed) players = [axl.Random(rng.random()) for _ in range(8)] tournament1 = axl.Tournament( players=players, turns=10, repetitions=repetitions, seed=seed ) tournament2 = axl.Tournament( players=players, turns=10, repetitions=repetitions, seed=seed ) for _ in range(4): results1 = tournament1.play(processes=2, progress_bar=False) results2 = tournament2.play(processes=2, progress_bar=False) assert results1.wins == results2.wins assert results1.match_lengths == results2.match_lengths assert results1.scores == results2.scores assert np.allclose(results1.normalised_scores, results2.normalised_scores) assert np.allclose(results1.ranking, results2.ranking) assert results1.ranked_names == results2.ranked_names assert results1.payoffs == results2.payoffs assert results1.payoff_matrix == results2.payoff_matrix assert np.allclose(results1.payoff_stddevs, results2.payoff_stddevs) assert results1.score_diffs == results2.score_diffs assert results1.payoff_diffs_means == results2.payoff_diffs_means assert results1.cooperation == results2.cooperation assert results1.normalised_cooperation == results2.normalised_cooperation assert results1.vengeful_cooperation == results2.vengeful_cooperation assert results1.cooperating_rating == results2.cooperating_rating assert results1.good_partner_matrix == results2.good_partner_matrix assert results1.good_partner_rating == results2.good_partner_rating assert np.allclose(results1.eigenmoses_rating, results2.eigenmoses_rating) assert np.allclose(results1.eigenjesus_rating, results2.eigenjesus_rating) ``` Note I'm using `np.isclose` for some properties. In this commit: - I add the specific seed for which the error was found as a hypothesis example (`seed=2`). - Replace the `results1 == results2` check with just a check of some properties (from which the others are essentially calculated). - Added `progress_bar=False` * Add instructions for using venv. * s/requirements/requirements.txt * Spell requirements correctly..
This PR
RandomGenerator
, and a bulk generating versionBulkRandomGenerator
EvolvablePlayer
,Match
,Tournament
,MoranProcess
, andFingerprint
objects seed reproducible; updates every strategy to use a local RNGplay
fromPlayer
toMatch
, for example), see Possibly move play method away from Player #1181The solution to #1277 is quite a bit more complex than discussed on the issue, so let me explain it all now so that the code changes will make sense.
The initial issue was the lack of exact reproducibility of multi-threaded tournaments. Here the crux of the issue is that once we start using threads, a global (seeded) random number generator (RNG) isn't necessarily going to generate values in the same order for all the matches across threads because threads may get pre-empted and so on. The solution discussed at #1277 is to generate seeds in the tournament class from a single initial seed and pass those into the threads so that they can be used to make the output of Matches reproducible.
There are a lot of ins and outs at this point because tournaments and matches can be noisy, metaplayers can contain subplayers, and in principle we might eventually have a Match that is itself multi-threaded, so how we make Matches seed-reproducible isn't so obvious. Since a Match can be noisy it needs access to a seeded RNG, as do the players, which currently call global RNG. So each stochastic player has to be re-worked to use a local or referenced RNG, which they could share with the Match or they could have their own.
If Players have their own RNG they also need to be seeded. That brings up the issue of how to handle resetting and cloning in the case of a seeded Player RNG, whether to initialize
Player
s with a non-seeded RNG, and whether matches, when re-run, should reset all the seeds or not. However, given a seed, aMatch
can generate new seeds or RNGs for the Players and so maintain reproducibility, even if we don't reset any of the seeds in subsequent matches. That reduces the need to create a lot of RNG objects (and we need only generate them in the case that a Match is stochastic).Finally note that whatever we do has to also work for axelrod-fortran. Those strategies require random values to be passed into the strategy function in each round, so those players also need to use a deterministic RNG.
Ultimately I decided on the following principles:
Match
that is the logical smallest context of reproducibility; all others derive reproducibility from it.Players
are seedable, but since aMatch
also needs a seed for noise, the best practice is for matches to subsequently seed players in most cases.__init__
means that rerunning a tournament will produce the same results. If we don't want that to be the case then we need to generate the matches on each invokation of play(). This could be changed...match_attributes
because they are not the same for all players like the other match attributesThen the following implementation details follow:
Match
is not supplied a seed, a random seed is used, mimicking the current behavior (with no guarantee of reproducibility)Match
must have a local RNG for reproducibility. ThePlayer.play
function was applying noise so I moved it toMatch
, and updated all uses ofPlayer.play
Player.__eq__
ignores the RNG / seed . This mimicks the current behavior where the global seed had to be set for a player and a clone to behave identically. Now a local seed has to be set for both (e.g. via the Match seed) for reproducibility to be valid.__init__
kwargs, so the seed is not naturally reused on cloning or resetting. I toyed with the alternative -- it's possible to generate seeds on the fly with__new__
but it's hacky and leads to a lot of seeds that get overwritten or are unecessarily generated (e.g. for deterministic players), makes a bunch of tests harder to reason about, and so on.Note that the order of players in a
Match
can affect the outcome (since the seeds generated by theMatch
would be reversed if the order was reversed). The other parameters like noise of course also must be the same.Consequences
Some things still to do:
Fixes #1277
Fixes #1283
Fixes #1181
Fixes #1214