Skip to content

Add expanded_def option for FX printing, render descriptor, update tests #158708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from

Conversation

ezyang
Copy link
Contributor

@ezyang ezyang commented Jul 19, 2025

Stack from ghstack (oldest at bottom):


  • First, we add a new expanded_def to FX, which will expand the
    definitions of variables into multiple lines, one per variable
    definition. This makes extremely long args/return lists much
    more readable.

  • Next, we extend this mechanism to also print out descriptors on
    placeholders and return values, as comments, if available. This
    is how we will test descriptors.

  • We update tlparse for AOTAutograd to use this format.

  • We update expect tests to use this format and update their formats,
    so you can inspect what it can look at. There may be other tests
    I should update, open to suggestions.

Signed-off-by: Edward Z. Yang ezyang@meta.com

cc @SherlockNoMad @EikanWang @jgong5 @wenzhe-nrv @voznesenskym @penguinwu @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @jiayisunx @chenyang78 @kadeng @chauhang @amjames @Lucaskabela

[ghstack-poisoned]
ezyang added a commit that referenced this pull request Jul 19, 2025
- First, we add a new expanded_def to FX, which will expand the
  definitions of variables into multiple lines, one per variable
  definition.  This makes extremely long args/return lists much
  more readable.

- Next, we extend this mechanism to also print out descriptors on
  placeholders and return values, as comments, if available.  This
  is how we will test descriptors.

- We update tlparse for AOTAutograd to use this format.

- We update expect tests to use this format and update their formats,
  so you can inspect what it can look at.  There may be other tests
  I should update, open to suggestions.

Signed-off-by: Edward Z. Yang <ezyang@meta.com>
ghstack-source-id: 09486be
Pull-Request: #158708
Copy link

pytorch-bot bot commented Jul 19, 2025

🔗 Helpful Links

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

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 5b49756 with merge base 85ee2fb (image):

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.

[ghstack-poisoned]
[ghstack-poisoned]
@ezyang ezyang requested review from jamesjwu and wconstab July 21, 2025 04:28
@ezyang ezyang added the ciflow/trunk Trigger trunk jobs on your pull request label Jul 21, 2025
primals_5: "Sym(s47)", # SubclassSizeAOTInput(base=PlainAOTInput(idx=2), idx=0)
primals_6: "Sym(s16)", # SubclassSizeAOTInput(base=PlainAOTInput(idx=2), idx=1)
primals_7: "Sym(s16)", # SubclassStrideAOTInput(base=PlainAOTInput(idx=2), idx=0)
):
mul: "f32[s47, s16]" = torch.ops.aten.mul.Tensor(primals_3, primals_1); primals_3 = None
mul_3: "f32[s47, s16]" = torch.ops.aten.mul.Tensor(primals_4, primals_1); primals_4 = None
return (mul, mul_3, primals_5, primals_7, primals_7, primals_1, primals_5, primals_7)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return printing not working here either 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because these are post partition and we lost the output desc

@ezyang ezyang mentioned this pull request Jul 22, 2025
# Unfortunately, flat_args_descs is not guaranteed to match the
# number of actual arguments that show up on the FX graph.
# Speciifcally, allow_token_discovery=True means that we will
# silently add extra token arguments to the backwards graph.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where can i learn what "token arguments" are?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @angelayi but I believe https://docs.google.com/document/d/179QyhicGzTXJ5jvTAoAosP_Nzgf3PpgZwU_E3VV9PlM/edit?tab=t.0#heading=h.pj3msnr95n30 was the design doc. But the idea is simple: if you need to prevent an operator from getting DCE'ed or reordered around other operators, having a "token" tensor which flows from one operation to the next enforces this dependency without having to introduce a new form of control dependency.

@@ -538,3 +538,12 @@ def call_and_expect_output_descs(fn, args):
outs_descs,
)
return outs_pair


def fn_wrappers(fn):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, is this the helper that you referred to as fn_stack in the next PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, and whoops I misremembered the name

Copy link
Contributor

@wconstab wconstab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assuming you fix the returns that aren't printing correctly, this LGTM.

[ghstack-poisoned]
ezyang added a commit that referenced this pull request Jul 24, 2025
- First, we add a new expanded_def to FX, which will expand the
  definitions of variables into multiple lines, one per variable
  definition.  This makes extremely long args/return lists much
  more readable.

- Next, we extend this mechanism to also print out descriptors on
  placeholders and return values, as comments, if available.  This
  is how we will test descriptors.

- We update tlparse for AOTAutograd to use this format.

- We update expect tests to use this format and update their formats,
  so you can inspect what it can look at.  There may be other tests
  I should update, open to suggestions.

Signed-off-by: Edward Z. Yang <ezyang@meta.com>
ghstack-source-id: 4df34ed
Pull-Request: #158708
Copy link

linux-foundation-easycla bot commented Jul 24, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@ezyang
Copy link
Contributor Author

ezyang commented Jul 24, 2025

Pushed the return values fix: the problem was I needed to have partitioner preserve descs when it creates the new output nodes. This is now implemented.

@ezyang ezyang force-pushed the gh/ezyang/3110/head branch from a900aa4 to 7b5fe5a Compare July 24, 2025 20:54
@ezyang ezyang force-pushed the gh/ezyang/3110/base branch from 932e256 to 4fdb7f9 Compare July 24, 2025 20:54
[ghstack-poisoned]
ezyang added a commit that referenced this pull request Jul 24, 2025
- First, we add a new expanded_def to FX, which will expand the
  definitions of variables into multiple lines, one per variable
  definition.  This makes extremely long args/return lists much
  more readable.

- Next, we extend this mechanism to also print out descriptors on
  placeholders and return values, as comments, if available.  This
  is how we will test descriptors.

- We update tlparse for AOTAutograd to use this format.

- We update expect tests to use this format and update their formats,
  so you can inspect what it can look at.  There may be other tests
  I should update, open to suggestions.

Signed-off-by: Edward Z. Yang <ezyang@meta.com>
ghstack-source-id: 4df34ed
Pull-Request: #158708
[ghstack-poisoned]
ezyang added a commit that referenced this pull request Jul 25, 2025
- First, we add a new expanded_def to FX, which will expand the
  definitions of variables into multiple lines, one per variable
  definition.  This makes extremely long args/return lists much
  more readable.

- Next, we extend this mechanism to also print out descriptors on
  placeholders and return values, as comments, if available.  This
  is how we will test descriptors.

- We update tlparse for AOTAutograd to use this format.

- We update expect tests to use this format and update their formats,
  so you can inspect what it can look at.  There may be other tests
  I should update, open to suggestions.

Signed-off-by: Edward Z. Yang <ezyang@meta.com>
ghstack-source-id: 29645ba
Pull-Request: #158708
@ezyang
Copy link
Contributor Author

ezyang commented Jul 25, 2025

@pytorchbot merge -f "all clear everything is green"

@pytorchmergebot
Copy link
Collaborator

Starting merge as part of PR stack under #158734

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

pytorchmergebot pushed a commit that referenced this pull request Jul 25, 2025
Wrapping is load bearing for things that introspect argument signatures,
but use of functools.wraps to do this is undesirable as this overrides
the name/module of the wrapping function, which is bad for tracking down
exactly what code is actually being run at runtime.  simple_wraps is
like wraps but it doesn't override the name information, so you still
get an appropriate printout.  To see the stack of all functions wrapping
each other, there is now a helper fn_stack.

I also make some assertions tighter in the descriptor PR.  These didn't
catch any bugs but I figure might as well.

Signed-off-by: Edward Z. Yang <ezyang@meta.com>
Pull Request resolved: #158734
Approved by: https://github.com/wconstab
ghstack dependencies: #158624, #158708
pytorchmergebot pushed a commit that referenced this pull request Jul 25, 2025
…riptors (#158715)

Signed-off-by: Edward Z. Yang <ezyang@meta.com>
Pull Request resolved: #158715
Approved by: https://github.com/fmassa, https://github.com/wconstab, https://github.com/xmfan
ghstack dependencies: #158624, #158708, #158734
yangw-dev pushed a commit that referenced this pull request Aug 1, 2025
…sts (#158708)

----

- First, we add a new expanded_def to FX, which will expand the
  definitions of variables into multiple lines, one per variable
  definition.  This makes extremely long args/return lists much
  more readable.

- Next, we extend this mechanism to also print out descriptors on
  placeholders and return values, as comments, if available.  This
  is how we will test descriptors.

- We update tlparse for AOTAutograd to use this format.

- We update expect tests to use this format and update their formats,
  so you can inspect what it can look at.  There may be other tests
  I should update, open to suggestions.

Signed-off-by: Edward Z. Yang <ezyang@meta.com>

Pull Request resolved: #158708
Approved by: https://github.com/wconstab
ghstack dependencies: #158624
yangw-dev pushed a commit that referenced this pull request Aug 1, 2025
Wrapping is load bearing for things that introspect argument signatures,
but use of functools.wraps to do this is undesirable as this overrides
the name/module of the wrapping function, which is bad for tracking down
exactly what code is actually being run at runtime.  simple_wraps is
like wraps but it doesn't override the name information, so you still
get an appropriate printout.  To see the stack of all functions wrapping
each other, there is now a helper fn_stack.

I also make some assertions tighter in the descriptor PR.  These didn't
catch any bugs but I figure might as well.

Signed-off-by: Edward Z. Yang <ezyang@meta.com>
Pull Request resolved: #158734
Approved by: https://github.com/wconstab
ghstack dependencies: #158624, #158708
yangw-dev pushed a commit that referenced this pull request Aug 1, 2025
…riptors (#158715)

Signed-off-by: Edward Z. Yang <ezyang@meta.com>
Pull Request resolved: #158715
Approved by: https://github.com/fmassa, https://github.com/wconstab, https://github.com/xmfan
ghstack dependencies: #158624, #158708, #158734
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request fx Merged module: dynamo release notes: fx release notes category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants