Conversation
set_tf_seed() wrote self$seed into dag$tf_environment$rng_seed, which nothing reads. mcmc() therefore drew a seed from R's RNG and discarded it, and a run was only reproducible when TensorFlow's global seed had already been set by something else in the session. It now calls tensorflow::set_random_seed(), guarding the two side effects calculate() also guards: that function calls set.seed() internally, so R's stream is saved and restored, and it sets CUDA_VISIBLE_DEVICES = -1 unless disable_gpu is passed, so that follows compute_options. The existing test passed only because of test-order pollution: the calculate() tests above it in test_seed.R set TensorFlow's seed as a side effect, so run on its own it failed. It also asserted that set.seed() and tensorflow::set_random_seed() give *different* draws - encoding the bug, and contradicting ?mcmc, which says they are identical. Both are fixed. The new test perturbs TensorFlow's global seed between two runs, so it cannot pass on session state left by an earlier test. Snapshots in test_greta_mcmc_list_class.R are updated: draws for a fixed seed change, and were verified stable across three runs. Part of #285, #427. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The note said set.seed() and tensorflow::set_random_seed() "both give identical results", which was aspirational - only the second worked, and only because it sets R's seed as well as TensorFlow's. Now that greta passes its own seed through, set.seed() is sufficient, and the note says which part each seed was doing: R's for the initial values, TensorFlow's for the sampler. The example ended by asserting the two routes give different answers. They now agree, so it asserts that instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The first version of this called tensorflow::set_random_seed(), which does two things beyond seeding: - it calls set.seed(), which would advance the user's R stream on every mcmc() call - unless disable_gpu = FALSE it sets CUDA_VISIBLE_DEVICES = -1, and never puts it back. mcmc() defaults to cpu_only(), so one ordinary run would have hidden the GPU from every later one in the session. mcmc() has never touched that variable before. Seeding the Python side directly avoids both. self$seed already came from R's RNG, so R is seeded by construction and there is nothing to restore. Also notes in ?mcmc that seeding covers the random numbers and not the arithmetic: some TensorFlow ops accumulate non-deterministically on GPU, so a seeded GPU run can still vary. CPU is the default and is reproducible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two regressions the first version of this fix would have introduced, now guarded: - cpu_only() and gpu_only() in one session. tensorflow::set_random_seed() sets CUDA_VISIBLE_DEVICES = -1 unless told otherwise and never puts it back, so an ordinary CPU run would have hidden the GPU from every later one. The test runs CPU, GPU, CPU and asserts the variable is untouched. - mcmc() advances R's stream rather than resetting it: the same seed gives the same run, but a second run without re-seeding does not repeat the first. The second test deliberately does not assert how many draws mcmc() takes from R. It takes more than one - get_seed() draws a seed, initial values use rnorm(), and HMC draws its leapfrog count per burst - and pinning that count would break on any of those changing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
calculate() called tensorflow::set_random_seed(), which sets CUDA_VISIBLE_DEVICES = -1 unless told otherwise and never puts it back, so one calculate(nsim = ) on the default cpu_only() hid the GPU from everything later in the session. Same bug mcmc() had. The fix is *not* the one mcmc() uses. Seeding only Python and TensorFlow there made calculate(seed = 42) depend on where R's stream happened to be - an intervening runif() changed the result - because sampling under nsim = draws from R's RNG as well as TensorFlow's. So set_all_seeds() keeps set.seed() and drops only the CUDA line. Draws are unchanged; no snapshots move. mcmc() stays as it is, and now says why: it draws its seed *from* R's stream, so re-seeding there would be circular and would reset the draws HMC takes for its leapfrog count partway through a run. Part of #285, and removes one of the two motivations from #839. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Each sampler draws its own seed from the calling session's stream, since get_seed() is build_sampler()'s default argument and so is evaluated once per sampler. Under future the sampler carries that seed to the worker, where set_tf_seed() runs during tracing and seeds that worker's Python and TensorFlow. Both properties are worth pinning: runs repeat under the same seed, and the chains are not all the same, which is what would happen if they shared one seed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
With more chains than samplers, some chains share a sampler and so share a seed. Starting them all from the same point leaves the sampler's own randomness as the only thing that can separate them - and it does, because the free state is a batch dimension, so TFP draws each chain's momentum separately. Worth pinning because the failure would be quiet: identical chains look like perfect convergence rather than like a bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Checking the new tests against main showed two of them passing there, which means they were not testing this change at all. "chains are seeded distinctly and reproducibly" passed because the test above it perturbed TensorFlow's seed to the same constant, 999, so the perturbation was a no-op: the first run already had that seed. Both now call perturb_tf_seed(), which draws a value, so two tests cannot collide and neither can inherit a seed an earlier test left set. With that, three of the five new seed tests fail on main and pass here. The other two - that mcmc() advances R's stream rather than resetting it, and that chains stay distinct from identical starting values - pass on main too. They guard properties this branch could have broken rather than the bug it fixes, which is worth keeping but worth being clear about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work in Progress, aiming to resolve #285 and #427
Resolves #285
Resolves #427