Skip to content

Fixed crash when calling pad_packed_tensor when packed with cuda tensors and ensure_sorted=false due to indexing with tensors on different devices #115028

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 8 commits into from
Closed

Fixed crash when calling pad_packed_tensor when packed with cuda tensors and ensure_sorted=false due to indexing with tensors on different devices #115028

wants to merge 8 commits into from

Conversation

shaltielshmid
Copy link
Contributor

Fixes #115027

Fix in csrc as done in the python code here.

@pytorch-bot pytorch-bot bot added the release notes: cpp release notes category label Dec 3, 2023
@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Dec 3, 2023 •
edited
Loading

🔗 Helpful Links

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

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

⏳ No Failures, 2 Pending

As of commit 4b63074 with merge base a6736ac (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 Dec 3, 2023 •
edited
Loading

CLA Signed

The committers listed above are authorized under a signed CLA.

@drisspg
Copy link
Contributor

drisspg commented Dec 4, 2023

Can you write a test for this failure?

@shaltielshmid
Copy link
Contributor Author

Sure, added

@drisspg
Copy link
Contributor

drisspg commented Dec 5, 2023

Cool, thank you and one more think, would you mind making the PR title more descriptive?

@shaltielshmid shaltielshmid changed the title Fixed https://github.com/pytorch/pytorch/issues/115027 Fixed crash when calling pad_packed_tensor when packed with cuda tensors and ensure_sorted=false due to indexing with tensors on different devices Dec 5, 2023
@shaltielshmid
Copy link
Contributor Author

Sure, done :)

@drisspg
Copy link
Contributor

drisspg commented Dec 5, 2023

@pytorchbot merge

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Dec 5, 2023

This PR needs to be approved by an authorized maintainer before merge.

@drisspg
Copy link
Contributor

drisspg commented Dec 5, 2023

@pytorchbot merge

@drisspg
Copy link
Contributor

drisspg commented Dec 5, 2023

Looks like need to fix some formatting

@shaltielshmid
Copy link
Contributor Author

Fixed the whitespace issue.

@drisspg
Copy link
Contributor

drisspg commented Dec 5, 2023

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Dec 5, 2023
@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

Merge failed

Reason: 1 mandatory check(s) failed. The first few are:

Dig deeper by viewing the failures on hud

Details for Dev Infra team Raised by workflow job

Failing merge rule: Core Maintainers

@shaltielshmid
Copy link
Contributor Author

shaltielshmid commented Dec 5, 2023 •
edited
Loading

Thanks for the patience - I got lintrunner running locally and applied the patches. All fixed now.
@drisspg Can you merge again?

Thanks!

@shaltielshmid
Copy link
Contributor Author

Hi @drisspg , is there anything else I can do to help get this fix in? Thanks!

@drisspg
Copy link
Contributor

drisspg commented Dec 7, 2023

hmm I don't know why this so did you run

make setup_lint
lintrunner -a

@shaltielshmid
Copy link
Contributor Author

shaltielshmid commented Dec 7, 2023 •
edited
Loading

Yeah, after the merge failed I ran again:

lintrunner init
lintrunner -a

I believe that one of the lines of the test was > 80 chars, so it split it into two lines.
You can see the fixes that lintrunner added here: 847f0f7

@drisspg
Copy link
Contributor

drisspg commented Dec 7, 2023

hmm I would try rebasing and then re-initing the lint runner and running again there might be an update on main

@drisspg drisspg closed this Dec 7, 2023
@drisspg drisspg reopened this Dec 7, 2023
@shaltielshmid
Copy link
Contributor Author

hmm I would try rebasing and then re-initing the lint runner and running again there might be an update on main

Done! It changed a few spacing stuff

@drisspg
Copy link
Contributor

drisspg commented Dec 7, 2023

@pytorchbot merge

@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

@shaltielshmid shaltielshmid deleted the fix-rnn-pack-cuda branch December 7, 2023 18:15
dmenig pushed a commit to dmenig/pytorch that referenced this pull request Dec 21, 2023
@shaltielshmid @dmenig
Fixed crash when calling pad_packed_tensor when packed with cuda tens…
9022b2c 
…ors and ensure_sorted=false due to indexing with tensors on different devices (pytorch#115028)

Fixes pytorch#115027

Fix in csrc as done in the python code [here](https://github.com/pytorch/pytorch/blob/main/torch/nn/utils/rnn.py#L338).

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

Reviewers

@drisspg drisspg drisspg approved these changes

Assignees

No one assigned

Labels

ciflow/trunk Trigger trunk jobs on your pull request Merged open source release notes: cpp release notes category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Calling pad_packed_sequence with tensors on cuda and enforce_sorted=false causes issue in C++

4 participants

@shaltielshmid @drisspg @pytorchmergebot @pytorchbot