fix unbacked + view incorrectness #145548
Conversation
[ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/145548
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit e2be08b with merge base 639dd54 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx ipiszy yf225 chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire chauhang aakhundov [ghstack-poisoned]
cc voznesenskym penguinwu EikanWang jgong5 Guobing-Chen XiaobingSuper zhuhaozhe blzheng wenzhe-nrv jiayisunx ipiszy yf225 chenyang78 kadeng muchulee8 ColinPeppler amjames desertfire chauhang aakhundov [ghstack-poisoned]
|
I don't understand what these API functions do well enough to review this, but I would be happy to if someone educates me :) |
| # TODO: unbacked should not diverge from backed in determining striding | ||
| # Need to require contiguous here instead of realize, see: | ||
| # https://github.com/pytorch/pytorch/issues/145561 | ||
| x = ExternKernel.require_contiguous(x) |
There was a problem hiding this comment.
This is fine following the existing logic, but I think unbacked_symbols_in_sizes doesn't necessarily mean x should be contiguous, so a better way is to realize x and create different view based on x's strides?
There was a problem hiding this comment.
This will create a contiguous copy if x is not contiguous or having symints? (e.g. an non contiguous ReinterpretView of a contiguous buffer)
There was a problem hiding this comment.
Initially, this branch is operating under the assumption that x is contiguous. Then that was extended to contiguous or has unbacked symints: https://github.com/pytorch/pytorch/pull/110520/files#diff-cf6ca00beddc32a2a6a2933fb9913b6a2b925ffc3b745488967210e4343134acR1735-R1739 bc dynamic_reshape_indexer fails with unbacked. The current api call (want_contiguous) does not enforce that it is contiguous, which I'm fixing in this pr.
I agree, in the future, we should add support for dynamic_reshape_indexer & unbacked symints. I will file an issue for that.
|
@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 |
|
@pytorchbot rebase |
|
@pytorchbot started a rebase job onto refs/remotes/origin/viable/strict. Check the current status here |
|
Successfully rebased |
|
@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 |
Merge failedReason: 1 mandatory check(s) failed. The first few are: Dig deeper by viewing the failures on hud |
|
@pytorchbot merge -i |
Merge startedYour change will be merged while ignoring the following 1 checks: Lint / lintrunner-noclang / linux-job Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
|
The merge job was canceled or timed out. This most often happen if two merge requests were issued for the same PR, or if merge job was waiting for more than 6 hours for tests to finish. In later case, please do not hesitate to reissue the merge command |
|
@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 |
Merge failedReason: 1 mandatory check(s) failed. The first few are: Dig deeper by viewing the failures on hud |
|
@pytorchbot merge -i |
Merge startedYour change will be merged while ignoring the following 1 checks: Lint / lintrunner-noclang / linux-job Learn more about merging in the wiki. Questions? Feedback? Please reach out to the PyTorch DevX Team |
5 participants
Stack from ghstack (oldest at bottom):
fix for #143498
We were incorrectly using contiguous strides for a non-contiguous tensor. There are two separate causes:
Fix size_hint call sites failing on unbacked SymInts #110520 made it so we turn Views contiguous with unbacked symints becuase
dynamic_reshape_indexer below will fail due to the size_hint's inability to process unbacked SymInts. Seems like we should fix. Regardless - it will make the input contiguous if input is unbacked to workaround this.We weren't actually making it contiguous! I filed an issue for this here: Confusing as_storage_and_layout(x, want_contiguous=True) behavior #145561.
This is still worth landing as a fix, even though we should those issues.
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @yf225 @chenyang78 @kadeng @muchulee8 @ColinPeppler @amjames @desertfire @chauhang @aakhundov