Skip to content

test: fix vacuous registry-cleanup assertion in test_mode_registry#1921

Closed
arham766 wants to merge 1 commit into
NVIDIA:mainfrom
arham766:fix/mode-registry-test-assert
Closed

test: fix vacuous registry-cleanup assertion in test_mode_registry#1921
arham766 wants to merge 1 commit into
NVIDIA:mainfrom
arham766:fix/mode-registry-test-assert

Conversation

@arham766

@arham766 arham766 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Type of change: Bug fix

Test-only fix (found writing #1915): test_mode_registry.py's final assertion compared the STRING 'test_registry' against registry OBJECTS in _all_registries — vacuously true regardless of behavior. It masked two real warts, now documented in-test: _ModeRegistryCls.del is unreachable (the class-level list holds a strong reference), and if a registry is removed from the list first, a later GC-triggered del raises ValueError from its unconditional list.remove (surfaced as PytestUnraisableExceptionWarning). The test now asserts the removed mode no longer resolves, cleans the registry from _all_registries explicitly, and checks residue by name — mutation-verified non-vacuous. The del warts may deserve a small source fix; happy to follow up.

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"

  • Is this change backward compatible?: ✅
  • If you copied code from any other sources or added a new PIP dependency, did you follow guidance in CONTRIBUTING.md: N/A
  • Did you write any new necessary tests?: ✅
  • Did you update Changelog?: N/A
  • Did you get Claude approval on this PR?: N/A (external contributor)

Additional Information

Issue: #1902 (wave-2 findings)

Summary by CodeRabbit

  • Tests
    • Improved coverage for mode registration and removal behavior.
    • Verified removed modes are no longer available and missing lookups fail as expected.
    • Strengthened test cleanup to prevent leftover state between runs.

The final assertion compared the string 'test_registry' against
registry OBJECTS in _ModeRegistryCls._all_registries, so it passed
vacuously regardless of behavior. It also masked two real warts, now
documented in the test: __del__ is unreachable because _all_registries
holds a strong reference (del never drops the refcount to zero), and
if the registry is removed from the list first, a later GC-triggered
__del__ raises ValueError from its unconditional list.remove.

The test now asserts the removed mode no longer resolves via any
registry, removes the registry from _all_registries explicitly
(neutralizing __del__ for the deterministic destruction), and checks
residue by name - verified non-vacuous by mutation (skipping the
removal makes it fail).

Part of the findings in NVIDIA#1902 (wave 2).

Signed-off-by: arham766 <arhamislam766@yahoo.com>
@arham766 arham766 requested a review from a team as a code owner July 6, 2026 01:49
@copy-pr-bot

copy-pr-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 39bfe226-18b3-4deb-af1a-eb6b95e6edbd

📥 Commits

Reviewing files that changed from the base of the PR and between b96a785 and 799c0bd.

📒 Files selected for processing (1)
  • tests/unit/torch/opt/test_mode_registry.py

📝 Walkthrough

Walkthrough

A test file for mode registry functionality was updated to import mock and pytest, add assertions verifying that a removed mode is no longer resolvable and raises KeyError, and rework test cleanup to explicitly remove registry instances and patch __del__ to avoid errors.

Changes

Test enhancement for mode registry

Layer / File(s) Summary
Imports and strengthened test assertions/cleanup
tests/unit/torch/opt/test_mode_registry.py
Adds mock and pytest imports; asserts removed mode is unresolvable via contained_in_any and that get_from_any raises KeyError; replaces del registry with explicit removal from _all_registries and patches __del__ to prevent cleanup exceptions.

Estimated code review effort: 1 (Trivial) | ~5 minutes

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the test-only fix to the vacuous registry-cleanup assertion in test_mode_registry.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Security Anti-Patterns ✅ Passed Only a unit test file changed; it contains no banned patterns like torch.load(weights_only=False), allow_pickle=True, trust_remote_code=True, eval/exec, or nosec.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@arham766

arham766 commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

Consolidated into #1927 per maintainer feedback in #1902 — the suite was trimmed to only the lines codecov reports uncovered, with parametrization clusters deduplicated. Closing in favor of that PR.

@arham766 arham766 closed this Jul 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant