fix: honor explicit seed=0, un-dead the distributed resync branch, exception-safe _deterministic_seed#1917
fix: honor explicit seed=0, un-dead the distributed resync branch, exception-safe _deterministic_seed#1917arham766 wants to merge 1 commit into
Conversation
…ception-safe _deterministic_seed Three related defects in the shared RNG helper: - is_manual was computed AFTER seed was reassigned by dist.broadcast(seed or getrandbits(64)), so it was always True and the resync branch for auto-seeded generators created before process-group init was dead code - distributed ranks could sample divergently (e.g. AutoNAS RandomSearcher subnets). - The same falsy check silently replaced an explicit seed=0 with random bits. - _deterministic_seed yielded without try/finally, leaking the seed-1024 generator (and the is_manual/is_synced flags) if the body raised, e.g. a failing sample callback in nas algorithms; the restore now also covers both flags, matching the docstring's 'resets the random state to prior upon exit'. Regression tests live in a fix-scoped file; the fuller suite for this module is proposed separately in NVIDIA#1916. Part of the findings in NVIDIA#1902 (wave 2). Signed-off-by: arham766 <arhamislam766@yahoo.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR updates ChangesRandom Generator Seed Handling
Estimated code review effort: 2 (Simple) | ~12 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant DeterministicSeedContext
participant GetGenerator
Caller->>DeterministicSeedContext: enter context(seed)
DeterministicSeedContext->>GetGenerator: capture prior generator, is_manual, is_synced
DeterministicSeedContext->>GetGenerator: set deterministic seed
Caller->>Caller: execute context body
DeterministicSeedContext->>GetGenerator: restore prior generator, is_manual, is_synced
Related PRs: None specified. Suggested labels: bug, tests Suggested reviewers: None specified. 🐰 A seed once lost is found again, 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
What does this PR do?
Type of change: Bug fix
Three related defects in modelopt/torch/utils/random.py (found writing #1916, confirmed by two reviews): is_manual was computed after the seed was reassigned, so it was ALWAYS True and the distributed resync branch for auto-seeded generators was dead code (ranks could sample divergent AutoNAS subnets); the same falsy check silently replaced an explicit seed=0 with random bits; and _deterministic_seed leaked the seed-1024 generator (and both flags) if the body raised. Regression tests are in a fix-scoped file (test_random_seed.py) so they don't collide with #1916's fuller suite; each was verified to fail pre-fix with the exact symptom (seed-0 stream divergence, is_manual True, leaked generator).
Usage
N/A
Testing
Regression tests included, each verified to FAIL with the fix reverted. Combined battery with all sibling wave fixes: 381 passed across tests/unit/torch/opt, tests/unit/torch/utils, tests/unit/recipe, and quantization test_mode. Note: the unmerged suite PRs #1913-#1916 contain behavior-documenting NOTE tests pinning the OLD behavior fixed here — whichever lands second gets the NOTE tests flipped (happy to rebase either way).
Before your PR is "Ready for review"
Additional Information
Issue: #1902 (wave-2 findings)
Summary by CodeRabbit
Bug Fixes
0.Tests