Re-enable FakeTensor caching for SymInts #152662
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. Tests seem to pass so I'm assuming some dynamic shape work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they seem to pass. [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/152662
Note: Links to docs will display an error until the docs builds have been completed. ✅ You can merge normally! (1 Unrelated Failure)As of commit c5661a7 with merge base b394c6e ( FLAKY - The following job failed but was likely due to flakiness present on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
aorenste
added a commit
that referenced
this pull request
May 2, 2025
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. Tests seem to pass so I'm assuming some dynamic shape work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they seem to pass. ghstack-source-id: 870317f Pull Request resolved: #152662
aorenste
added a commit
that referenced
this pull request
May 2, 2025
…key" ShapeEnv.evaluate_expr() behaves differently based on the (tls) global "suppress_guards" - so its cache key needs to include that value. This came up because #152662 triggered it in the test `test/dynamo/test_exc.py::ExcTests::test_trigger_bisect_on_error` - fixing this caused that test to work again. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv [ghstack-poisoned]
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. Tests seem to pass so I'm assuming some dynamic shape work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they seem to pass. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 2, 2025
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. Tests seem to pass so I'm assuming some dynamic shape work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they seem to pass. ghstack-source-id: 0dc4211 Pull Request resolved: #152662
aorenste
added a commit
that referenced
this pull request
May 2, 2025
…ds_tls in cache key" ShapeEnv.evaluate_expr() behaves differently based on the (tls) global "suppress_guards" - so its cache key needs to include that value. This came up because #152662 triggered it in the test `test/dynamo/test_exc.py::ExcTests::test_trigger_bisect_on_error` - fixing this caused that test to work again. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 2, 2025
…key" ShapeEnv.evaluate_expr() behaves differently based on the (tls) global "suppress_guards" - so its cache key needs to include that value. This came up because #152662 triggered it in the test `test/dynamo/test_exc.py::ExcTests::test_trigger_bisect_on_error` - fixing this caused that test to work again. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv [ghstack-poisoned]
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. Tests seem to pass so I'm assuming some dynamic shape work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they seem to pass. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 2, 2025
…ds_tls in cache key" ShapeEnv.evaluate_expr() behaves differently based on the (tls) global "suppress_guards" - so its cache key needs to include that value. This came up because #152662 triggered it in the test `test/dynamo/test_exc.py::ExcTests::test_trigger_bisect_on_error` - fixing this caused that test to work again. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 2, 2025
…key" ShapeEnv.evaluate_expr() behaves differently based on the (tls) global "suppress_guards" - so its cache key needs to include that value. This came up because #152662 triggered it in the test `test/dynamo/test_exc.py::ExcTests::test_trigger_bisect_on_error` - fixing this caused that test to work again. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 2, 2025
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. Tests seem to pass so I'm assuming some dynamic shape work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they seem to pass. ghstack-source-id: 0dc4211 Pull Request resolved: #152662
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. Tests seem to pass so I'm assuming some dynamic shape work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they seem to pass. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 2, 2025
…ds_tls in cache key" ShapeEnv.evaluate_expr() behaves differently based on the (tls) global "suppress_guards" - so its cache key needs to include that value. This came up because #152662 triggered it in the test `test/dynamo/test_exc.py::ExcTests::test_trigger_bisect_on_error` - fixing this caused that test to work again. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 2, 2025
…key" ShapeEnv.evaluate_expr() behaves differently based on the (tls) global "suppress_guards" - so its cache key needs to include that value. This came up because #152662 triggered it in the test `test/dynamo/test_exc.py::ExcTests::test_trigger_bisect_on_error` - fixing this caused that test to work again. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 2, 2025
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. Tests seem to pass so I'm assuming some dynamic shape work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they seem to pass. ghstack-source-id: 35ead1d Pull Request resolved: #152662
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. Tests seem to pass so I'm assuming some dynamic shape work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they seem to pass. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 5, 2025
…ds_tls in cache key" ShapeEnv.evaluate_expr() behaves differently based on the (tls) global "suppress_guards" - so its cache key needs to include that value. This came up because #152662 triggered it in the test `test/dynamo/test_exc.py::ExcTests::test_trigger_bisect_on_error` - fixing this caused that test to work again. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 5, 2025
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. Tests seem to pass so I'm assuming some dynamic shape work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they seem to pass. ghstack-source-id: f9927a5 Pull Request resolved: #152662
aorenste
added a commit
that referenced
this pull request
May 5, 2025
…key" ShapeEnv.evaluate_expr() behaves differently based on the (tls) global "suppress_guards" - so its cache key needs to include that value. This came up because #152662 triggered it in the test `test/dynamo/test_exc.py::ExcTests::test_trigger_bisect_on_error` - fixing this caused that test to work again. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 7, 2025
…ds_tls in cache key" ShapeEnv.evaluate_expr() behaves differently based on the (tls) global "suppress_guards" - so its cache key needs to include that value. This came up because #152662 triggered it in the test `test/dynamo/test_exc.py::ExcTests::test_trigger_bisect_on_error` - fixing this caused that test to work again. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 7, 2025
…key" ShapeEnv.evaluate_expr() behaves differently based on the (tls) global "suppress_guards" - so its cache key needs to include that value. This came up because #152662 triggered it in the test `test/dynamo/test_exc.py::ExcTests::test_trigger_bisect_on_error` - fixing this caused that test to work again. cc ezyang SherlockNoMad EikanWang jgong5 wenzhe-nrv voznesenskym penguinwu Guobing-Chen XiaobingSuper zhuhaozhe blzheng jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]
pytorchmergebot
pushed a commit
that referenced
this pull request
May 8, 2025
ShapeEnv.evaluate_expr() behaves differently based on the (tls) global "suppress_guards" - so its cache key needs to include that value. This came up because #152662 triggered it in the test `test/dynamo/test_exc.py::ExcTests::test_trigger_bisect_on_error` - fixing this caused that test to work again. Pull Request resolved: #152661 Approved by: https://github.com/laithsakka
anijain2305
approved these changes
May 9, 2025
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. Tests seem to pass so I'm assuming some dynamic shape work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they seem to pass. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx chenyang78 kadeng chauhang amjames [ghstack-poisoned]
pytorch-bot bot
pushed a commit
that referenced
this pull request
May 27, 2025
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. There has been a lot of dynamic shape fixes done this year and tests pass so I'm assuming some of that work fixed what was breaking previously. Approved by: https://github.com/anijain2305 Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/7d11c61c26c596076613aa0111892f7cbccae32e Test plan from GitHub: Reran the tests listed in T196779132 and they pass. ## Perf ### Instruction Counter Benchmark: - 26% win on add_loop_eager_dynamic - 13% win on add_loop_inductor_dynamic_gpu ### Perf Dashboard Compilation Latency wins across the board but especially strong on the dynamic tests (like cudagraphs_dynamic) - for example MobileBertForMaskedLM went from 66s -> 50s. Differential Revision: D75413218
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. There has been a lot of dynamic shape fixes done this year and tests pass so I'm assuming some of that work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they pass. ## Perf ### Instruction Counter Benchmark: - 26% win on add_loop_eager_dynamic - 13% win on add_loop_inductor_dynamic_gpu ### Perf Dashboard Compilation Latency wins across the board but especially strong on the dynamic tests (like cudagraphs_dynamic) - for example MobileBertForMaskedLM went from 66s -> 50s. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 27, 2025
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. Tests seem to pass so I'm assuming some dynamic shape work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they seem to pass. ghstack-source-id: 632955c Pull Request resolved: #152662
Contributor
Author
|
@aorenste has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Contributor
Author
|
@pytorchbot merge |
Collaborator
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 |
Collaborator
Merge failedReason: Command Details for Dev Infra teamRaised by workflow job |
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. There has been a lot of dynamic shape fixes done this year and tests pass so I'm assuming some of that work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they pass. ## Perf ### Instruction Counter Benchmark: - 26% win on add_loop_eager_dynamic - 13% win on add_loop_inductor_dynamic_gpu ### Perf Dashboard Compilation Latency wins across the board but especially strong on the dynamic tests (like cudagraphs_dynamic) - for example MobileBertForMaskedLM went from 66s -> 50s. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov Differential Revision: [D75467694](https://our.internmc.facebook.com/intern/diff/D75467694) [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 28, 2025
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. Tests seem to pass so I'm assuming some dynamic shape work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they seem to pass. ghstack-source-id: e3e9c8a Pull Request resolved: #152662
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. There has been a lot of dynamic shape fixes done this year and tests pass so I'm assuming some of that work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they pass. ## Perf ### Instruction Counter Benchmark: - 26% win on add_loop_eager_dynamic - 13% win on add_loop_inductor_dynamic_gpu ### Perf Dashboard Compilation Latency wins across the board but especially strong on the dynamic tests (like cudagraphs_dynamic) - for example MobileBertForMaskedLM went from 66s -> 50s. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov Differential Revision: [D75467694](https://our.internmc.facebook.com/intern/diff/D75467694) [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 28, 2025
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. Tests seem to pass so I'm assuming some dynamic shape work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they seem to pass. ghstack-source-id: 424b672 Pull Request resolved: #152662
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. There has been a lot of dynamic shape fixes done this year and tests pass so I'm assuming some of that work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they pass. ## Perf ### Instruction Counter Benchmark: - 26% win on add_loop_eager_dynamic - 13% win on add_loop_inductor_dynamic_gpu ### Perf Dashboard Compilation Latency wins across the board but especially strong on the dynamic tests (like cudagraphs_dynamic) - for example MobileBertForMaskedLM went from 66s -> 50s. cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx ipiszy chenyang78 kadeng muchulee8 amjames chauhang aakhundov Differential Revision: [D75467694](https://our.internmc.facebook.com/intern/diff/D75467694) [ghstack-poisoned]
aorenste
added a commit
that referenced
this pull request
May 29, 2025
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. Tests seem to pass so I'm assuming some dynamic shape work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they seem to pass. ghstack-source-id: 992c1d9 Pull Request resolved: #152662
yushangdi
reviewed
May 29, 2025
| # with self.assertRaisesRegex( | ||
| # RuntimeError, escape("Could not guard on data-dependent expression") | ||
| # ): | ||
| # export(Module(identity), inputs, strict=True) |
Contributor
There was a problem hiding this comment.
Does it make sense to leave line 426 uncommented? i.e. testing that we can export Module(identity) now.
Contributor
Author
There was a problem hiding this comment.
I don't really know what the test is trying to test so I didn't want to make the decision to "lock in" behavior that might be wrong - but if you think I should uncomment it I'm happy to do that.
Contributor
There was a problem hiding this comment.
I don't really know what the test is trying to test so I didn't want to make the decision to "lock in" behavior that might be wrong - but if you think I should uncomment it I'm happy to do that.
commenting out is fine then. Thanks!
Contributor
Author
|
@pytorchbot merge |
Collaborator
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 |
Contributor
|
FYI for some reason this increase the overhead for this benchmark the benchmark is not enabeled so its ok JUST an FYI but it will be interesting to at least know why is that overhead |
Contributor
|
nWEIdia
pushed a commit
to nWEIdia/pytorch
that referenced
this pull request
Jun 2, 2025
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. There has been a lot of dynamic shape fixes done this year and tests pass so I'm assuming some of that work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they pass. ## Perf ### Instruction Counter Benchmark: - 26% win on add_loop_eager_dynamic - 13% win on add_loop_inductor_dynamic_gpu ### Perf Dashboard Compilation Latency wins across the board but especially strong on the dynamic tests (like cudagraphs_dynamic) - for example MobileBertForMaskedLM went from 66s -> 50s. Differential Revision: [D75467694](https://our.internmc.facebook.com/intern/diff/D75467694) Pull Request resolved: pytorch#152662 Approved by: https://github.com/anijain2305
iupaikov-amd
pushed a commit
to ROCm/pytorch
that referenced
this pull request
Jun 4, 2025
Summary: This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present. There has been a lot of dynamic shape fixes done this year and tests pass so I'm assuming some of that work fixed what was breaking previously. Test Plan: Reran the tests listed in T196779132 and they pass. ## Perf ### Instruction Counter Benchmark: - 26% win on add_loop_eager_dynamic - 13% win on add_loop_inductor_dynamic_gpu ### Perf Dashboard Compilation Latency wins across the board but especially strong on the dynamic tests (like cudagraphs_dynamic) - for example MobileBertForMaskedLM went from 66s -> 50s. Differential Revision: [D75467694](https://our.internmc.facebook.com/intern/diff/D75467694) Pull Request resolved: pytorch#152662 Approved by: https://github.com/anijain2305
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Stack from ghstack (oldest at bottom):
Summary:
This backs out D60320595 which itself turned off FakeTensor caching when a SymInt was present.
There has been a lot of dynamic shape fixes done this year and tests pass so I'm assuming some of that work fixed what was breaking previously.
Test Plan: Reran the tests listed in T196779132 and they pass.
Perf
Instruction Counter Benchmark:
Perf Dashboard
Compilation Latency wins across the board but especially strong on the dynamic tests (like cudagraphs_dynamic) - for example MobileBertForMaskedLM went from 66s -> 50s.
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @chenyang78 @kadeng @muchulee8 @amjames @chauhang @aakhundov
Differential Revision: D75467694