test: targeted unit tests for codecov-uncovered lines across eight modules#1927
Open
arham766 wants to merge 1 commit into
Open
test: targeted unit tests for codecov-uncovered lines across eight modules#1927arham766 wants to merge 1 commit into
arham766 wants to merge 1 commit into
Conversation
…dules Consolidates the standalone test-suite PRs into a single PR per maintainer feedback in NVIDIA#1902: every test here exercises lines the codecov report marks uncovered, asserts meaningful logic rather than coverage for its own sake, and parametrization clusters are deduplicated. Modules targeted (codecov coverage before): - torch/export/postprocess.py (26.6%): TP/PP split layouts, padding math, quantization-scale resharding - torch/utils/graph.py (26.4%): subgraph matching, memoization, arity/connectivity mismatches - torch/quantization/calib/bias.py (70.0%): method dispatch, running averages, bias round-trips, reset - torch/utils/logging.py (74.3%): tqdm silencing, tty ANSI wrapping, capture_io, warning filters - torch/utils/distributed.py (81.4%): FileLock wait loop, master_only, rank fallbacks - torch/distill/losses.py (81.9%): MGD forward math, MFT threshold branch - torch/quantization/conversion.py (87.0%): SVDQuant entrypoint, restore error paths, SequentialQuantizer edge branches, context manager module-type restoration - torch/utils/robust_json.py (87.0%): encoder special-type fallbacks Also folds in the test_mode_registry assertion fix (was NVIDIA#1921). Lines that remain uncovered need multi-rank process groups or are unreachable defensive branches; they are enumerated in the PR body. Signed-off-by: arham766 <arhamislam766@yahoo.com>
This was referenced Jul 6, 2026
Closed
Contributor
📝 WalkthroughWalkthroughThis PR adds new unit test modules and expands existing tests covering distillation losses (MFTLoss/MGDLoss), export postprocessing (TP/PP splitting, padding, LM-head quantization), mode registry cleanup, quantization bias calibration and conversion, and torch utils (distributed, graph matching, logging, robust JSON). No non-test source files are modified. ChangesTest Coverage Additions
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Suggested reviewers: 🚥 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/unit/torch/opt/test_mode_registry.py (1)
54-70: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCleanup won't run if an earlier assertion fails, leaking
registryinto_all_registriesfor other tests.If the assertions on Lines 55-57 fail, the explicit removal on Line 66 and the
__del__patch on Lines 67-68 never execute, leavingregistryin_ModeRegistryCls._all_registriesfor the rest of the test session — exactly the residue this code is trying to prevent. Wrapping the mode-registration/removal logic intry/finallywould guarantee cleanup runs regardless of assertion outcome.♻️ Proposed fix using try/finally
- registry.remove_mode("a_test_mode") - - # after removal, the mode must no longer resolve via any registry - assert not _ModeRegistryCls.contained_in_any("a_test_mode") - with pytest.raises(KeyError, match="a_test_mode"): - _ModeRegistryCls.get_from_any("a_test_mode") - - # NOTE: `del registry` alone does NOT remove the registry from - # `_ModeRegistryCls._all_registries`: the class-level list holds a strong reference, so the - # local `del` never drops the refcount to zero and `__del__` (which would remove it from the - # list) is unreachable. Remove it explicitly so this test leaves no residue behind for other - # tests sharing the process. Since `__del__` unconditionally calls `list.remove`, it would - # then raise ValueError once the orphaned object is garbage collected, so neutralize it for - # the actual destruction. - _ModeRegistryCls._all_registries.remove(registry) - with mock.patch.object(_ModeRegistryCls, "__del__", return_value=None): - del registry # refcount now drops to zero -> patched `__del__` runs here - - assert all(r._registry_name != "test_registry" for r in _ModeRegistryCls._all_registries) + try: + registry.remove_mode("a_test_mode") + + # after removal, the mode must no longer resolve via any registry + assert not _ModeRegistryCls.contained_in_any("a_test_mode") + with pytest.raises(KeyError, match="a_test_mode"): + _ModeRegistryCls.get_from_any("a_test_mode") + finally: + # NOTE: `del registry` alone does NOT remove the registry from + # `_ModeRegistryCls._all_registries`: the class-level list holds a strong reference, so the + # local `del` never drops the refcount to zero and `__del__` (which would remove it from the + # list) is unreachable. Remove it explicitly so this test leaves no residue behind for other + # tests sharing the process. Since `__del__` unconditionally calls `list.remove`, it would + # then raise ValueError once the orphaned object is garbage collected, so neutralize it for + # the actual destruction. + _ModeRegistryCls._all_registries.remove(registry) + with mock.patch.object(_ModeRegistryCls, "__del__", return_value=None): + del registry # refcount now drops to zero -> patched `__del__` runs here + + assert all(r._registry_name != "test_registry" for r in _ModeRegistryCls._all_registries)🤖 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/opt/test_mode_registry.py` around lines 54 - 70, The cleanup in test_mode_registry relies on assertions completing first, so a failure before the explicit removal can leave `registry` behind in `_ModeRegistryCls._all_registries`. Wrap the registration/assertion block around the `registry` lifecycle in a `try/finally` so the cleanup always runs, and keep the explicit `_all_registries.remove(registry)` plus the `mock.patch.object(_ModeRegistryCls, "__del__", return_value=None)` teardown in the `finally` block to ensure no residue remains for later tests.tests/unit/torch/utils/test_distributed.py (1)
74-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTimer-based synchronization risks flakiness under CI load.
This test relies on a real
threading.Timer(0.2, ...)racing against a polling loop (poll_time=0.02) inside thewithblock. Under a slow/contended CI runner, the timer thread could be delayed enough for the pollingFileLockto time out (if it has an internal timeout) or the test to hang longer than expected, since real wall-clock timing is used rather than a deterministic signal.
[recommended_refactor_reason]🤖 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_distributed.py` around lines 74 - 86, The test test_filelock_all_acquire_waits_for_release is flaky because it depends on real wall-clock timing via threading.Timer and a polling FileLock acquire loop. Refactor it to use deterministic synchronization in the test, such as an Event or a mocked release trigger, so the first FileLock is released only after the second acquire attempt is definitely waiting. Keep the assertions around dist.FileLock, try_acquire, and the all_acquire context, but remove reliance on timing-sensitive poll_time behavior.tests/unit/torch/export/test_postprocess.py (1)
552-556: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReliance on private
Tensor._is_view()API.
_is_view()is an internal/private PyTorch tensor method (underscore-prefixed), not part of the documented public API. It could change or be removed in a future torch release. A more stable check is verifyingtensor._base is not Noneis avoided too (also private) — consider comparingdata_ptr()/storage().data_ptr()equality with the base tensor, or checkingtensor.is_contiguous()/shape post-clone, to avoid depending on a private method.♻️ Alternative using storage identity instead of private API
def test_postprocess_tensors_clones_views(): base = torch.randn(4, 4) view = base[:2] - assert view._is_view() + assert view.storage().data_ptr() == base.storage().data_ptr() weights = {"a.weight": view} postprocess_tensors(weights, torch.float32) - assert not weights["a.weight"]._is_view() + assert weights["a.weight"].storage().data_ptr() != base.storage().data_ptr() assert torch.equal(weights["a.weight"], base[:2])🤖 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/export/test_postprocess.py` around lines 552 - 556, Replace the private Tensor._is_view() assertions in test_postprocess with a public, stable check in the postprocess_tensors test case. Use the existing view/base tensors in the test to verify aliasing via storage or data pointer identity before and after postprocess_tensors, and keep the final equality assertion against base[:2] to confirm the cloned tensor preserves values.
🤖 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/export/test_postprocess.py`:
- Around line 552-556: Replace the private Tensor._is_view() assertions in
test_postprocess with a public, stable check in the postprocess_tensors test
case. Use the existing view/base tensors in the test to verify aliasing via
storage or data pointer identity before and after postprocess_tensors, and keep
the final equality assertion against base[:2] to confirm the cloned tensor
preserves values.
In `@tests/unit/torch/opt/test_mode_registry.py`:
- Around line 54-70: The cleanup in test_mode_registry relies on assertions
completing first, so a failure before the explicit removal can leave `registry`
behind in `_ModeRegistryCls._all_registries`. Wrap the registration/assertion
block around the `registry` lifecycle in a `try/finally` so the cleanup always
runs, and keep the explicit `_all_registries.remove(registry)` plus the
`mock.patch.object(_ModeRegistryCls, "__del__", return_value=None)` teardown in
the `finally` block to ensure no residue remains for later tests.
In `@tests/unit/torch/utils/test_distributed.py`:
- Around line 74-86: The test test_filelock_all_acquire_waits_for_release is
flaky because it depends on real wall-clock timing via threading.Timer and a
polling FileLock acquire loop. Refactor it to use deterministic synchronization
in the test, such as an Event or a mocked release trigger, so the first FileLock
is released only after the second acquire attempt is definitely waiting. Keep
the assertions around dist.FileLock, try_acquire, and the all_acquire context,
but remove reliance on timing-sensitive poll_time behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 96c3e069-5874-4268-bdec-d9e8ae1badc9
📒 Files selected for processing (9)
tests/unit/torch/distill/test_losses.pytests/unit/torch/export/test_postprocess.pytests/unit/torch/opt/test_mode_registry.pytests/unit/torch/quantization/test_bias_calib.pytests/unit/torch/quantization/test_conversion.pytests/unit/torch/utils/test_distributed.pytests/unit/torch/utils/test_graph.pytests/unit/torch/utils/test_logging.pytests/unit/torch/utils/test_robust_json.py
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.
What does this PR do?
Type of change: New tests
Overview: Consolidates all open test-suite PRs into a single targeted PR, per maintainer feedback in #1902:
Every test in this PR exercises lines the codecov report marks uncovered on
main; suites for modules already >=90% covered by GPU/e2e CI were dropped entirely, and the rest were trimmed and deduplicated hard (parametrization clusters merged, redundant variants removed). Result: 95 tests, ~1.4k lines, <1s runtime (down from 914 tests / ~9.4k lines across the original PRs).torch/export/postprocess.pytorch/utils/graph.pytorch/quantization/calib/bias.pytorch/utils/logging.pytorch/utils/distributed.pytorch/distill/losses.pytorch/quantization/conversion.pytorch/utils/robust_json.pyLines deliberately left uncovered, so nothing is fake-covered:
postprocess.py297-471, 488-542, 577-623: the TP/PP shared-memory merge paths require a real multi-rank process group (get_tensors_parallelassertslen(ranks) > 1) — GPU/multi-process CI territory.distributed.py165-166: CUDA allgather padding branch, needsworld_size > 1.logging.py87: the positional-arg branch of the tqdm silencer overwrites tqdm'sasciiargument instead ofdisable(off-by-one in the positional index), so covering it would pin broken behavior. Happy to fix in a follow-up.conversion.py303 androbust_json.py65: unreachable defensive branches (pydantic coerces dict cfgs before the isinstance check; the__class__.__module__fallback accepts every real object).Also folds in the
test_mode_registry.pyassertion fix (was #1921).Replaces (closing in favor of this PR): #1903, #1904, #1905, #1906, #1907, #1913, #1914, #1915, #1916, #1921, #1922, #1923, #1924, #1925.
The open bug-fix PRs (#1908-#1912, #1917-#1920) are unaffected — their regression tests live in existing test files. Two tests here pin current arguably-buggy behavior with
NOTE:comments (postprocess odd-row padding mutating the input config; MGD loss not detaching teacher features, see #1926) and will be flipped if/when fixes land.Testing
Each file was also mutation-checked (seeded logic breaks in the target module -> the intended test fails, source reverted).
Before your PR is "Ready for review"
Additional Information
Issue: #1902
Summary by CodeRabbit