Revert to old behaviour of not padding strides if shape or stride is dynamic #163639
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/163639
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 6532526 with merge base bcb893a ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
pytorch-bot
bot
added
ci-no-td
|
@nandesuka has exported this pull request. If you are a Meta employee, you can view the originating diff in D83053287. |
|
@pytorchbot label "topic: not user facing" |
blaine-rister
left a comment
blaine-rister
left a comment
There was a problem hiding this comment.
LGTM. Left a nit about cleaning up some related code.
| isinstance(s, (int, sympy.Integer)) | ||
| for s in itertools.chain(in_strides, size) | ||
| ) | ||
| if not config.pad_dynamic_shapes and is_dynamic: |
There was a problem hiding this comment.
nit: is it safe to remove the or config.pad_dynamic_shapes on line 3784? Now that we're returning early, that might be redundant.
There was a problem hiding this comment.
let's just leave it for now, can clean up later once we have a root cause
There was a problem hiding this comment.
are we padding with out guards if we remove this?
shall we instead make sure we guard properly .
There was a problem hiding this comment.
Can you clarify, guard what values with what constraints?
blaine-rister
left a comment
blaine-rister
left a comment
There was a problem hiding this comment.
LGTM. Left a nit about possibly removing a redundant check further down in the function.
…dynamic (pytorch#163639) Summary: forward fix for S563814 Test Plan: foward fix for S563814 Reviewed By: blaine-rister Differential Revision: D83053287
nandesuka
force-pushed
the
export-D83053287
branch
from
845f5f1 to
6532526
Compare
|
@nandesuka has exported this pull request. If you are a Meta employee, you can view the originating diff in D83053287. |
|
@pytorchbot merge (Initiating merge automatically since Phabricator Diff has merged) |
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 |
…dynamic (#163639) Differential Revision: D83053287 Pull Request resolved: #163639 Approved by: https://github.com/blaine-rister
5 participants
Differential Revision: D83053287
cc @voznesenskym @penguinwu @EikanWang @jgong5 @Guobing-Chen @XiaobingSuper @zhuhaozhe @blzheng @wenzhe-nrv @jiayisunx @ipiszy @chenyang78 @kadeng @muchulee8 @amjames @chauhang @aakhundov @coconutruben