10000 [Dynamo][pytree] handle `isinstance(...)` check for polyfilled class by XuehaiPan · Pull Request #146921 · pytorch/pytorch · GitHub
[go: up one dir, main page]
Skip to content

[Dynamo][pytree] handle isinstance(...) check for polyfilled class #146921

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 8 commits into from
Closed

[Dynamo][pytree] handle isinstance(...) check for polyfilled class #146921

wants to merge 8 commits into from

Conversation

XuehaiPan
Copy link
Collaborator
@XuehaiPan XuehaiPan commented Feb 11, 2025 •
edited
Loading

@XuehaiPan
Update
d5d05d0 
[ghstack-poisoned]
@pytorch-bot PyTorch Bot
Copy link
pytorch-bot bot commented Feb 11, 2025 •
edited
Loading

🔗 Helpful Links

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

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

❌ 2 New Failures

As of commit 72d44f2 with merge base 1677a31 (image):

NEW FAILURES - The following jobs have failed:

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

XuehaiPan added a commit that referenced this pull request Feb 11, 2025
@XuehaiPan
[Dynamo][pytree] handle isinstance(...) check for polyfilled class
5de9922 
ghstack-source-id: dd5897e
Pull Request resolved: #146921
@XuehaiPan XuehaiPan requested review from jansel, anijain2305 and zou3519 and removed request for jansel February 11, 2025 18:38
@XuehaiPan XuehaiPan mentioned this pull request Feb 11, 2025
Closed
@XuehaiPan
Update
9e78423 
[ghstack-poisoned]
XuehaiPan added a commit that referenced this pull request Feb 11, 2025
@XuehaiPan
[Dynamo][pytree] handle isinstance(...) check for polyfilled class
03846bc 
ghstack-source-id: 7f2037a
Pull Request resolved: #146921
zou3519
zou3519 reviewed Feb 11, 2025
View reviewed changes
test/dynamo/test_misc.py
x = [1, [2, [3, 4]]]
leaves, treespec, _ = fn(x, y)
# Compiled function returns an instance of the polyfilled class instead of the original class
self.assertIsInstance(treespec, polyfilled_cxx_pytree.PyTreeSpec)
Copy link
Contributor

Choose a reason for hiding this comment

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

My point is, it is wrong for the compiled function to return an instance of the polyfilled class instead of the original class. The compiled function needs to return an instance of the original class.

Copy link
Contributor

Choose a reason for hiding this comment

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

We either need Animesh's polyfill infra for classes, or we need to actually make a TreeSpecVariable in Dynamo that has a reconstruct method. I'd prefer hardening the polyfill infra for classes because that is more generically applicable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The compiled function needs to return an instance of the original class.

That is the ideal solution for polyfilling a class. We need to find a way to batch register the Python version of the polyfill methods of the C++ class. cc @anijain2305 about the class polyfill infra design.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After a second thought, I think polyfilling the methods and using a variable tracker of instance original class during inlining will cause performance issues. Also, it is not easy to polyfill C++ descriptors and support the pybind11 property and read-only property.

The compiled function needs to return an instance of the original class.

We should use the polyfilled class object while inlining the graph and find a way to convert between the original/polyfilled class instances at the graph boundaries.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should use the polyfilled class object while inlining the graph and find a way to convert between the original/polyfilled class instances at the graph boundaries.

Yes

@XuehaiPan
Update
468d321 
[ghstack-poisoned]
XuehaiPan added a commit that referenced this pull request Feb 11, 2025
@XuehaiPan
[Dynamo][pytree] handle isinstance(...) check for polyfilled class
116dc33 
ghstack-source-id: f392bfe
Pull Request resolved: #146921
@XuehaiPan
Update
eba607c 
[ghstack-poisoned]
XuehaiPan added a commit that referenced this pull request Feb 11, 2025
@XuehaiPan
[Dynamo][pytree] handle isinstance(...) check for polyfilled class
dcf199d 
ghstack-source-id: 35cfbc4
Pull Request resolved: #146921
jansel
jansel requested changes Feb 11, 2025
View reviewed changes
Copy link
Contributor
@jansel jansel left a comment

Choose a reason for hiding this comment

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

I agree with @zou3519's comments. Polyfill types should not leak into user code.

XuehaiPan added a commit to XuehaiPan/pytorch that referenced this pull request Feb 12, 2025
@XuehaiPan
[Dynamo][pytree] handle isinstance(...) check for polyfilled class
c216ea0 
ghstack-source-id: 35cfbc4
Pull Request resolved: pytorch#146921
@XuehaiPan
Update
837db58 
[ghstack-poisoned]
@XuehaiPan XuehaiPan mentioned this pull request Feb 12, 2025
Closed
XuehaiPan added a commit that referenced this pull request Feb 12, 2025
@XuehaiPan
[Dynamo][pytree] handle isinstance(...) check for polyfilled class
30761ca 
ghstack-source-id: 0246bf1
Pull Request resolved: #146921
@XuehaiPan
Update
f417298 
[ghstack-poisoned]
XuehaiPan added a commit that referenced this pull request Feb 12, 2025
@XuehaiPan
[Dynamo][pytree] handle isinstance(...) check for polyfilled class
8bbb190 
ghstack-source-id: 0246bf1
Pull Request resolved: #146921
XuehaiPan added a commit to XuehaiPan/pytorch that referenced this pull request Feb 12, 2025
@XuehaiPan
[Dynamo][pytree] handle isinstance(...) check for polyfilled class
b3497a1 
ghstack-source-id: 0246bf1
Pull Request resolved: pytorch#146921
@XuehaiPan
Update
ac62c83 
[ghstack-poisoned]
XuehaiPan added a commit that referenced this pull request Feb 12, 2025
@XuehaiPan
[Dynamo][pytree] handle isinstance(...) check for polyfilled class
e1741e3 
ghstack-source-id: c2486d4
Pull Request resolved: #146921
@XuehaiPan
Update
72d44f2 
[ghstack-poisoned]
XuehaiPan added a commit that referenced this pull request Feb 16, 2025
@XuehaiPan
[Dynamo][pytree] handle isinstance(...) check for polyfilled class
f231662 
ghstack-source-id: f0e34ee
Pull Request resolved: #146921
XuehaiPan added a commit to XuehaiPan/pytorch that referenced this pull request Mar 3, 2025
@XuehaiPan
[Dynamo][pytree] handle isinstance(...) check for polyfilled class
03ac54a 
ghstack-source-id: f0e34ee
Pull Request resolved: pytorch#146921
@github-actions GitHub Actions
Copy link
Contributor

Looks like this PR hasn't been updated in a while so we're going to go ahead and mark this as Stale.
Feel free to remove the Stale label if you feel this was a mistake.
If you are unable to remove the Stale label please contact a maintainer in order to do so.
If you want the bot to never mark this PR stale again, add the no-stale label.
Stale pull requests will automatically be closed after 30 days of inactivity.

@github-actions github-actions bot added the Stale label Apr 17, 2025
@XuehaiPan XuehaiPan closed this Apr 19, 2025
@github-actions github-actions bot deleted the gh/XuehaiPan/242/head branch May 25, 2025 02:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zou3519 zou3519 zou3519 left review comments

@jansel jansel jansel requested changes

@anijain2305 anijain2305 Awaiting requested review from anijain2305

Assignees
No one assigned
Labels
ciflow/inductor module: dynamo module: pytree open source Stale topic: not user facing topic category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@XuehaiPan @jansel @zou3519 @pytorchbot
0