🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21003

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 76d8597 with merge base 0f02e85 ():
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

This PR needs a release notes: label

If your change should be included in the release notes (i.e. would users of this library care about this change?), please use a label starting with release notes:. This helps us keep track and include your important work in the next release notes.

To add a label, you can comment to pytorchbot, for example
@pytorchbot label "release notes: none"

For more information, see
https://github.com/pytorch/pytorch/wiki/PyTorch-AutoLabel-Bot#why-categorize-for-release-notes-and-how-does-it-work.

@claude review

Claude finished @psiddh's task in 5m 36s —— View job

PR Review: Pytest Refactor - Passes

• Gather context (read changed files)
• Review pass changes (_passes/)
• Review new test framework (tests/rework/passes/)
• Post review feedback

Reviewed the pass simplifications and the new pytest-based pass-testing framework. Overall this is a clean, well-structured refactor — the fixture plumbing (enumerate_backends / repack_pass_fixtures / unpack_pass_fixtures) and the Assertions / PassPipeline helpers are nicely factored, and the pass changes mostly remove now-redundant edge-dialect handling as passes move to the pre-edge pipelines. CI is green. A few things worth addressing below.

Correctness / robustness

1. decompose_any.py — substring regex can false-match

if re.search(r"any\.(dim|default)", str(node.target)):

re.search matches anywhere in the string, so a target whose stringified name contains any.dim / any.default (e.g. a hypothetical ...many.dim...) would match unintentionally. This substring risk pre-existed with "any.dim" in str(...), but note that decompose_expm1.py in this same PR does the clean thing — matching against an explicit op-overload set:

if node.target in {torch.ops.aten.special_expm1.default, torch.ops.aten.expm1.default}:

Recommend the same set-membership approach here (torch.ops.aten.any.dim, torch.ops.aten.any.default) for consistency and safety. Fix this →

2. decompose_maxpool3d.py — return_indices detection

return_indices = (
    any(user.args[1] == 1 for user in node.users)
    if len(node.meta["val"]) > 1
    else False
)

The new user-traversal approach (detecting whether the indices output is actually consumed) is a real improvement over trusting node.args[6], and the len(node.meta["val"]) > 1 guard correctly excludes the single-output variant. One assumption to flag: every user of a tuple-output node is expected to be a getitem with an index in args[1]. That holds for the standard max_pool3d_with_indices lowering, so it's fine in practice, but a stray non-getitem user would raise IndexError. Worth a defensive user.op == "call_function" / target check or a short comment locking in the getitem assumption.

Also note the changed return → continue: previously the pass bailed on the first unsupported node, now it correctly skips just that node and keeps processing the rest. Good catch — that's a latent bug fix.

Nits

3. Stale comments in DecomposeVar test (src/pattern.py)
This PR removed the create_const_node / const_cache scale-node logic from decompose_var.py (scale is now a plain Python float). The test comments still describe the old behavior:

• L3332: "adds mul + get_attr scale node"
• L3410: "mul×2 (diff² + scale) + get_attr scale const"

The assertions themselves are correct (they only count mul), but the comments now describe a get_attr node that no longer exists. Recommend updating them to avoid confusing future readers.

4. Dead assertion in PassPipeline.lower_edge_ep (passes_helper.py:253)

edge_passes = PassPipeline._slice_to_target(...)
assert edge_passes is not None, f"{target_pass.__name__} not found in edge passes"

_slice_to_target already asserts internally and never returns None, so this follow-up assert is unreachable. Minor cleanup.

5. default_property() in conftest.py
Defining the Property dataclass inside the function and returning a fresh instance on every call is a slightly unusual pattern. A module-level dataclass would be cleaner and cheaper, but this is cosmetic.

Questions

• The removal of DecomposeCDist, DecomposeFill, and DecomposeVar from get_default_pass_activations() and the dependency table (qnn_pass_manager.py) — these now run only in the annotation/export pipelines rather than the to-edge pipeline. Can you confirm this is intentional and that no downstream path still relied on them firing during to-edge lowering? CI green is reassuring, but a one-line note in the PR description on why these moved would help reviewers.

Nice work overall — the test framework is a solid foundation and the per-pass pattern coverage (backend enumeration, quantized vs fp16, subtests for op variants) is thorough.
· branch dev1/winskuo/test_framework_refactor_passes