Fix create_mutable_buffer recording the requested name instead of the name torch.fx assigned - #21565
Fix create_mutable_buffer recording the requested name instead of the name torch.fx assigned#21565slipstr34m wants to merge 2 commits into
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/21565
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
|
@pytorchbot label "release notes: none" |
|
The merged implementation had a check to avoid duplicate parameter names on recompile. This one does not, so we can still have this duplicate issue. Since the other has landed, maybe update this PR to extract a helper and use that, so they stay in sync and we don't have holes like this |
… name torch.fx assigned
…le buffer requests
3bf2858 to
286b7bf
Compare
|
Good call. Extracted |
Follow up from the review discussion on #21542 where @JakeStevens flagged that
create_mutable_bufferprobably has the same latent bug. It does, and the collision case is a bit worse.Same pattern, the node gets created with the requested name and the signature records that name instead of whatever fx actually assigned:
A digit-leading name leaves dangling refs in both the input and output specs and recompile dies with
SyntaxError: invalid decimal literal. A name that collides with an existing placeholder is nastier, fx renames the node and the output spec silently binds to the colliding node, so the mutation entry points at the wrong tensor before recompile fails withSyntaxError: duplicate argument.No in-repo callers yet (came in with #13813, only its tests use it), so this is hardening a public utility rather than fixing a user visible crash.
Fix is the same as #21542, everything follows the assigned name:
node.target, the state_dict key and bothTensorArguments usenode.name, with theb_prefix convention applied to the assigned name when deriving the target. If a rename lands on an existing state_dict key the node is removed and the existing duplicate-target error is raised instead of overwriting the entry. Valid unique names behave exactly as before.Per review, the creation logic is now a shared helper,
_create_placeholder_node, used by both functions so they stay in sync, with a_find_placeholderlookup beside it. The dedup semantics stay deliberately different:create_constant_placeholderreturns the existing node for a repeated request, whilecreate_mutable_bufferraises, since a mutable buffer cannot be silently shared.Before/after with the same probe, checking signature consistency and a recompile round trip:
Two new tests: one covers both rename triggers, asserting node, signature and state_dict consistency plus a recompile round trip, and is verified failing-first by reverting just
backends/transforms/utils.py. The other pins the duplicate-request error for a clean and a renamed name; it passes before the helper change too and exists to guard the raise semantics.lintrunner is clean on the changed files. Independent of #21542, either can merge first.
cc @JakeStevens @digantdesai