Skip to content

Conversation

@c00w
Copy link
Contributor

@c00w c00w commented Jan 17, 2025

Stack from ghstack (oldest at bottom):

If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

Note that I couldn't figure out how to test this, so advice would be awesome. I have my best case attempt at #145799, but it doesn't seem to reproduce the crash.

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

… attribute

If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Jan 17, 2025

🔗 Helpful Links

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

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

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit 2017811 with merge base 53759cc (image):
💚 Looks good so far! There are no failures yet. 💚

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

…s missing a attribute"

If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

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

[ghstack-poisoned]
c00w added a commit that referenced this pull request Jan 17, 2025
… attribute

If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

ghstack-source-id: 2db6660
Pull Request resolved: #145122
@pytorch-bot pytorch-bot bot temporarily deployed to upload-benchmark-results January 18, 2025 00:21 Inactive
@pytorch-bot pytorch-bot bot temporarily deployed to upload-benchmark-results January 18, 2025 00:21 Inactive
@c00w c00w added the topic: not user facing topic category label Jan 25, 2025
…s missing a attribute"

If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

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

[ghstack-poisoned]
c00w added a commit that referenced this pull request Jan 25, 2025
… attribute

If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

ghstack-source-id: c75fa7c
Pull Request resolved: #145122
@pytorch-bot pytorch-bot bot temporarily deployed to upload-benchmark-results January 25, 2025 01:24 Inactive
@pytorch-bot pytorch-bot bot temporarily deployed to upload-benchmark-results January 25, 2025 01:24 Inactive
…missing a attribute"

If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

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

[ghstack-poisoned]
…s missing a attribute"

If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

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

[ghstack-poisoned]
@c00w c00w marked this pull request as ready for review January 27, 2025 23:48
…s missing a attribute"


If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

Note that I couldn't figure out how to test this, so advice would be awesome.  I have my best case attempt at  #145799, but it doesn't seem to reproduce the crash.

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

[ghstack-poisoned]
@c00w c00w requested review from eellison and jansel January 27, 2025 23:50
…s missing a attribute"


If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

Note that I couldn't figure out how to test this, so advice would be awesome.  I have my best case attempt at  #145799, but it doesn't seem to reproduce the crash.

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

[ghstack-poisoned]
c00w added a commit that referenced this pull request Jan 27, 2025
… attribute

If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

ghstack-source-id: be26a74
Pull Request resolved: #145122
@pytorch-bot pytorch-bot bot temporarily deployed to upload-benchmark-results January 28, 2025 00:22 Inactive
@pytorch-bot pytorch-bot bot temporarily deployed to upload-benchmark-results January 28, 2025 00:22 Inactive
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.

Can you add a test for this?

@c00w
Copy link
Contributor Author

c00w commented Feb 4, 2025

Update - we depend on attribute error being raised in this code to support certain models of execution. My plan is to raise a UnimplementedAttributeError, so that we can catch this "correctly", while still not breaking compilation if we cannot.

…s missing a attribute"


If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

Note that I couldn't figure out how to test this, so advice would be awesome.  I have my best case attempt at  #145799, but it doesn't seem to reproduce the crash.

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

[ghstack-poisoned]
@c00w c00w requested a review from jansel February 4, 2025 22:12
…s missing a attribute"


If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

Note that I couldn't figure out how to test this, so advice would be awesome.  I have my best case attempt at  #145799, but it doesn't seem to reproduce the crash.

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

[ghstack-poisoned]
…s missing a attribute"


If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

Note that I couldn't figure out how to test this, so advice would be awesome.  I have my best case attempt at  #145799, but it doesn't seem to reproduce the crash.

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

[ghstack-poisoned]
@c00w
Copy link
Contributor Author

c00w commented Feb 5, 2025

Chatted with @anijain2305 switching to raise_observed_exception

…s missing a attribute"


If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

Note that I couldn't figure out how to test this, so advice would be awesome.  I have my best case attempt at  #145799, but it doesn't seem to reproduce the crash.

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

[ghstack-poisoned]
@c00w
Copy link
Contributor Author

c00w commented Feb 5, 2025

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

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

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

…s missing a attribute"


If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

Note that I couldn't figure out how to test this, so advice would be awesome.  I have my best case attempt at  #145799, but it doesn't seem to reproduce the crash.

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

[ghstack-poisoned]
c00w added a commit that referenced this pull request Feb 5, 2025
… attribute

If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

ghstack-source-id: b8cc6b2
Pull Request resolved: #145122
@c00w
Copy link
Contributor Author

c00w commented Feb 5, 2025

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

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

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

If a nn.module getattr call throws, we should make sure that we don't crash with an internal error

ghstack-source-id: d26a8c8
Pull Request resolved: pytorch/pytorch#145122
@github-actions github-actions bot deleted the gh/c00w/30/head branch March 8, 2025 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-no-td Do not run TD on this PR ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request Merged module: dynamo Reverted topic: not user facing topic category

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants