Skip to content
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

Avoid non-blocking GPU->CPU copies. #11288

Merged
merged 5 commits into from
Jan 3, 2022
Merged

Avoid non-blocking GPU->CPU copies. #11288

merged 5 commits into from
Jan 3, 2022

Conversation

edpizzi
Copy link
Contributor

@edpizzi edpizzi commented Dec 31, 2021 •
edited by carmocca
Loading

Non-blocking GPU->CPU transfers can create race windows where tensor contents are observed to have incorrect values.

Fixes #11287. See #11287 for detailed mechanics of the issue this resolves.

Verified that this change fixes the behavior in the reproduce script from #11287.

Does your PR introduce any breaking changes? If yes, please list them.

No

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified
    • can't seem to do this, and the review guidelines wiki page no longer loads for me

Did you have fun?

Fixing: yes. Root causing the bug: no. It's subtle, and many log statements you add to debug the issue fix the issue (by observing the value of GPU tensors, forcing a CUDA sync), which is frustrating if you don't know that this is the nature of the issue.

I also had some trouble getting tests to run in my environment. It looks to me like some tests fail in GPU environments but pass in non-GPU environments. Others fail when run on slurm environments, which is the only way I can easily access GPUs.

@edpizzi edpizzi force-pushed the fix_11287_non_blocking_copy_to_cpu branch from 685b053 to 61ff6a4 Compare January 1, 2022 23:01
@edpizzi edpizzi marked this pull request as ready for review January 2, 2022 07:16
justusschock
justusschock approved these changes Jan 3, 2022
View reviewed changes
pytorch_lightning/utilities/apply_func.py Outdated Show resolved Hide resolved
rohitgr7
rohitgr7 approved these changes Jan 3, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@mergify mergify bot added the ready PRs ready to be merged label Jan 3, 2022
awaelchli
awaelchli approved these changes Jan 3, 2022
View reviewed changes
pytorch_lightning/utilities/apply_func.py Outdated Show resolved Hide resolved
@mergify mergify bot added the has conflicts label Jan 3, 2022
edpizzi and others added 4 commits January 3, 2022 17:18
@edpizzi
Avoid non-blocking GPU->CPU copies.
abb3fba 
Non-blocking GPU->CPU transfers can create race windows where tensor contents
are observed to have incorrect values. Lightning-AI#11287
@Borda @rohitgr7
@justusschock @edpizzi
Apply suggestions from code review
5f02be4 
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>
@awaelchli @edpizzi
update changelog
e677d76
@edpizzi
remove None from CPU_DEVICES
6ca1018
@edpizzi edpizzi force-pushed the fix_11287_non_blocking_copy_to_cpu branch from 43caaa8 to 6ca1018 Compare January 3, 2022 17:27
@mergify mergify bot removed the has conflicts label Jan 3, 2022
carmocca
carmocca approved these changes Jan 3, 2022
View reviewed changes
pytorch_lightning/utilities/apply_func.py Outdated Show resolved Hide resolved
pytorch_lightning/utilities/apply_func.py Outdated Show resolved Hide resolved
pytorch_lightning/utilities/apply_func.py Outdated Show resolved Hide resolved
@carmocca carmocca added accelerator bug Something isn't working labels Jan 3, 2022
@carmocca carmocca added this to the 1.5.x milestone Jan 3, 2022
@carmocca carmocca enabled auto-merge (squash) January 3, 2022 21:42
@carmocca carmocca merged commit cf32127 into Lightning-AI:master Jan 3, 2022
rohitgr7 added a commit that referenced this pull request Jan 4, 2022
@edpizzi @Borda
@rohitgr7 @justusschock @awaelchli
Avoid non-blocking GPU->CPU copies. (#11288)
512ec4f 
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
awaelchli added a commit that referenced this pull request Jan 4, 2022
@edpizzi @Borda
@rohitgr7 @justusschock @awaelchli
Avoid non-blocking GPU->CPU copies. (#11288)
85dacad 
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
awaelchli added a commit that referenced this pull request Jan 5, 2022
@edpizzi @Borda
@rohitgr7 @justusschock @awaelchli
Avoid non-blocking GPU->CPU copies. (#11288)
3ab6788 
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
awaelchli added a commit that referenced this pull request Jan 5, 2022
@edpizzi @Borda
@rohitgr7 @justusschock @awaelchli
Avoid non-blocking GPU->CPU copies. (#11288)
257c0ec 
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
lexierule pushed a commit that referenced this pull request Jan 5, 2022
@edpizzi @Borda
@rohitgr7 @justusschock @awaelchli @lexierule
Avoid non-blocking GPU->CPU copies. (#11288)
15f524b 
Co-authored-by: Jirka Borovec <Borda@users.noreply.github.com>
Co-authored-by: Rohit Gupta <rohitgr1998@gmail.com>
Co-authored-by: Justus Schock <12886177+justusschock@users.noreply.github.com>
Co-authored-by: Adrian Wälchli <aedu.waelchli@gmail.com>
@rohitgr7 rohitgr7 mentioned this pull request Feb 7, 2022
Closed
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awaelchli awaelchli awaelchli approved these changes

@Borda Borda Borda approved these changes

@carmocca carmocca carmocca approved these changes

@justusschock justusschock justusschock approved these changes

@rohitgr7 rohitgr7 rohitgr7 approved these changes

@kaushikb11 kaushikb11 Awaiting requested review from kaushikb11

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@tchaton tchaton Awaiting requested review from tchaton tchaton is a code owner

@williamFalcon williamFalcon Awaiting requested review from williamFalcon

Assignees
No one assigned
Labels
accelerator bug Something isn't working ready PRs ready to be merged
Projects
None yet
Milestone
pl:1.5.x
Development

Successfully merging this pull request may close these issues.

GPU prediction race condition results in BasePredictionWriter observing incorrect (zero) values
6 participants
@edpizzi @awaelchli @Borda @carmocca @justusschock @rohitgr7