[Fix]: ScriptObject lifting issue #130952

jiashenC · 2024-07-17T16:25:37Z

Issue

ScriptObject was treated as normal attribute by the converter previously. This PR lifts it to be a constant and convert it directly to a GetAttr fx node. ScriptObject would also trigger CallMethod and this PR adds that support as well.

Test Plan

Add test case for ScriptObject.
pytest test/export/test_converter.py -s -k test_convert_script_object

pytorch-bot · 2024-07-17T16:25:40Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit the bot commands wiki or our office hours

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 86a724d with merge base 6c65fd0 ():

BROKEN TRUNK - The following job failed but were present on the merge base:

👉 Rebase onto the `viable/strict` branch to avoid these failures

Lint / lintrunner-noclang / linux-job (gh) (trunk failure)
>>> Lint for torch/_higher_order_ops/wrap.py:

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

test/export/test_converter.py

torch/_export/converter.py

facebook-github-bot · 2024-07-24T16:41:12Z

@jiashenC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: #### Issue ScriptObject was treated as normal attribute by the converter previously. This PR lifts it to be a constant and convert it directly to a GetAttr fx node. ScriptObject would also trigger `CallMethod` and this PR adds that support as well. Test Plan: Add test case for ScriptObject. `pytest test/export/test_converter.py -s -k test_convert_script_object` Differential Revision: D60177312 Pulled By: jiashenC

facebook-github-bot · 2024-07-24T22:09:45Z

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

jiashenC · 2024-07-24T23:25:48Z

torch/_export/converter.py


-        attr_value = node.output()
        if self.is_top_level_graph():
-            if attr_value.type().annotation_str == "Tensor":


I find this check to be not reliable, e.g., sometimes this is Optional[Tensor].

Summary: ScriptObject was treated as normal attribute by the converter previously. This PR lifts it to be a constant and convert it directly to a GetAttr fx node. ScriptObject would also trigger `CallMethod` and this PR adds that support as well. Test Plan: Add test case for ScriptObject. `pytest test/export/test_converter.py -s -k test_convert_script_object` Differential Revision: D60177312 Pulled By: jiashenC

torch/_export/converter.py

Summary: ScriptObject was treated as normal attribute by the converter previously. This PR lifts it to be a constant and convert it directly to a GetAttr fx node. ScriptObject would also trigger `CallMethod` and this PR adds that support as well. Test Plan: Add test case for ScriptObject. `pytest test/export/test_converter.py -s -k test_convert_script_object` Differential Revision: D60177312 Pulled By: jiashenC

facebook-github-bot · 2024-08-02T04:23:37Z

@jiashenC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-08-02T15:50:20Z

@jiashenC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: ScriptObject was treated as normal attribute by the converter previously. This PR lifts it to be a constant and convert it directly to a GetAttr fx node. ScriptObject would also trigger `CallMethod` and this PR adds that support as well. Test Plan: Add test case for ScriptObject. `pytest test/export/test_converter.py -s -k test_convert_script_object` Differential Revision: D60177312 Pulled By: jiashenC

facebook-github-bot · 2024-08-03T00:09:30Z

@jiashenC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Summary: ScriptObject was treated as normal attribute by the converter previously. This PR lifts it to be a constant and convert it directly to a GetAttr fx node. ScriptObject would also trigger `CallMethod` and this PR adds that support as well. Test Plan: Add test case for ScriptObject. `pytest test/export/test_converter.py -s -k test_convert_script_object` Differential Revision: D60177312 Pulled By: jiashenC

facebook-github-bot · 2024-08-04T02:11:09Z

@jiashenC has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-08-04T16:50:55Z

@pytorchbot merge -f 'Landed internally'

(Initiating merge automatically since Phabricator Diff has merged, using force because this PR might not pass merge_rules.json but landed internally)

pytorchmergebot · 2024-08-04T16:52:31Z

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

pytorch-bot bot added the ciflow/inductor label Jul 17, 2024

jiashenC requested a review from angelayi July 17, 2024 16:25

angelayi reviewed Jul 17, 2024

View reviewed changes

test/export/test_converter.py Outdated Show resolved Hide resolved

torch/_export/converter.py Outdated Show resolved Hide resolved

jiashenC added the topic: not user facing topic category label Jul 17, 2024

jiashenC requested a review from angelayi July 17, 2024 18:43

jiashenC marked this pull request as ready for review July 17, 2024 18:43

jiashenC requested review from avikchaudhuri, gmagogsfm, tugsbayasgalan and zhxchen17 as code owners July 17, 2024 18:43

jiashenC force-pushed the fix_script_object branch from cd57526 to 469f634 Compare July 18, 2024 22:46

jiashenC requested a review from ydwu4 as a code owner July 19, 2024 22:56

angelayi reviewed Jul 23, 2024

View reviewed changes

torch/_export/converter.py Outdated Show resolved Hide resolved

torch/_export/converter.py Outdated Show resolved Hide resolved

jiashenC force-pushed the fix_script_object branch from 65aaaac to 0cb99c3 Compare July 24, 2024 16:39

facebook-github-bot force-pushed the fix_script_object branch from 0cb99c3 to 27eccf0 Compare July 24, 2024 22:09

facebook-github-bot added the fb-exported label Jul 24, 2024

jiashenC commented Jul 24, 2024

View reviewed changes

jiashenC requested a review from angelayi July 24, 2024 23:26

jiashenC force-pushed the fix_script_object branch from eb6efea to 3b74950 Compare July 26, 2024 17:37

angelayi reviewed Aug 1, 2024

View reviewed changes

torch/_export/converter.py Show resolved Hide resolved

torch/_export/converter.py Outdated Show resolved Hide resolved

torch/_export/converter.py Show resolved Hide resolved

jiashenC force-pushed the fix_script_object branch from 3b74950 to 7c1f34c Compare August 1, 2024 23:45

angelayi approved these changes Aug 2, 2024

View reviewed changes

jiashenC force-pushed the fix_script_object branch from b316509 to b24fbfa Compare August 2, 2024 15:49

jiashenC force-pushed the fix_script_object branch from b24fbfa to 491948b Compare August 3, 2024 00:09

jiashenC added 7 commits August 3, 2024 19:09

fix lint

6d6ec9a

merge name_to_constant/tensor_constant

e446f98

update how to populate constants

0ded527

fix constants

43a2a86

switch to assertion

6afa372

add another assertion for inputs

86a724d

jiashenC force-pushed the fix_script_object branch from 491948b to 86a724d Compare August 4, 2024 02:09

pytorchmergebot added the merging label Aug 4, 2024

pytorchmergebot closed this in 09fcd79 Aug 4, 2024

pytorchmergebot added Merged and removed merging labels Aug 4, 2024

github-actions bot deleted the fix_script_object branch September 5, 2024 02:02

[Fix]: ScriptObject lifting issue #130952

[Fix]: ScriptObject lifting issue #130952

Uh oh!

Conversation

jiashenC commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Test Plan

Uh oh!

pytorch-bot bot commented Jul 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

✅ You can merge normally! (1 Unrelated Failure)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot commented Jul 24, 2024

Uh oh!

facebook-github-bot commented Jul 24, 2024

Uh oh!

jiashenC Jul 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

facebook-github-bot commented Aug 2, 2024

Uh oh!

facebook-github-bot commented Aug 2, 2024

Uh oh!

facebook-github-bot commented Aug 3, 2024

Uh oh!

facebook-github-bot commented Aug 4, 2024

Uh oh!

facebook-github-bot commented Aug 4, 2024

Uh oh!

pytorchmergebot commented Aug 4, 2024

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jiashenC commented Jul 17, 2024 •

edited

Loading

pytorch-bot bot commented Jul 17, 2024 •

edited

Loading