test: unit suites for seven untested torch/utils modules (109 tests)#1916
test: unit suites for seven untested torch/utils modules (109 tests)#1916arham766 wants to merge 2 commits into
Conversation
Hermetic CPU-only suites for random, robust_json, list, graph, import_utils, logging, and the single-process contracts of distributed (process-group paths excluded honestly). Adversarially reviewed: 18 expectations re-derived from source, 8 seeded mutations killed, double-run and interleaved-order runs clean (no global-state leakage; the generator-cache fixture clears module state around every test). Documents two genuine defects with NOTE tests in random.py: the is_manual flag is computed after the seed is reassigned and is therefore always True (the distributed re-sync branch is dead code, and an explicit seed=0 is silently replaced), and _deterministic_seed does not restore the previous generator if the body raises. Part of the coverage initiative in NVIDIA#1902. Signed-off-by: arham766 <arhamislam766@yahoo.com>
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds seven new pytest unit test modules under tests/unit/torch/utils/, covering modelopt.torch.utils.distributed, graph.match, import_utils, list, logging, random, and robust_json. No production code or public API declarations are modified. ChangesNew torch.utils unit test suites
Estimated code review effort: 2 (Simple) | ~15 minutes Related PRs: None identified. Suggested labels: tests Suggested reviewers: None identified. Seven new suites of tests take flight, Each module checked, each edge case tight — Distributed, graph, and random seeds, JSON and logs meet all their needs. A rabbit hops through green and gold, Nodding at coverage, well and bold. 🐇✅🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/unit/torch/utils/test_import_utils.py (1)
36-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated "no warning raised" boilerplate.
The
with warnings.catch_warnings(): warnings.simplefilter("error")pattern is duplicated across five tests (Lines 37-38, 59-60, 65-66, 77-78, 83-84). Could be consolidated into a small fixture or context-manager helper for readability, but it's not blocking.♻️ Optional helper extraction
+import contextlib + + +@contextlib.contextmanager +def assert_no_warnings(): + with warnings.catch_warnings(): + warnings.simplefilter("error") + yield + + def test_no_error_and_no_success_msg_is_silent(): - with warnings.catch_warnings(): - warnings.simplefilter("error") # any warning would fail the test + with assert_no_warnings(): with import_plugin("foo"): pass🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/torch/utils/test_import_utils.py` around lines 36 - 85, Several tests repeat the same “treat warnings as errors” setup, so please extract that duplicated warnings-catching pattern into a small reusable fixture or context manager and use it across the affected tests. Update the tests around import_plugin, _import_missing_module, and _raise_runtime_error to rely on the shared helper instead of repeating warnings.catch_warnings() and warnings.simplefilter("error"), keeping the existing assertions and behavior unchanged.tests/unit/torch/utils/test_random.py (2)
127-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding an assertion that pins the "no restore on exception" defect.
The comment documents that
_deterministic_seedlacks atry/finally, so state isn't restored if the body raises, but only the happy path is exercised. Adding a small test that raises inside the context manager and asserts the outer stream is left corrupted would turn this into an actual regression pin rather than just a code comment.♻️ Suggested addition
def test_deterministic_seed_context_leaks_state_on_exception(): # NOTE: documents the defect — `_deterministic_seed` has no try/finally, so an # exception inside the `with` block leaves the inner (seed=1024) generator active. _set_deterministic_seed(99) with pytest.raises(RuntimeError): with _deterministic_seed(): raise RuntimeError("boom") assert mo_random.random() == _stdlib_random.Random(1024).random() # BUG: outer state not restored🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/torch/utils/test_random.py` around lines 127 - 140, Add a regression test for `_deterministic_seed` that exercises the exception path, since the current `test_deterministic_seed_context_uses_seed_1024_and_restores_state` only covers the happy path. In `test_random.py`, create a small test around `_deterministic_seed()` that raises inside the `with` block and then asserts `mo_random.random()` continues from the inner `Random(1024)` stream instead of the outer `Random(99)` stream, pinning the lack of restore on exception.
142-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider also documenting the auto-seed half of the
is_manualdefect.The comment explains the bug (
is_manualis alwaysTrueafter theseed = dist.broadcast(...)reassignment, even for auto-generated seeds), but the test only asserts the manual-seed case, which would pass regardless of whether the bug exists. Since the PR's stated goal is to pin these defects with NOTE tests for follow-up, consider adding a companion assertion for the auto-seed path so a future fix is caught by the test suite, not just the comment.♻️ Suggested addition
def test_is_manual_flag_after_seeding(): # NOTE: `_get_generator` computes `is_manual = seed is not None` *after* reassigning # `seed = dist.broadcast(seed or _random.getrandbits(64))`, so `is_manual` is True even # for auto-generated seeds. In a distributed run, an auto-seeded generator created before # process-group initialization would therefore never be re-synced. This test documents # the (correct) manual-seed half of that contract only. _set_deterministic_seed(13) assert _get_generator.is_manual is True + + +def test_is_manual_flag_incorrectly_true_after_auto_seeding(): + # NOTE: documents the defect — no manual seed was provided, yet `is_manual` ends up True. + _get_generator() # auto-seeded, no explicit seed + assert _get_generator.is_manual is True # BUG: should be False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/torch/utils/test_random.py` around lines 142 - 149, The test `test_is_manual_flag_after_seeding` only covers the manual-seed case, so it won’t fail if the `is_manual` bug remains in the auto-seed path; add a companion assertion that exercises the auto-generated seed behavior in `_get_generator` and verifies `is_manual` is false for that case. Keep the existing NOTE-style documentation, but make sure the test pins both sides of the contract so the `is_manual` regression is caught by the suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/unit/torch/utils/test_import_utils.py`:
- Around line 36-85: Several tests repeat the same “treat warnings as errors”
setup, so please extract that duplicated warnings-catching pattern into a small
reusable fixture or context manager and use it across the affected tests. Update
the tests around import_plugin, _import_missing_module, and _raise_runtime_error
to rely on the shared helper instead of repeating warnings.catch_warnings() and
warnings.simplefilter("error"), keeping the existing assertions and behavior
unchanged.
In `@tests/unit/torch/utils/test_random.py`:
- Around line 127-140: Add a regression test for `_deterministic_seed` that
exercises the exception path, since the current
`test_deterministic_seed_context_uses_seed_1024_and_restores_state` only covers
the happy path. In `test_random.py`, create a small test around
`_deterministic_seed()` that raises inside the `with` block and then asserts
`mo_random.random()` continues from the inner `Random(1024)` stream instead of
the outer `Random(99)` stream, pinning the lack of restore on exception.
- Around line 142-149: The test `test_is_manual_flag_after_seeding` only covers
the manual-seed case, so it won’t fail if the `is_manual` bug remains in the
auto-seed path; add a companion assertion that exercises the auto-generated seed
behavior in `_get_generator` and verifies `is_manual` is false for that case.
Keep the existing NOTE-style documentation, but make sure the test pins both
sides of the contract so the `is_manual` regression is caught by the suite.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5e8f75b9-36bd-470b-998c-372db18b741a
📒 Files selected for processing (7)
tests/unit/torch/utils/test_distributed.pytests/unit/torch/utils/test_graph.pytests/unit/torch/utils/test_import_utils.pytests/unit/torch/utils/test_list.pytests/unit/torch/utils/test_logging.pytests/unit/torch/utils/test_random.pytests/unit/torch/utils/test_robust_json.py
… manager Signed-off-by: arham766 <arhamislam766@yahoo.com>
arham766
force-pushed
the
tests/torch-utils-pure
branch
from
1208c26 to
cde86eb
Compare
1 participant
What does this PR do?
Type of change: new tests
Part of the unit-coverage initiative in #1902. Unit suites for seven previously-untested modelopt/torch/utils modules (109 tests, ~1s):
Two genuine defects documented with NOTE tests (fix offered as follow-up): in random.py,
is_manual = seed is not Noneis evaluated afterseed = dist.broadcast(seed or _random.getrandbits(64)), so it is ALWAYS True — the distributed re-sync branch for auto-seeded generators is dead code (ranks can sample divergent subnets), and an explicitseed=0is silently replaced by random bits; and_deterministic_seedlacks try/finally, leaking the seed-1024 generator if the body raises. (Also observed, report-only: a positional-args off-by-one in logging's _disable_tqdm, and a flatten_tree name-collision in _pytree that silently drops values — the latter module is owned by the existing test_pytree.py.)Usage
N/A — tests only.
Testing
228 passed for the whole tests/unit/torch/utils dir (22 pre-existing dataset skips). Adversarial review: 18 expectations independently re-derived, 8 mutations killed, dir run twice plus interleaved file orders — no order dependence or global-state leakage; Windows/Linux-safe.
Before your PR is "Ready for review"
Additional Information
Issue: #1902
Summary by CodeRabbit