-
Notifications
You must be signed in to change notification settings - Fork 25.7k
Fixes for collections.NamedTuple
#159367
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
Fixes for collections.NamedTuple
#159367
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/159367
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (2 Unrelated Failures)As of commit 6b959c2 with merge base e299926 ( FLAKY - The following job failed but was likely due to flakiness present on trunk:
BROKEN TRUNK - The following job 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. |
|
test? Especially for the |
torch/_dynamo/variables/lists.py
Outdated
| return None | ||
|
|
||
| if name == "_fields": | ||
| return VariableTracker.build(tx, self.fields()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have a source for this, or else it will use sourceless builder
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually have DataclassFieldsSource all ready for you :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried and it doesn't work. I think I need to implement my own NamedTupleFieldsSource.
[33] > ~/micromamba/envs/pytorch313/lib/python3.13/dataclasses.py(1319)
....
1308 def fields(class_or_instance):
1309 """Return a tuple describing the fields of this dataclass.
1310
1311 Accepts a dataclass or an instance of one. Tuple elements are of
1312 type Field.
1313 """
1314
1315 # Might it be worth caching this, per class?
1316 try:
1317 fields = getattr(class_or_instance, _FIELDS)
1318 except AttributeError:
1319 -> breakpoint()
1320 raise TypeError('must be called with a dataclass type or instance') from None
1321
1322 # Exclude pseudo-fields. Note that fields is sorted by insertion
1323 # order, so the order of the tuple is as the fields were defined.
1324 return tuple(f for f in fields.values() if f._field_type is _FIELD)
(Pdb+) class_or_instance
MyTuple(a=tensor([1.]), b=tensor([2.]), ab=tensor([3.]))
(Pdb+) _FIELDS
'__dataclass_fields__'
(Pdb+) getattr(class_or_instance, _FIELDS)
*** AttributeError: 'MyTuple' object has no attribute '__dataclass_fields__'There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, yeah I think I was tired and thought these were compatible, implementing your own source will work. You can follow my PR implementing the DataclassFieldsSource if it helps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added. If you could take a look.
|
@pytorchbot merge |
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 |
Pull Request resolved: pytorch#159367 Approved by: https://github.com/mlazos ghstack dependencies: pytorch#159365, pytorch#159366, pytorch#159368, pytorch#159483, pytorch#159902, pytorch#159864, pytorch#159865
Pull Request resolved: pytorch#159367 Approved by: https://github.com/mlazos ghstack dependencies: pytorch#159365, pytorch#159366, pytorch#159368, pytorch#159483, pytorch#159902, pytorch#159864, pytorch#159865
Stack from ghstack (oldest at bottom):
collections.NamedTuple#159367list(UserDefinedObject)viaforce_unpack_var_sequence#159864set_fullgraph(False)intest_collections#159902cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @chenyang78 @kadeng @chauhang @amjames @Lucaskabela @mlazos