-
Notifications
You must be signed in to change notification settings - Fork 505
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
Reset Dynamo at Setup for all tests #9561
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/9561
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 1 PendingAs of commit 5417019 with merge base 5c5b84e ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
@mcr229 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
any side effects from dynamo? Also I guess stuff like inheritance or decorator is just confusing? |
@mcr229 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
why is this recompiling in the first place? |
We have a for loop exporting the same model for different configurations multiple times. I think the issue lies there where we are "re-exporting" the same model. So it seems like we need to reset dynamo after each export. It is strange though because in each loop we are technically initializing a new module, I'm guessing there is some internal cache in dynamo that recognizes its the same model structure, and tries to recompile instead. |
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.
Nit: It might be nice to define a common test fixture for models + ops. If this fixes it, maybe we can create a "good first issue"-tagged issue to get help doing the refactoring.
@tugsbayasgalan do you know if this is the right change? |
I am not really sure, but i know torchdynamo test cases do the same thing due to some caching stuff. Maybe cc: @anijain2305 knows better |
yea i was thinking that, but i didn't want to add another abstraction we maintain just for _dynamo.reset(). I guess in the future could be useful. |
trunk / test-models-macos (llama2, xnnpack-quantization-delegation) / macos-job (push) seems to be flaky and have intermittent failures. This PR likely doesn't have anything to do with that failure. Will follow up on that flaky test after landing this PR. |
From latest viable/strict: https://hud.pytorch.org/hud/pytorch/executorch/viable%2Fstrict/1?per_page=50 Fixes #144480 This commit has important CI stability fixes, such as pytorch/executorch#9561 and pytorch/executorch#9634 Pull Request resolved: #150308 Approved by: https://github.com/jathu, https://github.com/malfet
https://github.com/pytorch/executorch/actions/runs/14047575373/job/39331644423
There seems to be some CI issues with:
To help resolve this we reset dynamo at setup for all unittests. Let's see if this helps