fix non-strict placeholder naming with kwargs #144278
Conversation
Fixes #143732 Differential Revision: [D67872055](https://our.internmc.facebook.com/intern/diff/D67872055/) [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/144278
Note: Links to docs will display an error until the docs builds have been completed. ❌ 2 New Failures, 1 Unrelated FailureAs of commit ac4e4e1 with merge base f6488d8 ( NEW FAILURES - The following jobs have failed:
BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Fixes #143732 Differential Revision: [D67872055](https://our.internmc.facebook.com/intern/diff/D67872055/) ghstack-source-id: 260395123 Pull Request resolved: #144278
|
This pull request was exported from Phabricator. Differential Revision: D67872055 |
torch/_export/utils.py
Outdated
| @@ -821,6 +821,14 @@ def _extract_pytree_key(x): | |||
|
|
|||
| # map user input names with mod.forward() signature | |||
| combined_args = _bind_signature_to_inputs(mod, fake_args, fake_kwargs) | |||
| # reorder following args, kwargs | |||
| combined_args = { | |||
| name: combined_args[name] | |||
There was a problem hiding this comment.
just curious why the order in the dictionary matters? If the order matters, maybe we should make it an ordered dict to be explicit?
There was a problem hiding this comment.
Use itertools.chain. No need for this weird list unpacking
There was a problem hiding this comment.
Turns out that the graph signature has user inputs in the order they were received, but combined args orders them in the order of the original forward signature. The actual ordering doesn't matter so much as that they should be consistent, because we zip them soon afterwards.
There was a problem hiding this comment.
@yushangdi I ended up changing the implementation a bit because it was not accounting for variadic kwargs. Latest update matches what happens in strict.
Given a module signature: def forward(a, b, c, **kwargs): ...
and example inputs as args: a_val, kwargs: {c=c_val, b=b_val, d=d_val}
we should now get placeholder names a, c, b, d. So overall preserving call site order instead of def site order.
Fixes #143732 Differential Revision: [D67872055](https://our.internmc.facebook.com/intern/diff/D67872055/) [ghstack-poisoned]
|
This pull request was exported from Phabricator. Differential Revision: D67872055 |
Fixes #143732 Differential Revision: [D67872055](https://our.internmc.facebook.com/intern/diff/D67872055/) [ghstack-poisoned]
|
This pull request was exported from Phabricator. Differential Revision: D67872055 |
Fixes #143732 Differential Revision: [D67872055](https://our.internmc.facebook.com/intern/diff/D67872055/) [ghstack-poisoned]
|
This pull request was exported from Phabricator. Differential Revision: D67872055 |
Pull Request resolved: #144278 Fixes #143732 ghstack-source-id: 260419919 @exported-using-ghexport Differential Revision: [D67872055](https://our.internmc.facebook.com/intern/diff/D67872055/)
|
@pytorchbot merge (Initiating merge automatically since Phabricator Diff has merged) |
Merge startedYour change will be merged once all checks pass (ETA 0-4 Hours). Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Merge failedReason: 2 jobs have failed, first few of them are: linux-aarch64 / linux-jammy-aarch64-py3.10 / test (default, 4, 4, linux.arm64.2xlarge), linux-aarch64 / linux-jammy-aarch64-py3.10 / test (default, 2, 3, linux.arm64.m7g.4xlarge) Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge -f "bogus CI failures" |
Merge startedYour change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
Stack from ghstack (oldest at bottom):
Fixes #143732
Differential Revision: D67872055