-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Turn on new tiling by default #154768
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
Turn on new tiling by default #154768
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/154768
Note: Links to docs will display an error until the docs builds have been completed. ⏳ 1 Pending, 1 Unrelated FailureAs of commit 744b0fb with merge base 4405dc1 ( FLAKY - The following job failed but was likely due to flakiness present on trunk:
UNSTABLE - The following job is marked as unstable, possibly due to flakiness on trunk:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
@pytorchbot merge -f "timing out" |
Can't merge closed PR #154768 |
@pytorchbot revert -m "Looks like it broke inductor CPU, see https://hud.pytorch.org/hud/pytorch/pytorch/231eb9902ba78a4ef70203243058f3c7c0ced15d/1?per_page=50&name_filter=inductor-triton-cpu&mergeEphemeralLF=true" -c nosignal |
@pytorchbot successfully started a revert job. Check the current status here. |
@eellison your PR has been successfully reverted. |
This reverts commit 7dcc77e. Reverted #154768 on behalf of https://github.com/malfet due to Looks like it broke inductor CPU, see https://hud.pytorch.org/hud/pytorch/pytorch/231eb9902ba78a4ef70203243058f3c7c0ced15d/1?per_page=50&name_filter=inductor-triton-cpu&mergeEphemeralLF=true ([comment](#154768 (comment)))
could we get inductor cpu in tests ? |
@pytorchbot merge |
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 |
Pull Request resolved: #155342 Approved by: https://github.com/Skylion007, https://github.com/bdhirsh ghstack dependencies: #154768
Stack from ghstack (oldest at bottom):
Turning on in fbcode to come. Also updates
max_tiles
to have a default value of None. The existing tiling logic doesn't really handle max_tiles=3 well, but we do in the new tiling logic, so we default to 3 in the new logic and 2 elsewhere unless max_tiles has been explicitly set.TB runners have been very unstable recently (do we need to bump batch size ?) but e.g. for a recent torchbench inference run we had 15 models with a lower execution time (i.g. green) and 2 models with higher (i.e.. red)
I am doing another run and will update here.
Dynamic shapes is not yet turned on because there are a lot of fixes to be done in splitting that don't work yet.. See:
and also - unbacked shape is not multiple of itself.
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @chenyang78 @kadeng @muchulee8 @amjames @chauhang @aakhundov