Skip to content

Comparator fix for weakref - #1411

Merged
KRRT7 merged 5 commits into
mainfrom
comparator-nn-module
Feb 11, 2026
Merged

Comparator fix for weakref#1411
KRRT7 merged 5 commits into
mainfrom
comparator-nn-module

Conversation

@aseembits93

Copy link
Copy Markdown
Contributor

Some extra tests to ensure more torch datatypes are supported but no changes in the comparator

@claude

claude Bot commented Feb 6, 2026

Copy link
Copy Markdown
Contributor

PR Review Summary

Prek Checks

✅ All checks pass (ruff check and ruff format both passed).

Mypy

⚠️ 426 mypy errors found in the two changed files, but these are all pre-existing (missing return type annotations in test file, no-any-return errors in comparator.py). No new type errors introduced by this PR.

Code Review

✅ No critical issues found in the latest changes.

Changes reviewed:

  • codeflash/verification/comparator.py: Adds weakref import and handler for weakref.ref objects (lines 175-184). Adds type annotation for superset_obj parameter.
  • tests/test_comparator.py: Adds comprehensive tests for weakref handling (3 test functions) and torch.nn module comparisons (16 test functions).

Analysis:

  • The weakref handler correctly dereferences both refs and compares referents recursively
  • Dead ref handling (both dead = equal, one dead = not equal) is correct
  • The type guard at line 127-132 ensures new is also a weakref.ref when orig is one
  • Potential infinite recursion from circular weakrefs is caught by the existing RecursionError handler at line 528
  • torch.nn tests properly use pytest.skip() when torch is not available

Test Coverage

File Stmts (main) Stmts (PR) Coverage (main) Coverage (PR) Change
codeflash/verification/comparator.py 332 341 61% 62% +1% ✅
  • New weakref lines (175-184): ✅ Fully covered by tests
  • Overall: Coverage improved slightly (+1%), no regressions
  • Skipped tests: 42 skipped (vs 19 on main) — the new torch.nn tests are skipped when torch is not installed in CI, which is expected

Last updated: 2026-02-11

@KRRT7
KRRT7 merged commit ce67f09 into main Feb 11, 2026
25 of 27 checks passed
@KRRT7
KRRT7 deleted the comparator-nn-module branch February 11, 2026 01:52
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.

2 participants