10BC0 fix slice w/ dynamic shapes by cgufb · Pull Request #153131 · pytorch/pytorch · GitHub
[go: up one dir, main page]
Skip to content

fix slice w/ dynamic shapes #153131

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

fix slice w/ dynamic shapes #153131

wants to merge 1 commit into from

Conversation

cgufb
Copy link
Contributor
@cgufb cgufb commented May 8, 2025 •
edited by laithsakka
Loading

Summary: guard_size_oblivious has side effects that'll result in invalid strides when slice nodes take negative index on dynamic input shapes.
Cause overflow error with a huge number “9223372036854776048”
Test Plan: CIs should pass.

Differential Revision: D74354663

@pytorch-bot PyTorch Bot
Copy link
pytorch-bot bot commented May 8, 2025 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/153131

Note: Links to docs will display an error until the docs builds have been completed.

✅ No Failures

As of commit 4494765 with merge base 093fd47 (image):
💚 Looks good so far! There are no failures yet. 💚

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link
linux-foundation-easycla bot commented May 8, 2025 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D74354663

@cgufb cgufb added the topic: not user facing topic category label May 8, 2025
@cgufb cgufb force-pushed the export-D74354663 branch from e2cc103 to ecea16f Compare May 12, 2025 16:14
pytorch-bot bot pushed a commit that referenced this pull request May 12, 2025
@cgufb @facebook-github-bot
fix slice w/ dynamic shapes (#153131)
ecea16f 
Summary:

guard_size_oblivious has side effects that'll result in invalid strides when slice nodes take negative index on dynamic input shapes.

Test Plan: CIs should pass.

Differential Revision: D74354663
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D74354663

cgufb added a commit to cgufb/pytorch that referenced this pull request May 12, 2025
@cgufb @facebook-github-bot
fix slice w/ dynamic shapes (pytorch#153131)
5d13e70 
Summary:
Pull Request resolved: pytorch#153131

guard_size_oblivious has side effects that'll result in invalid strides when slice nodes take negative index on dynamic input shapes.

Test Plan: CIs should pass.

Differential Revision: D74354663
@cgufb cgufb force-pushed the export-D74354663 branch 2 times, most recently from 5d13e70 to 6d73b50 Compare May 12, 2025 18:52
cgufb added a commit to cgufb/pytorch that referenced this pull request May 12, 2025
@cgufb @facebook-github-bot
fix slice w/ dynamic shapes (pytorch#153131)
6d73b50 
Summary:

guard_size_oblivious has side effects that'll result in invalid strides when slice nodes take negative index on dynamic input shapes.

Test Plan: CIs should pass.

Differential Revision: D74354663
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D74354663

@cgufb cgufb requested review from ezyang, laithsakka and TroyGarden May 12, 2025 20:10
laithsakka
laithsakka requested changes May 12, 2025
View reviewed changes
Copy link
Contributor
@laithsakka laithsakka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add a unit test

@laithsakka
Copy link
Contributor

chatting off line this is the graph where this currently fails

acc_graph():
    %a : f32[s35, s16][#users=1] = placeholder[target='a']
    %slice_tensor : f32[s35, s16][#users=1] = call_function[target=aten.slice.Tensor](kwargs = {input: %a, dim: 0, start: 0, end: s35, step: 1})
    %slice_tensor_1 : f32[s35, 241][#users=1] = call_function[target=aten.slice.Tensor](kwargs = {input: %slice_tensor, dim: 1, start: Mod(-241, s16), end: s16, step: 1})
    return (%slice_tensor_1,)

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D74354663

@cgufb cgufb force-pushed the export-D74354663 branch from 6d73b50 to 87216e3 Compare June 12, 2025 18:42
pytorch-bot bot pushed a commit that referenced this pull request Jun 12, 2025
@cgufb @facebook-github-bot
fix slice w/ dynamic shapes (#153131)
87216e3 
Summary:
Pull Request resolved: #153131

guard_size_oblivious has side effects that'll result in invalid strides when slice nodes take negative index on dynamic input shapes.

Test Plan: CIs should pass.

Differential Revision: D74354663
@cgufb @facebook-github-bot
fix slice w/ dynamic shapes (pytorch#153131)
4494765 
Summary:
Pull Request resolved: pytorch#153131

guard_size_oblivious has side effects that'll result in invalid strides when slice nodes take negative index on dynamic input shapes.

Test Plan: CIs should pass.

Differential Revision: D74354663
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D74354663

@cgufb cgufb force-pushed the export-D74354663 branch from 87216e3 to 4494765 Compare June 12, 2025 20:10
@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Jun 12, 2025
@laithsakka
Copy link
Contributor

although idk what is wrong exactly with dynamic shapes, i accept to unblock since the change does not look harmful.

@facebook-github-bot
Copy link
Contributor 8000

@pytorchbot merge

(Initiating merge automatically since Phabricator Diff has merged)

@pytorchmergebot
Copy link
Collaborator

Merge started

Your 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

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

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
For more information see pytorch-bot wiki.

@clee2000
Copy link
Contributor

@pytorchbot merge -f "merged internally"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes). Please use -f as last resort and instead consider -i/--ignore-current to continue the merge ignoring current failures. This will allow currently pending tests to finish and report signal before the merge.

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

thatgeeman pushed a commit to thatgeeman/pytorch-docathon that referenced this pull request Jun 15, 2025
@cgufb @thatgeeman
fix slice w/ dynamic shapes (pytorch#153131)
1a34900 
Summary: guard_size_oblivious has side effects that'll result in invalid strides when slice nodes take negative index on dynamic input shapes.
Cause overflow error with a huge number “9223372036854776048”
Test Plan: CIs should pass.

Differential Revision: D74354663

Pull Request resolved: pytorch#153131
Approved by: https://github.com/laithsakka
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laithsakka laithsakka laithsakka approved these changes

@ezyang ezyang Awaiting requested review from ezyang

@TroyGarden TroyGarden Awaiting requested review from TroyGarden

@bobrenjc93 bobrenjc93 Awaiting requested review from bobrenjc93

@zou3519 zou3519 Awaiting requested review from zou3519

@bdhirsh bdhirsh Awaiting requested review from bdhirsh

Assignees
No one assigned
Labels
ciflow/inductor ciflow/trunk Trigger trunk jobs on your pull request fb-exported Merged topic: not user facing topic category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cgufb @facebook-github-bot @laithsakka @pytorchmergebot @clee2000
0