Add aot_autograd.fx_utils #159005
Add aot_autograd.fx_utils #159005
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/159005
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit 378ad97 with merge base ba949c5 ( UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
github-actions
bot
requested review from
SherlockNoMad,
albanD,
antoniojkim and
miladm
| ) | ||
|
|
||
|
|
||
| def _raise_autograd_subclass_not_implemented() -> NoReturn: |
There was a problem hiding this comment.
nit: might be nice to pass in the 'name' of the unsupported subclass to include in the RuntimeError (e.g. input vs output node, possibly also the full name of the placeholder).
Also, as it stands the sentence isn't grammatical, "Subclasses are currently not.."
| if isinstance(sub_d, SubclassGetAttrAOTOutput): | ||
| _raise_autograd_subclass_not_implemented() | ||
| output_index[sub_d] = (sub_n, None) | ||
| for n in g.nodes: |
There was a problem hiding this comment.
any reason to have a separate loop instead of an elif n.op == 'placeholder"?
There was a problem hiding this comment.
Yes, you need to fully populate the output_index first before you do the other nodes.
| if n.op == "placeholder": | ||
| desc = n.meta["desc"] | ||
| if isinstance(desc, SubclassGetAttrAOTInput): | ||
| _raise_fqn_subclass_not_implemented() |
There was a problem hiding this comment.
so, the FQN problem is indeed distinct from the other subclass error above. but doesn't that error also apply here? i guess it doesn't matter which error you return if that's true, so this is also fine.
There was a problem hiding this comment.
Yes it is technically a different problem. It is best understood by thinking about what we'd have to do to "fix" the problem. For the autograd problem, you need to define what to do with subclass primals/tangents (which possibly mismatch). For the fqn problem, you need to define what to do if you have a subclass parameter.
| A list of FX nodes representing all parameters in the graph. | ||
|
|
||
| Raises: | ||
| RuntimeError: If subclass tensors are encountered (not yet supported), as |
There was a problem hiding this comment.
i could imagine someone arguing for this function just returning the flat list of parameters. But i'm ok with leaving it raising for now until someone complains
There was a problem hiding this comment.
I actually don't understand this comment. Isn't this a flat list of parameter (nodes)? You can't return the parameters, I don't have 'em!
ezyang
added
topic: not user facing
See docblock for details. The API here has been validated by use in autoparallel but I'm always open to suggestions for tweaks. One particular choice I made is to make most of the functions return dicts by default; this isn't strictly necessary for inputs but it is very convenient for outputs as the output desc lives on the output node, not the argument that feeds into the node. Signed-off-by: Edward Z. Yang <ezyang@meta.com> ghstack-source-id: 8d200da Pull-Request: #159005
See docblock for details. The API here has been validated by use in autoparallel but I'm always open to suggestions for tweaks. One particular choice I made is to make most of the functions return dicts by default; this isn't strictly necessary for inputs but it is very convenient for outputs as the output desc lives on the output node, not the argument that feeds into the node. Signed-off-by: Edward Z. Yang <ezyang@meta.com> ghstack-source-id: f6d9578 Pull-Request: #159005
|
@pytorchbot merge |
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: New commits were pushed while merging. Please rerun the merge command. Details for Dev Infra teamRaised by workflow job |
|
@pytorchbot merge |
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 |
See docblock for details. The API here has been validated by use in autoparallel but I'm always open to suggestions for tweaks. One particular choice I made is to make most of the functions return dicts by default; this isn't strictly necessary for inputs but it is very convenient for outputs as the output desc lives on the output node, not the argument that feeds into the node. Signed-off-by: Edward Z. Yang <ezyang@meta.com> Pull Request resolved: #159005 Approved by: https://github.com/wconstab
3 participants
Stack from ghstack (oldest at bottom):
See docblock for details. The API here has been validated by use
in autoparallel but I'm always open to suggestions for tweaks. One
particular choice I made is to make most of the functions return dicts
by default; this isn't strictly necessary for inputs but it is very
convenient for outputs as the output desc lives on the output node,
not the argument that feeds into the node.
Signed-off-by: Edward Z. Yang ezyang@meta.com