Skip to content

Conversation

@zhxchen17
Copy link
Contributor

@zhxchen17 zhxchen17 commented Sep 3, 2024

Summary:
Current implementation for lifted graph takes a dict of [constant name: constant value]. And the constant value is used to run_node and excute the constant graph to get the folded values and then create new getattr nodes for folded values.

We don't have constant values for lifted graph during model compilation on MTIA. I think it is more general to allow the constant folding pass to just take the constant names only to produce the constant graph and represent the folded nodes as placeholders to make it consistent with lifted graph. Additionally, this mimic the real situation on Sigmoid, where Sigmoid executes the constant graph, get the folded values and set the folded values to the main graph. This diff is to update the pass to work with a list of constant names.

Test Plan:

buck run mode/opt caffe2/test:test_export -- -r split_const_gm

Differential Revision: D62144791

cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @ColinPeppler @amjames @desertfire @chauhang @aakhundov

@pytorch-bot
Copy link

pytorch-bot bot commented Sep 3, 2024

🔗 Helpful Links

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

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 8031cf0 with merge base a57e418 (image):
💚 Looks good so far! There are no failures yet. 💚

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

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D62144791

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D62144791

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D62144791

pytorch-bot bot pushed a commit that referenced this pull request Sep 10, 2024
Summary:
Pull Request resolved: #135060

Current implementation for lifted graph takes a dict of [constant name: constant value]. And the constant value is used to run_node and excute the constant graph to get the folded values and then create new getattr nodes for folded values.

However, we don't have constant values for lifted graph during model compilation on MTIA. I think it makes the pass more general by allowing it to just take the constant names only to produce the constant graph and represent the folded nodes as placeholders to make it consistent with lifted graph. Additionally, this mimic the real situation in Sigmoid, where Sigmoid executes the constant graph, get the folded values and set the folded values to the main graph. This diff is to update the pass to work with a list of constant names.

Test Plan:
```
buck run mode/opt caffe2/test:test_export -- -r split_const_gm
```

Differential Revision: D62144791
@eellison eellison self-requested a review September 10, 2024 19:01
@zhxchen17
Copy link
Contributor Author

clarification: I'm not the author of this diff, but I clicked the "Export" button on Phabricator.

Copy link
Contributor

@eellison eellison left a comment

Choose a reason for hiding this comment

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

many test failures - also cc @muchulee8 who added this part

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D62144791

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D62144791

1 similar comment
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D62144791

@zhxchen17 zhxchen17 force-pushed the export-D62144791 branch 2 times, most recently from 3fa1a5a to 90ef37a Compare September 12, 2024 17:01
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D62144791

zhxchen17 pushed a commit to zhxchen17/pytorch that referenced this pull request Sep 12, 2024
Summary:
Pull Request resolved: pytorch#135060

Current implementation for lifted graph takes a dict of [constant name: constant value]. And the constant value is used to run_node and excute the constant graph to get the folded values and then create new getattr nodes for folded values.

However, we don't have constant values for lifted graph during model compilation on MTIA. I think it makes the pass more general by allowing it to just take the constant names only to produce the constant graph and represent the folded nodes as placeholders to make it consistent with lifted graph. Additionally, this mimic the real situation in Sigmoid, where Sigmoid executes the constant graph, get the folded values and set the folded values to the main graph. This diff is to update the pass to work with a list of constant names.

Test Plan:
```
buck run mode/opt caffe2/test:test_export -- -r split_const_gm
```

Differential Revision: D62144791
@zhxchen17 zhxchen17 added the topic: not user facing topic category label Sep 12, 2024
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D62144791

zhxchen17 pushed a commit to zhxchen17/pytorch that referenced this pull request Sep 12, 2024
Summary:
Pull Request resolved: pytorch#135060

Current implementation for lifted graph takes a dict of [constant name: constant value]. And the constant value is used to run_node and excute the constant graph to get the folded values and then create new getattr nodes for folded values.

However, we don't have constant values for lifted graph during model compilation on MTIA. I think it makes the pass more general by allowing it to just take the constant names only to produce the constant graph and represent the folded nodes as placeholders to make it consistent with lifted graph. Additionally, this mimic the real situation in Sigmoid, where Sigmoid executes the constant graph, get the folded values and set the folded values to the main graph. This diff is to update the pass to work with a list of constant names.

Test Plan:
```
buck run mode/opt caffe2/test:test_export -- -r split_const_gm
```

Differential Revision: D62144791
zhxchen17 pushed a commit to zhxchen17/pytorch that referenced this pull request Sep 30, 2024
Summary:
Pull Request resolved: pytorch#135060

Current implementation for lifted graph takes a dict of [constant name: constant value]. And the constant value is used to run_node and excute the constant graph to get the folded values and then create new getattr nodes for folded values.

However, we don't have constant values for lifted graph during model compilation on MTIA. I think it makes the pass more general by allowing it to just take the constant names only to produce the constant graph and represent the folded nodes as placeholders to make it consistent with lifted graph. Additionally, this mimic the real situation in Sigmoid, where Sigmoid executes the constant graph, get the folded values and set the folded values to the main graph. This diff is to update the pass to work with a list of constant names.

Test Plan:
```
buck run mode/opt caffe2/test:test_export -- -r split_const_gm
```

Reviewed By: SherlockNoMad

Differential Revision: D62144791
pytorch-bot bot pushed a commit that referenced this pull request Oct 24, 2024
Summary:

Current implementation for lifted graph takes a dict of [constant name: constant value]. And the constant value is used to run_node and excute the constant graph to get the folded values and then create new getattr nodes for folded values.

However, we don't have constant values for lifted graph during model compilation on MTIA. I think it makes the pass more general by allowing it to just take the constant names only to produce the constant graph and represent the folded nodes as placeholders to make it consistent with lifted graph. Additionally, this mimic the real situation in Sigmoid, where Sigmoid executes the constant graph, get the folded values and set the folded values to the main graph. This diff is to update the pass to work with a list of constant names.

Test Plan:
```
buck run mode/opt caffe2/test:test_export -- -r split_const_gm
```

Reviewed By: muchulee8, SherlockNoMad

Differential Revision: D62144791
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D62144791

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D62144791

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D62144791

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D62144791

Summary:

Current implementation for lifted graph takes a dict of [constant name: constant value]. And the constant value is used to run_node and excute the constant graph to get the folded values and then create new getattr nodes for folded values.

However, we don't have constant values for lifted graph during model compilation on MTIA. I think it makes the pass more general by allowing it to just take the constant names only to produce the constant graph and represent the folded nodes as placeholders to make it consistent with lifted graph. Additionally, this mimic the real situation in Sigmoid, where Sigmoid executes the constant graph, get the folded values and set the folded values to the main graph. This diff is to update the pass to work with a list of constant names.

Test Plan:
```
buck run mode/opt caffe2/test:test_export -- -r split_const_gm
```

Reviewed By: muchulee8, SherlockNoMad

Differential Revision: D62144791
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D62144791

@facebook-github-bot
Copy link
Contributor

@pytorchbot merge

(Initiating merge automatically since Phabricator Diff has merged)

@pytorchmergebot
Copy link
Collaborator

Merge started

Your 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

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

The merge job was canceled or timed out. This most often happen if two merge requests were issued for the same PR, or if merge job was waiting for more than 6 hours for tests to finish. In later case, please do not hesitate to reissue the merge command
For more information see pytorch-bot wiki.

@wdvr
Copy link
Contributor

wdvr commented Oct 28, 2024

@pytorchbot merge -i

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged while ignoring the following 0 checks:

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

muchulee8 added a commit that referenced this pull request Apr 27, 2025
Summary:
Bug fix for #135060
Simple review:
https://github.com/pytorch/pytorch/pull/135060/files#diff-f23386709ff7e1235b15e18f835a48e5124e0ddd596aeb33c201daad1abbedd7R357
We mistakenly typed get_attr into getattr.

This causes constants never get untagged, and forces all constants get
cloned twice which greatly increases the memory consumption.

Test Plan:
python test/inductor/test_aot_inductor.py -k test_empty_constant_folding

Reviewers:

Subscribers:

Tasks:

Tags:

[ghstack-poisoned]
muchulee8 added a commit that referenced this pull request Apr 27, 2025
Summary:
Bug fix for #135060
Simple review:
https://github.com/pytorch/pytorch/pull/135060/files#diff-f23386709ff7e1235b15e18f835a48e5124e0ddd596aeb33c201daad1abbedd7R357
We mistakenly typed get_attr into getattr.

This causes constants never get untagged, and forces all constants get
cloned twice which greatly increases the memory consumption.

Test Plan:
python test/inductor/test_aot_inductor.py -k test_empty_constant_folding

Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: 4ee4b54
Pull Request resolved: #152273
pytorchmergebot pushed a commit that referenced this pull request May 1, 2025
Summary:
Bug fix for #135060
Simple review:
https://github.com/pytorch/pytorch/pull/135060/files#diff-f23386709ff7e1235b15e18f835a48e5124e0ddd596aeb33c201daad1abbedd7R357
We mistakenly typed get_attr into getattr.

This causes constants never get untagged, and forces all constants get
cloned twice which greatly increases the memory consumption.

Test Plan:
python test/inductor/test_aot_inductor.py -k test_empty_constant_folding

Reviewers:

Subscribers:

Tasks:

Tags:

Pull Request resolved: #152273
Approved by: https://github.com/trieuat, https://github.com/zhxchen17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants