[dynamo] Support list subclasses and fix dict subclasses mutation bugs #146819

anijain2305 · 2025-02-10T07:34:00Z

Stack from ghstack (oldest at bottom):

This PR adds support for list subclasses. Among other things are

Tracking the mutations on internal vts like _dict_vt and _list_vt using sources. This helps identify if there was a mutation in the underlying data structures, and we need to reconstruct it.
UserDefinedObjectVariable now has a new method - is_modified which side_effect infra relies upon to check mutations in the underlying vts (like _dict_vt).
reconstruction logic ensures that we use dict.__getitem__ and list.__getitem__ methods. This is super important because we don't want to call the overridden __getitem__ methods.

If this PR is hard to review, please let me know. I can break it into several small PRs.

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

[ghstack-poisoned]

pytorch-bot · 2025-02-10T07:34:04Z

🔗 Helpful Links

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

📄 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.

❌ 1 New Failure, 5 Pending, 5 Unrelated Failures

As of commit 3e734ab with merge base 6105b6f ():

NEW FAILURE - The following job has failed:

inductor / unit-test / cuda12.4-py3.12-gcc9-sm86 / test (inductor, 1, 2, linux.g5.4xlarge.nvidia.gpu) (gh)
'Test'

FLAKY - The following job failed but was likely due to flakiness present on trunk:

trunk / win-vs2022-cpu-py3 / test (default, 2, 3, windows.4xlarge.nonephemeral) (gh) (similar failure)
Process completed with exit code 1.

BROKEN TRUNK - The following jobs failed but was 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.

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

ghstack-source-id: a0de941 Pull Request resolved: #146819

Skylion007 · 2025-02-10T15:28:15Z

torch/_dynamo/utils.py


 tuple_new = tuple.__new__
 tuple_methods = {method for method in tuple.__dict__.values() if callable(method)}
+list_methods = {method for method in list.__dict__.values() if callable(method)}


Nit, probably better for another PR, but these global sets are good candidates to become frozen sets

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

ghstack-source-id: 5d63e95 Pull Request resolved: #146819

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

…utation bugs" This PR adds support for list subclasses. Among other things are 1) Tracking the mutations on internal vts like `_dict_vt` and `_list_vt` using sources. This helps identify if there was a mutation in the underlying data structures, and we need to reconstruct it. 2) `UserDefinedObjectVariable` now has a new method - `is_modified` which `side_effect` infra relies upon to check mutations in the underlying vts (like `_dict_vt`). 3) `reconstruction` logic ensures that we use `dict.__getitem__` and `list.__getitem__` methods. This is super important because we don't want to call the overridden `__getitem__` methods. If this PR is hard to review, please let me know. I can break it into several small PRs. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

anijain2305 · 2025-02-11T17:53:03Z

CI failures are not relevant to this PR

StrongerXi

Cool! Dropped some questions.

StrongerXi · 2025-02-11T19:25:03Z

test/dynamo/test_dicts.py

-            x = x * sd.attr
-            sd.attr = 10
-            x = x * sd.attr
            return x


Did the removed portion cause a failure?

Yes, it uncovered cases where there is mutation on the dict but no attribute mutation. Earlier, this was missed. We have other test that test for attr mutation, so we are covered in general.

StrongerXi · 2025-02-11T19:25:30Z

test/dynamo/test_dicts.py

+            d["baz"] = 4
            return x * d["foo"] * d["bar"]

+        fn(torch.randn(4), d)


Why do we call this twice?

Unintentional, I was debugging an issue. Let me remove it.

StrongerXi · 2025-02-11T19:43:35Z

torch/_dynamo/utils.py

-    return next(itertools.islice(iter(d), n, n + 1))
+    # Call dict(d) to prevent calling overridden __iter__
+    dict_class = dict
+    if isinstance(d, OrderedDict):
+        dict_class = OrderedDict
+    return next(itertools.islice(iter(dict_class(d)), n, n + 1))


qqs:

do we have a test that forced this change?

can we use dict/OrderedDict.keys(d) (to avoid reconstructing the entire object via dict_class(d)), given that's how we accessed the keys in builder?

pytorch/torch/_dynamo/utils.py

Lines 2367 to 2375 in 6f15a60

def get_items_from_dict(obj):

# Get items without calling the user defined __getitem__ or keys method.

assert isinstance(obj, dict)

if istype(obj, (dict, OrderedDict)):

return obj.items()

elif isinstance(obj, OrderedDict):

return [(k, OrderedDict.__getitem__(obj, k)) for k in OrderedDict.keys(obj)]

else:

return [(k, dict.__getitem__(obj, k)) for k in dict.keys(obj)]

Very good idea. Let me try it out.

Yes, there was a failure in one of the tests in test_dicts.py.

StrongerXi · 2025-02-11T19:52:45Z

torch/_dynamo/variables/user_defined.py

+        return side_effects.is_attribute_mutation(self) or side_effects.is_modified(
+            self._dict_vt
+        )


I think side_effects.is_attribute_mutation(self) would always return true for UserDefinedListVariable etc.?
Doesn't that cause redundant codegen calls if the underlying user-defined list was never modified?

Thanks for catching this. yes, this is wrong. I wanted to use something like

self in side_effects.store_attr_mutations

I see; in that case the better abstraction to use might be just side_effects.is_modified(self).

jansel · 2025-02-12T00:10:11Z

torch/_dynamo/side_effects.py

        dict_class.__setitem__(dict_to, k, v)


+def _manual_list_setitem(list_from, list_to):


This isn't setitem, it is more of list_replace.

jansel · 2025-02-12T00:11:07Z

torch/_dynamo/side_effects.py

+                        ]
+                    )
+
+                    list_update_insts = bytecode_from_template(


Will this work if the user does:

list = None

in global scope?

No. This is an existing problem at other places as well, like dict = None would also cause similar issues. I will merge the PR and send another after auditing other places.

…utation bugs" This PR adds support for list subclasses. Among other things are 1) Tracking the mutations on internal vts like `_dict_vt` and `_list_vt` using sources. This helps identify if there was a mutation in the underlying data structures, and we need to reconstruct it. 2) `UserDefinedObjectVariable` now has a new method - `is_modified` which `side_effect` infra relies upon to check mutations in the underlying vts (like `_dict_vt`). 3) `reconstruction` logic ensures that we use `dict.__getitem__` and `list.__getitem__` methods. This is super important because we don't want to call the overridden `__getitem__` methods. If this PR is hard to review, please let me know. I can break it into several small PRs. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

ghstack-source-id: 1228ee6 Pull Request resolved: #146819

anijain2305 · 2025-02-12T05:45:55Z

@pytorcchnot merge

anijain2305 · 2025-02-12T05:46:17Z

@pytorchbot merge

pytorchmergebot · 2025-02-12T05:47:59Z

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 · 2025-02-12T10:27:07Z

Merge failed

Reason: 1 jobs have failed, first few of them are: inductor / unit-test / cuda12.4-py3.12-gcc9-sm86 / test (inductor, 1, 2, linux.g5.4xlarge.nvidia.gpu)

Details for Dev Infra team

Raised by workflow job

anijain2305 · 2025-02-12T17:44:08Z

@pytorchbot merge -f "flaky CI"

pytorchmergebot · 2025-02-12T17:45:39Z

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

Summary: This PR adds support for list subclasses. Among other things are 1) Tracking the mutations on internal vts like `_dict_vt` and `_list_vt` using sources. This helps identify if there was a mutation in the underlying data structures, and we need to reconstruct it. 2) `UserDefinedObjectVariable` now has a new method - `is_modified` which `side_effect` infra relies upon to check mutations in the underlying vts (like `_dict_vt`). 3) `reconstruction` logic ensures that we use `dict.__getitem__` and `list.__getitem__` methods. This is super important because we don't want to call the overridden `__getitem__` methods. If this PR is hard to review, please let me know. I can break it into several small PRs. X-link: pytorch/pytorch#146819 Approved by: https://github.com/StrongerXi, https://github.com/jansel Reviewed By: huydhn Differential Revision: D69537369 fbshipit-source-id: 9c20f4ee84c91639c320a3a04a1a153859623ab6

Pull Request resolved: #146995 Approved by: https://github.com/StrongerXi ghstack dependencies: #146819

Pull Request resolved: #147046 Approved by: https://github.com/StrongerXi ghstack dependencies: #146819, #146995

ghstack-source-id: c74750d Pull Request resolved: pytorch/pytorch#146819

[dynamo][lists] Support list subclasses

7270a4a

[ghstack-poisoned]

This was referenced Feb 10, 2025

[dynamo][user-defined] User class.__new__ instead of special casing #146677

Closed

[dynamo][user-defined] Unify standard and non-standard __new__ codebase #146737

Closed

pytorch-bot bot added ciflow/inductor module: dynamo labels Feb 10, 2025

Update on "[dynamo][lists] Support list subclasses"

8fbc9df

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

anijain2305 added a commit that referenced this pull request Feb 10, 2025

[dynamo][lists] Support list subclasses

62eb4f6

ghstack-source-id: a0de941 Pull Request resolved: #146819

Skylion007 reviewed Feb 10, 2025

View reviewed changes

Update on "[dynamo][lists] Support list subclasses"

7ec9604

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

Update on "[dynamo][lists] Support list subclasses"

af2cf08

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

anijain2305 added ciflow/trunk Trigger trunk jobs on your pull request topic: not user facing topic category labels Feb 11, 2025

Update on "[dynamo][lists] Support list subclasses"

06a2b88

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

anijain2305 added a commit that referenced this pull request Feb 11, 2025

[dynamo][lists] Support list subclasses

4198f4c

ghstack-source-id: 5d63e95 Pull Request resolved: #146819

Update on "[dynamo][lists] Support list subclasses"

c8bfcdb

cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]

anijain2305 mentioned this pull request Feb 11, 2025

[dynamo][source] Remove index_is_slice from GetItemSource #146887

Closed

anijain2305 changed the title ~~[dynamo][lists] Support list subclasses~~ [dynamo] Support list subclasses and fix dict subclasses mutation bugs Feb 11, 2025

anijain2305 requested review from StrongerXi, jansel and williamwen42 February 11, 2025 18:40

StrongerXi approved these changes Feb 11, 2025

View reviewed changes

jansel approved these changes Feb 12, 2025

View reviewed changes

anijain2305 added a commit that referenced this pull request Feb 12, 2025

[dynamo][lists] Support list subclasses

458ad61

ghstack-source-id: 1228ee6 Pull Request resolved: #146819

pytorchmergebot added the merging label Feb 12, 2025

pytorchmergebot removed the merging label Feb 12, 2025

anijain2305 mentioned this pull request Feb 12, 2025

[dynamo] Fix tensordict regression #146995

Closed

pytorchmergebot added the merging label Feb 12, 2025

pytorchmergebot closed this in d6513f3 Feb 12, 2025

pytorchmergebot added Merged and removed merging labels Feb 12, 2025

anijain2305 mentioned this pull request Feb 13, 2025

[dynamo] Make SliceVariable a subclass of VariableTracker #147046

Closed

desai0007 pushed a commit to desai0007/test-repo-pytorch that referenced this pull request Feb 26, 2025

[dynamo][lists] Support list subclasses

40850da

ghstack-source-id: c74750d Pull Request resolved: pytorch/pytorch#146819

github-actions bot deleted the gh/anijain2305/677/head branch March 23, 2025 02:17

	def get_items_from_dict(obj):
	# Get items without calling the user defined __getitem__ or keys method.
	assert isinstance(obj, dict)
	if istype(obj, (dict, OrderedDict)):
	return obj.items()
	elif isinstance(obj, OrderedDict):
	return [(k, OrderedDict.__getitem__(obj, k)) for k in OrderedDict.keys(obj)]
	else:
	return [(k, dict.__getitem__(obj, k)) for k in dict.keys(obj)]

		dict_class.__setitem__(dict_to, k, v)


		def _manual_list_setitem(list_from, list_to):

[dynamo] Support list subclasses and fix dict subclasses mutation bugs #146819

[dynamo] Support list subclasses and fix dict subclasses mutation bugs #146819

Uh oh!

Conversation

anijain2305 commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Feb 10, 2025 • 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/146819

❌ 1 New Failure, 5 Pending, 5 Unrelated Failures

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anijain2305 commented Feb 11, 2025

Uh oh!

StrongerXi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anijain2305 Feb 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anijain2305 commented Feb 12, 2025

Uh oh!

anijain2305 commented Feb 12, 2025

Uh oh!

pytorchmergebot commented Feb 12, 2025

Merge started

Uh oh!

pytorchmergebot commented Feb 12, 2025

Merge failed

Uh oh!

anijain2305 commented Feb 12, 2025

Uh oh!

pytorchmergebot commented Feb 12, 2025

Merge started

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

anijain2305 commented Feb 10, 2025 •

edited

Loading

pytorch-bot bot commented Feb 10, 2025 •

edited

Loading

anijain2305 Feb 11, 2025 •

edited

Loading