NamedTuple: Allow side effects for dynamic attributes#161645
Closed
morrison-turnansky wants to merge 16 commits intopytorch:mainfrom
Closed
NamedTuple: Allow side effects for dynamic attributes#161645morrison-turnansky wants to merge 16 commits intopytorch:mainfrom
morrison-turnansky wants to merge 16 commits intopytorch:mainfrom
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/161645
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 7068799 with merge base 6737e2c ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Collaborator
Author
|
@pytorchbot label "topic: not user facing" |
anijain2305
requested changes
Aug 27, 2025
anijain2305
reviewed
Aug 28, 2025
anijain2305
reviewed
Aug 28, 2025
anijain2305
reviewed
Aug 28, 2025
Contributor
There was a problem hiding this comment.
If supporting NamedTuple creation inside the compiled frame and then returning is hard, we can do a graph break and not support it in this PR.
Let me know - this PR is little more complicated than I anticipated because most of the code around tuples is written with immutability in mind, and named tuple creation in CPython is not a standard process (CPython generates some string, and then eval it on the fly), so using a UserDefinedObjectVariable is also not readily available.
Collaborator
Author
|
If these changes are close to what you wanted, I am good to keep going. Otherwise, we can introduce a graph break. |
anijain2305
reviewed
Sep 2, 2025
anijain2305
reviewed
Sep 2, 2025
anijain2305
requested changes
Sep 2, 2025
Contributor
anijain2305
left a comment
There was a problem hiding this comment.
Very close to be done. Thanks for sending the PR.
b81dfd5 to
fed34a7
Compare
…ned variable call function. raised exception for converting dynamic attribute of named tuple variable to python constant when variable does not have to python constant method
a37a275 to
e9b7abd
Compare
Contributor
|
Trigger the CI workflows. |
Collaborator
Author
|
@anijain2305 Please review and/or resolve requested change. |
Contributor
|
Looks great, merging, thank you! |
StrongerXi
approved these changes
Sep 9, 2025
Contributor
|
@pytorchbot merge |
|
This PR has pending changes requested. Please address the comments and update the PR before merging. |
anijain2305
approved these changes
Sep 9, 2025
Contributor
|
@pytorchbot merge |
Collaborator
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 |
markc-614
pushed a commit
to markc-614/pytorch
that referenced
this pull request
Sep 17, 2025
I confirmed that the tracing was correct i.e. NamedTupleVariable had the correct dynamic attribute added to it. The problem was that NamedTupleVariable was always marked as immutable. This does not reflect the behavior of namedtuple. Subclasses of namedtuple may be mutable, so when a NamedTupleVariable is derived from a subclass that is mutable, I made NamedTupleVariable mutable as well. Then side_effects correctly updates the returned object. Fixes pytorch#161610 Pull Request resolved: pytorch#161645 Approved by: https://github.com/anijain2305, https://github.com/StrongerXi
mansiag05
pushed a commit
to mansiag05/pytorch
that referenced
this pull request
Sep 22, 2025
I confirmed that the tracing was correct i.e. NamedTupleVariable had the correct dynamic attribute added to it. The problem was that NamedTupleVariable was always marked as immutable. This does not reflect the behavior of namedtuple. Subclasses of namedtuple may be mutable, so when a NamedTupleVariable is derived from a subclass that is mutable, I made NamedTupleVariable mutable as well. Then side_effects correctly updates the returned object. Fixes pytorch#161610 Pull Request resolved: pytorch#161645 Approved by: https://github.com/anijain2305, https://github.com/StrongerXi
cleonard530
pushed a commit
to cleonard530/pytorch
that referenced
this pull request
Sep 22, 2025
I confirmed that the tracing was correct i.e. NamedTupleVariable had the correct dynamic attribute added to it. The problem was that NamedTupleVariable was always marked as immutable. This does not reflect the behavior of namedtuple. Subclasses of namedtuple may be mutable, so when a NamedTupleVariable is derived from a subclass that is mutable, I made NamedTupleVariable mutable as well. Then side_effects correctly updates the returned object. Fixes pytorch#161610 Pull Request resolved: pytorch#161645 Approved by: https://github.com/anijain2305, https://github.com/StrongerXi
dsashidh
pushed a commit
to dsashidh/pytorch
that referenced
this pull request
Sep 26, 2025
I confirmed that the tracing was correct i.e. NamedTupleVariable had the correct dynamic attribute added to it. The problem was that NamedTupleVariable was always marked as immutable. This does not reflect the behavior of namedtuple. Subclasses of namedtuple may be mutable, so when a NamedTupleVariable is derived from a subclass that is mutable, I made NamedTupleVariable mutable as well. Then side_effects correctly updates the returned object. Fixes pytorch#161610 Pull Request resolved: pytorch#161645 Approved by: https://github.com/anijain2305, https://github.com/StrongerXi
pytorchmergebot
pushed a commit
that referenced
this pull request
Nov 20, 2025
…Variable (#167468) Continuation of work from previous PR, see link for context #161645 (comment) I think this PR is a step in that direction. There is probably some room for simplification. At a high level, the new class NamedTupleVariable handles methods that branch on structseq or the more dynamic subclasses of namedtuple, and falls back to UserDefinedTupleVariable otherwise. Please let me know what you think. @StrongerXi Pull Request resolved: #167468 Approved by: https://github.com/guilhermeleobas, https://github.com/StrongerXi, https://github.com/mlazos
JacobSzwejbka
pushed a commit
that referenced
this pull request
Dec 8, 2025
…Variable (#167468) Continuation of work from previous PR, see link for context #161645 (comment) I think this PR is a step in that direction. There is probably some room for simplification. At a high level, the new class NamedTupleVariable handles methods that branch on structseq or the more dynamic subclasses of namedtuple, and falls back to UserDefinedTupleVariable otherwise. Please let me know what you think. @StrongerXi Pull Request resolved: #167468 Approved by: https://github.com/guilhermeleobas, https://github.com/StrongerXi, https://github.com/mlazos
pytorchmergebot
pushed a commit
that referenced
this pull request
Dec 12, 2025
…Variable (#167468) Continuation of work from previous PR, see link for context #161645 (comment) I think this PR is a step in that direction. There is probably some room for simplification. At a high level, the new class NamedTupleVariable handles methods that branch on structseq or the more dynamic subclasses of namedtuple, and falls back to UserDefinedTupleVariable otherwise. Please let me know what you think. @StrongerXi Pull Request resolved: #167468 Approved by: https://github.com/guilhermeleobas, https://github.com/Lucaskabela
vishalgoyal316
pushed a commit
to vishalgoyal316/pytorch
that referenced
this pull request
Dec 17, 2025
…Variable (pytorch#167468) Continuation of work from previous PR, see link for context pytorch#161645 (comment) I think this PR is a step in that direction. There is probably some room for simplification. At a high level, the new class NamedTupleVariable handles methods that branch on structseq or the more dynamic subclasses of namedtuple, and falls back to UserDefinedTupleVariable otherwise. Please let me know what you think. @StrongerXi Pull Request resolved: pytorch#167468 Approved by: https://github.com/guilhermeleobas, https://github.com/Lucaskabela
krastogi-in
pushed a commit
to krastogi-in/pytorch
that referenced
this pull request
Jan 9, 2026
…Variable (pytorch#167468) Continuation of work from previous PR, see link for context pytorch#161645 (comment) I think this PR is a step in that direction. There is probably some room for simplification. At a high level, the new class NamedTupleVariable handles methods that branch on structseq or the more dynamic subclasses of namedtuple, and falls back to UserDefinedTupleVariable otherwise. Please let me know what you think. @StrongerXi Pull Request resolved: pytorch#167468 Approved by: https://github.com/guilhermeleobas, https://github.com/Lucaskabela
pytorchmergebot
pushed a commit
that referenced
this pull request
Jan 13, 2026
…Variable (#167468) Continuation of work from previous PR, see link for context #161645 (comment) I think this PR is a step in that direction. There is probably some room for simplification. At a high level, the new class NamedTupleVariable handles methods that branch on structseq or the more dynamic subclasses of namedtuple, and falls back to UserDefinedTupleVariable otherwise. Please let me know what you think. @StrongerXi Pull Request resolved: #167468 Approved by: https://github.com/Lucaskabela
mattteochen
pushed a commit
to mattteochen/pytorch
that referenced
this pull request
Jan 15, 2026
…Variable (pytorch#167468) Continuation of work from previous PR, see link for context pytorch#161645 (comment) I think this PR is a step in that direction. There is probably some room for simplification. At a high level, the new class NamedTupleVariable handles methods that branch on structseq or the more dynamic subclasses of namedtuple, and falls back to UserDefinedTupleVariable otherwise. Please let me know what you think. @StrongerXi Pull Request resolved: pytorch#167468 Approved by: https://github.com/Lucaskabela
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I confirmed that the tracing was correct i.e. NamedTupleVariable had the correct dynamic attribute added to it.
The problem was that NamedTupleVariable was always marked as immutable. This does not reflect the behavior of namedtuple.
Subclasses of namedtuple may be mutable, so when a NamedTupleVariable is derived from a subclass that is mutable, I made NamedTupleVariable mutable as well. Then side_effects correctly updates the returned object.
Fixes #161610
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames @Lucaskabela