Repository navigation
[MRG] Make BSP-OT random seeding reproducible across calls - #898
Conversation
ot.bsp.compute_bspot_bijection documents `seed` as a reproducibility knob, but the BSP-OT engines are `thread_local std::mt19937` objects constructed once per thread from `global_seed` (BSP-OT_header_only.h:549-553), so the `global_seed = seed` assignment in BSPOT_wrap reaches the engine of the first call made on that thread only: a repeated call continued the previous stream and returned different plans, different intermediary stacks and a different cost (Issue PythonOT#897). The same lines also cost solution quality, because the candidate plans are generated by `#pragma omp parallel for` and each worker built its engine from the same `global_seed`: iterations at equal positions in their chunks consumed identical subsequences and produced identical matchings, so `n_plans=64` yielded ceil(n_plans / n_threads) distinct candidates on this machine and `MergePlansNoPar` walked over the copies. Re-seed the engine at the start of each iteration from a SplitMix64 mix of (user_seed, plan index) and pass the seed down as a parameter, so that candidate plan i is a function of the arguments alone. The mixing must not be additive: with `user_seed + task`, plan t of seed s is plan t-1 of seed s+1, so changing the seed would leave nearly the whole candidate set unchanged. The matching object is now constructed per iteration, which removes the only mutable state shared between the iterations of this loop, and the now dead `global_seed = seed` write is dropped; those two thread-safety edits do not change the result for a given seed.
Repeated calls, interleaved history, a fresh-interpreter reference, invariance to OMP_NUM_THREADS, and the property that candidate plan i is determined by (seed, i), which is checked by comparing the first rows of the candidate stacks returned for n_plans=4 and n_plans=37. Five of the six fail on current master. The candidate matchings are deliberately not asserted to be pairwise distinct: independent streams can legitimately return the same bijection -- for a similarity transform every BSP candidate is the identity -- so distinctness is a property of the instance and of the standard library's distributions rather than of the seeding, and PythonOT#897 reports it as a quality metric. No historical cost or permutation is hardcoded, so the tests do not freeze the new random sequence. The children report the extension hash they loaded and the parent asserts it matches, so a child importing another build cannot silently pass.
Required by .github/workflows/check_release.yml, which fails any pull request that does not touch RELEASES.md.
|
The red checks are an infrastructure failure, not a test failure: every failed or cancelled job died in No job failed at an install, build, lint or test step. For reference, the two jobs that did get past checkout are green on head
with |
|
Hi maintainers, Could someone with write access please re-run the failed jobs for PR #898? The failures occurred during Windows and macOS have already passed. Affected runs:
A re-run would help verify whether there are any actual code or test failures. Thanks for your help! |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #898 +/- ##
==========================================
- Coverage 97.01% 97.00% -0.02%
==========================================
Files 130 130
Lines 26879 26995 +116
==========================================
+ Hits 26078 26187 +109
- Misses 801 808 +7 🚀 New features to boost your workflow:
|
|
Hi maintainers, PR #898 is now conflict-free after syncing with the latest master. All 13 GitHub Actions test jobs and the GitHub Actions documentation build have passed on the new head ( The remaining failure is CircleCI Could someone with CircleCI permissions please rerun this build when convenient? No code changes appear necessary. Thank you! |
baptiste-genest
left a comment
baptiste-genest
left a comment
There was a problem hiding this comment.
Hi!
Thank you for your time! I checked locally: same permutations across runs and thread counts, 64/64 distinct plans, and quality back in line with upstream.
A few small things before merging:
seeddocstring: could you mention that results are now fixed for a given seed (per build), so loops refining withinitial_permshould change the seed each round? With a fixed seed they would stall.RELEASES.md: could you addPR #898to the entry and shorten it a bit to match others?- PR description: the test counts are out of date. There are now 8 new tests.
The merge_bijections out-of-bounds read you mentioned does look real from the code, so a separate issue would be great. Thanks!
|
The comments and tests are overall quite lengthy too, if you see ways to shorten them a bit it would be nice but nothing dramatic. |
State in the seed docstring that candidate plans are reproducible for a given seed within a fixed build, and that a loop refining a solution with initial_perm must change the seed each round, since reusing it repeats the same candidates and stalls refinement. Shorten the RELEASES.md entry to match its neighbours and credit PR PythonOT#898, and drop repeated explanations from the seeding tests.
|
Hi @baptiste-genest, all five points are addressed on
The Local build: MSVC 14.44 with |
|
Thank you this is near perfect, I would just add back a short sentence to say that the previous version was underperforming when using multiple threads, few words would be enough then all good to me! |
|
Thanks for the feedback! I've restored the brief note about the previous implementation's underperformance with multiple threads in All applicable GitHub Actions and CircleCI checks on the new commit have passed. The wheels job was skipped as expected by its workflow condition for this documentation-only change. Everything is ready for your final review. Thank you! |
baptiste-genest
left a comment
baptiste-genest
left a comment
There was a problem hiding this comment.
Looks good to me! Thank you for your time.
…s-plan-count test/test_bspot.py conflicted because both branches append after the last pre-existing test. Kept both sets: PythonOT#898's seeding/reproducibility tests and this branch's merge_bijections tests.
Why
ot.bsp.compute_bspot_bijection(..., seed=...)— and thereforerandom_stateofot.solve_sample(..., method="bsp")— is documented as a reproducibility knob, but it only held for thefirst call made on a thread. Reported with measurements in #897.
engis a thread-local object whose constructor consumesglobal_seedthe first timerng()is used onthat thread and never again, so on later calls
global_seed = seedcannot reach it and every drawcontinues the previous stream. Three identical calls in one process returned three different bijections
and three different costs (0.786970722 / 0.786334761 / 0.793428761 on the instance in #897).
The same lines also cost solution quality: the candidate matchings are produced by
#pragma omp parallel for(ot/bsp/bsp_wrapper.cpp:40-48) and each worker builds its engine from thesame
global_seed, so with the default static schedule the iterations at equal positions in theirchunks consume identical subsequences and return identical matchings. Measured,
n_plans=64:64 distinct candidates at 1 thread, 32 at 2, 16 at 4, 8 at 8, 4 at 16 — i.e.
ceil(n_plans / n_threads)distinct plans, the rest being copies that
MergePlansNoParthen walks over again.What
Both changes are inside the loop that generates the candidate plans, in
ot/bsp/bsp_wrapper.cpp:thread_local) engine at the start of each iteration with a value derived from(user_seed, plan index), and passseeddown as a parameter instead of reading the mutable global;BijectiveBSPMatchingper iteration, so the loop body no longer writes state sharedwith other iterations (
random_pivotwas written by every thread of the team).The derivation must not be additive: with
user_seed + task, plantof seedsis plant-1of seeds+1, so changing the seed would leave nearly the whole candidate set unchanged — hence the SplitMix64finalizer. The vendored amalgamated header, the Cython layer, every default and the solver behaviour on
any other input are untouched; the only change in the Python layer is the
seeddocstring.Contract
For a fixed build,
compute_bspot_bijection(X, Y, n_plans, p, initial_perm, gaussian_slicing, seed)isnow a function of its arguments: repeat-equality, equality after unrelated calls, equality with a first
call in a fresh interpreter, and equality across
OMP_NUM_THREADS. Candidate planidepends on(seed, i)alone, which is what the new "seed and index" test checks by comparing the first rows of thestacks returned for
n_plans=4andn_plans=37.Two deliberate non-claims. Results are reproducible per build, not across standard libraries:
std::mt19937::seedis specified by the standard, butstd::normal_distributionandstd::uniform_int_distributionare implementation-specified, so a guarantee across libstdc++/libc++/MSVCwould need self-contained distributions. And different seeds are not required to give different
bijections. For the same reason the tests do not assert that the candidate matchings are pairwise
distinct — independent streams can legitimately return the same bijection (for
Y = 2X + tevery BSPcandidate is the identity, and on a 20-point instance I measured 120 different seeds producing only 114
distinct matchings). Diversity of the candidates is the consequence that #897 reports as a quality
metric (4/64 → 64/64 here), not something a test should promise.
For a given seed the returned permutation does change; unavoidable, since the old sequence depended on
the call history and on the CPU count, so there was no stable behaviour to keep. Existing assertions on
fixed configurations (
test_bsp_ot_exact_identity,test_bsp_ot_identity_null_cost,test_bsp_ot_bijective,test_bsp_ot_torch_backend) hold for any stream and still pass.Added cost: one
mt19937state initialisation (624 words) per candidate plan, i.e.O(1)innagainst the
O(n log n)work of generating that plan.Tests
Eight tests added to
test/test_bspot.py; no historical cost or permutation is hardcoded, so they do notfreeze the new random sequence. Two of them spawn child interpreters (cross-process reproducibility and
invariance to
OMP_NUM_THREADSis otherwise untestable); the child also reports the SHA-256 of theextension it loaded, and the parent asserts it matches its own, so a child importing a different build
cannot silently pass.
Measured on Windows 11, MSVC 14.44 with
/openmp, 16 logical CPUs, Python 3.13.1, numpy 2.4.6,torch 2.10.0+cpu, jax 0.12.0, built with
python setup.py build_ext --inplace. The first three bulletsand the lint check were re-run at
fd4706b; the contract sweep is the measurement taken earlier at5cb82c9:test pass on master as well, which is correct — they are not the defect);
OMP_NUM_THREADS=1, 2, 4, 8, 16;pytest test/test_solvers.py -k bsp: 6 passed;n_plans1–256, bothslicing modes, several seeds; each also compared against the exact
ot.emdvalue) checksrepeat-equality, history-equality, permutation structure, and consistency between the cost the solver
reports for its own permutation and the cost of that permutation: 39 configurations pass, 0 fail.
The same script against the master build fails every one of the 39.
ruffv0.5.2 of.pre-commit-config.yamlthe changed test file reports nofindings, on this branch and on
HEAD; a newer localruff(0.16.3) reports the same 5 pre-existingfindings on both, so there is no new lint under either version.
The
seeddocstring warns that reusing a seed can stall a loop that refines throughinitial_perm,and that is measured rather than inferred: on a 120-point instance, six rounds at
n_plans=32with afixed seed freeze after the second round (
0.184978four rounds running) while changing the seed eachround keeps improving (
0.198502 → 0.191056, one round of five unchanged). Not added as a test: it isinstance-dependent, whereas the determinism the tests do assert is what makes the stall possible.
Known limitation, stated rather than hidden: on a build or machine where OpenMP is unavailable (POT's
setup.pydrops the flag silently) or pinned to one thread, the thread-count test cannot distinguishthe old behaviour from the new one; and a change that simply deleted
#pragma omp parallel forwouldalso pass these tests — staying parallel is a performance property, not something these assertions
cover.
Out of scope
n_plans=0crashesMergePlansNoPar(it indexes an empty plan vector), andmerge_bijectionsreadsplans.shape[1]as the number of plans, which is an out-of-bounds read whenn_plans < n_samplesandsilently drops the plans at index
>= n_samplesotherwise. The second one is now reported separately as#910, with an AddressSanitizer trace on the merge loop; neither is mixed into this change.
Fixes #897