[shoumikhin]

What is wrong today

Programs that go through constant_prop_pass can crash in two ways: one inside the pass, and one later when the program is lowered.

1. A folded value that is not a tensor. The pass also folds values such as the int from .item() on a buffer, and the bools that torch._check turns into runtime checks. When a node that stays in the graph reads such a value, the pass tries to turn it into a constant tensor input and fails:

n = self.n.item()        # self.n is a buffer
torch._check(n >= 1)
torch._check(n <= x.shape[0])
return x[:n] * self.w[:n]

TypeError: cannot create weak reference to 'bool' object

2. A folded parameter that still requires grad. Folding self.w.t(), or one chunk of a packed weight, gives a view that still has requires_grad=True, because torch.no_grad() does not clear that flag on a view. For some views, such as a transposed one, to_edge then copies the constant into a non-leaf tensor, and lowering to a backend can fail when it deep-copies the program:

RuntimeError: Only Tensors created explicitly by the user (graph leaves) support the deepcopy protocol at the moment.

An op can also return the parameter itself, like self.w.to(torch.float32) on a float32 parameter. Then the pass stores the nn.Parameter as a constant, and to_edge fails with AttributeError: ... is not a buffer.

What this change does

Two commits, one per crash:

1. Only tensors can become constant inputs. So a non-tensor value that a node outside the fold reads now stays in the graph, together with the non-tensor values it is computed from. Tensors computed from it still fold: self.w[:n] still becomes one constant. Programs that constant_prop_pass handled before are unchanged.
2. A folded constant must be a plain tensor that does not require grad, so the pass now detaches every folded tensor.

What was tested

On macOS arm64:

• Two new tests in exir/tests/test_passes.py, one per commit. Each fails before its commit and passes after: the first with TypeError: cannot create weak reference to 'int' object, the second with the AttributeError above.
  • The first covers a scalar read at runtime, one returned as an output, one whose first reader is folded, and one read only by folded nodes (it folds away). The program also lowers with to_edge(...).to_executorch().
  • The second covers the transposed view and the to case, and checks that the output matches eager.
• The rest of exir/tests/test_passes.py, exir/emit/test/test_emit.py and exir/tests/test_quant_fusion_pass.py. Five tests in those files fail the same way with and without this change, because they need the quantized and custom op libraries.

[pytorch-bot]

🔗 Helpful Links

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

• 📄 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 0d11cea with merge base 8739d28 ():
💚 Looks good so far! There are no failures yet. 💚

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

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[shoumikhin]

@claude review

[claude]

Claude finished @shoumikhin's task in 3m 55s —— View job

---

I found nothing blocking: both fixes look correct from reading the code, and the remaining points are minor test gaps and one comment whose stated reason may be wrong.

Detailed review

PR Review: #24182

Summary

The PR makes constant_prop_pass leave non-tensor values that a runtime op reads in the graph, and detaches every folded tensor. I found no correctness problems, and the callers (backends/samsung/_passes/customized_constant_prop.py, backends/transforms/fuse_rms_norm.py) are not broken.

I couldn't execute anything here, because Python and pytest need approval in this environment. Every finding below comes from reading the code.

Code Quality

• The comment at exir/passes/constant_prop_pass.py:186-188 may give the wrong cause (low confidence, could not run it).
  • As far as I know, an op run under torch.no_grad() returns a tensor with requires_grad=False, even when it is a view of a Parameter. What survives is the view's link to the parameter (_base), and detach() cuts that link. If so, the line "A view of a parameter still requires grad under no_grad" points future readers at the wrong mechanism.
  • The detach() itself is fine. Please check with with torch.no_grad(): v = p.t(); print(v.requires_grad, v._base is p) and reword the comment if needed.
• Stale type hints, minor. get_propagated_const_tensor_dict (constant_prop_pass.py:132) and the helpers it uses still annotate the dict as OrderedDict[Node, torch.Tensor]. The new pruning loop exists because the dict holds non-tensor values, so Union[torch.Tensor, Any] (or a short note) would match what the code does.

Testing

• test_constant_prop_pass_keeps_scalar_read_at_runtime never lowers the program.
  • After the pass, self.w[:n] becomes a constant with a fixed shape (2, 3), while x[:n] still has a shape that depends on n at runtime. The mul therefore reads two inputs whose shape metadata disagree, and aten.item is now left in the graph to run at runtime.
  • The test only runs new_ep.module() in eager mode, so a failure in a verifier, in spec propagation or in memory planning would go unnoticed. Adding to_edge(new_ep).to_executorch(), as the parameter-view test already does with to_edge, would show that the crash is fixed end to end.
• That same test never checks the bool path the description is about. It asserts that aten.item remains, but nothing about the ge/le → _assert_scalar nodes that produced the TypeError on a bool. If export ever settled those checks statically, the test would still pass while covering only the int path. Adding assertIn(torch.ops.aten._assert_scalar.default, targets) would close this.
• The "every user folds" branch of the pruning loop is untested.
  • Example: self.w * self.n.item(), where item should still be erased and the mul folded.
  • A version that dropped every non-tensor from the fold would also pass the current test.
  • Also worth covering: a dropped scalar computed from a folded call_function tensor rather than a placeholder, e.g. (self.n + 1).item() read at runtime. That checks the add result is turned into a new constant input.
• test_constant_prop_pass_folded_parameter_view_can_be_copied only checks that deepcopy doesn't raise. It never checks that t was actually folded into a constant, so the test would quietly stop covering the fix if a future skip-target or is_impure change stopped folding t. Asserting that aten.t is gone, plus a numerics check against eager, would harden it.

Recommendation

Approve. Nothing blocks merging. The to_edge/to_executorch check and the _assert_scalar assertion in the scalar test are the most worthwhile follow-ups.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[shoumikhin]

Thanks for the review. Here is what I did with each point.

1. The comment about requires_grad: I ran your check. Under torch.no_grad(), p.t() still has requires_grad=True, and _base is p. A view keeps the flag of its base, so the comment is right and I kept it.

2. Type hints: not changed. The dict held non-tensor values before this change too, the file is pyre-unsafe, and every value that becomes a placeholder is now a tensor. Widening four signatures fits better in its own cleanup.

3. Lowering the scalar test: done. It now runs to_edge(...).to_executorch().

4. _assert_scalar: the scalar test now checks that it stays in the graph.

5. Every user folds: the scalar test now has a second scalar that only folded nodes read, and checks that its buffer is gone. That check fails if every non-tensor is dropped from the fold. The first scalar now comes from a folded tensor, (self.n * 2).item().

6. Parameter view test: it now checks that t was folded and that the output matches eager.

Both tests still fail without their fixes.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

[shoumikhin]

@pytorchbot cherry-pick --onto release/1.6 -c critical

[pytorchbot]

Cherry picking #24182

The cherry pick PR is at #24224 and it is recommended to link a critical cherry pick PR with an issue.

Details for Dev Infra team

Raised by workflow job