Skip to content

[Dynamo] Fix str(Guard.obj_weakref) bug to re-ennable support overriding __getattr__ #88564

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
yanboliang wants to merge 1 commit into from
Closed

[Dynamo] Fix str(Guard.obj_weakref) bug to re-ennable support overriding __getattr__ #88564

yanboliang wants to merge 1 commit into from

Conversation

@yanboliang
Copy link
Contributor

@yanboliang yanboliang commented Nov 7, 2022 •
edited by pytorch-bot bot
Loading

@pytorch-bot
Copy link

pytorch-bot bot commented Nov 7, 2022 •
edited
Loading

🔗 Helpful Links

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

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

❌ 1 Failures

As of commit d56e79d:

The following jobs have failed:

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

@yanboliang yanboliang requested review from ezyang and jansel November 7, 2022 04:34
yanboliang
yanboliang commented Nov 7, 2022
View reviewed changes
torch/_dynamo/guards.py
@staticmethod
def weakref_to_str(obj_weakref):
"""
This is a workaround of a Python weakref bug.
Copy link
Contributor Author

@yanboliang yanboliang Nov 7, 2022

Choose a reason for hiding this comment

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

Opened python/cpython#99184 to track this bug at cpython.

@ezyang
Copy link
Contributor

ezyang commented Nov 7, 2022

cpython devs disagree with the assessment that it's a cpython bug, wdyt? Why are we printing weakrefs in the first place?

@yanboliang
Copy link
Contributor Author

yanboliang commented Nov 7, 2022 •
edited
Loading

cpython devs disagree with the assessment that it's a cpython bug, wdyt?

I responded on that thread.

Why are we printing weakrefs in the first place?

I think this is info and debug purpose.

Overall, I think even cpython devs disagree with this is a cpython bug, we should still have this fix at dynamo. Because we don't expect PyTorch/Dynamo users who override __getattr__ need to take care of handling a lot of python internal stuff like '__name__', '__class__', ..., which is a bad UX.

Update: cpython fix is ready - python/cpython#99244.

@yanboliang yanboliang requested review from anijain2305 and jansel and removed request for jansel November 11, 2022 06:16
ezyang
ezyang approved these changes Nov 11, 2022
View reviewed changes
Copy link
Contributor

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

link the weakref bug and happy to accept

@anijain2305 anijain2305 mentioned this pull request Nov 11, 2022
Closed
anijain2305 added a commit that referenced this pull request Nov 11, 2022
@anijain2305
Not needed once #88564 gets in
17c3bf6
@yanboliang
Copy link
Contributor Author

yanboliang commented Nov 11, 2022

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 11, 2022
@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Nov 11, 2022

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
Copy link
Collaborator

pytorchmergebot commented Nov 11, 2022

Merge failed

Reason: This PR is too stale; the last push date was more than 3 days ago. Please rebase and try again. You can rebase by leaving the following comment on this PR:
@pytorchbot rebase

Details for Dev Infra team Raised by workflow job

@yanboliang
Copy link
Contributor Author

yanboliang commented Nov 11, 2022

@pytorchbot merge -f "irrelevant MacOS errors"

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Nov 11, 2022

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

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

@yanboliang yanboliang deleted the weakref branch November 11, 2022 22:33
pytorchmergebot pushed a commit that referenced this pull request Nov 12, 2022
@anijain2305 @pytorchmergebot
Not needed once #88564 gets in
106954f
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Dec 10, 2022
@yanboliang @kulinseth
[Dynamo] Fix str(Guard.obj_weakref) bug to re-ennable support overrid…
bced4cf 
…ing __getattr__ (pytorch#88564)

See my inline comments!

Pull Request resolved: pytorch#88564
Approved by: https://github.com/ezyang, https://github.com/anijain2305
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ezyang ezyang ezyang approved these changes

@anijain2305 anijain2305 anijain2305 approved these changes

@jansel jansel Awaiting requested review from jansel

Assignees

No one assigned

Labels

ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yanboliang @ezyang @pytorchmergebot @anijain2305