Seed the neighbor-sampling RNG per rank instead of leaving it unset - #730
Draft
kmontemayor2-sc wants to merge 3 commits into
Draft
Seed the neighbor-sampling RNG per rank instead of leaving it unset#730kmontemayor2-sc wants to merge 3 commits into
kmontemayor2-sc wants to merge 3 commits into
Conversation
`create_sampling_config` hardcoded `seed=None` into GLT's `SamplingConfig`, so `NeighborSampler` never called `RandomSeedManager::setSeed` -- the only `setSeed` call site in GLT -- and `is_set` stayed false for the whole run. That is not just a determinism choice. `CPURandomSampler::UniformSample` reads `RandomSeedManager::getInstance().getSeed()` on every call, once per source row of a batch, and discards the result because the `std::mt19937` it feeds is `thread_local static` and already constructed. Unseeded, `getSeed()` constructs a `std::random_device` and draws from it, roughly 5 us of real work per source row for a value that is thrown away. Measured on an unmodified GLT build, setting any seed took the sampler call from ~1450 us to ~50 us (~29x) with byte-identical neighbor output. In a multi-node GPU inference workload it took per-batch data loading from 47-184 ms to 21-25 ms and left recall unchanged. The seed is derived from the global rank so ranks do not draw identical samples, mixed with a run-level base that defaults to wall-clock time so successive runs still vary as unseeded sampling did. The derived seed is logged with its base so a run stays reproducible after the fact. `crc32` is used rather than `hash()`, whose string hashing is randomized per process, and its range matches `setSeed(unsigned int)` exactly so no masking is needed. Callers may still pass an explicit seed, which is used verbatim.
Point the GLT source references at permalinks pinned to the commit `gigl/scripts/install_glt.sh` installs, so they stay valid as upstream moves. Line numbers verified against that commit. Trim the measured numbers down to the headline result, drop the note about `crc32` already matching `setSeed(unsigned int)`, and add a TODO to remove the workaround if upstream hoists the `getSeed()` call into the engine initializer so the unseeded path stops paying per-row entropy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The per-rank crc32 derivation existed to make the seed reproducible from a single recorded base. That reproducibility is not wanted, and paying for it cost more than it was worth: the seed had to be built outside create_sampling_config so the caller could supply a rank, which meant both DistNeighborLoader and DistABLPLoader had to change, and a public derive_sampling_seed had to exist purely as plumbing. Drawing a random uint32 inside create_sampling_config gets the same properties for the parts that matter. Any seed is enough for the performance fix, since only RandomSeedManager::is_set is load-bearing, not the value. Distinctness across ranks still holds because each rank builds its own loader and so draws its own seed; collisions are ~1e-6 at fleet scale and are harmless anyway, as two ranks sharing a seed still sample different source nodes. It is also better in one case the derivation handled badly: two loaders in the same process previously got identical seeds, since rank and wall-clock second were both the same. Callers are untouched, and the explanation of why a seed is set at all now lives on create_sampling_config, where someone reading the seed argument will find it.
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.
create_sampling_confighardcodedseed=Noneinto GLT'sSamplingConfig, soNeighborSamplernever calledRandomSeedManager::setSeed-- the onlysetSeedcall site in GLT -- andis_setstayed false for the whole run.That is not just a determinism choice.
CPURandomSampler::UniformSamplereadsRandomSeedManager::getInstance().getSeed()on every call, once per source row of a batch, and discards the result because thestd::mt19937it feeds isthread_local staticand already constructed. Unseeded,getSeed()constructs astd::random_deviceand draws from it, roughly 5 us of real work per source row for a value that is thrown away.Measured on an unmodified GLT build, setting any seed took the sampler call from ~1450 us to ~50 us (~29x) with byte-identical neighbor output. In a multi-node GPU inference workload it took per-batch data loading from 47-184 ms to 21-25 ms and left recall unchanged.
The seed is derived from the global rank so ranks do not draw identical samples, mixed with a run-level base that defaults to wall-clock time so successive runs still vary as unseeded sampling did. The derived seed is logged with its base so a run stays reproducible after the fact.
crc32is used rather thanhash(), whose string hashing is randomized per process, and its range matchessetSeed(unsigned int)exactly so no masking is needed.Callers may still pass an explicit seed, which is used verbatim.