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

[CUBLAS][CUDA GRAPHS] (re-re-re-re-open of #83461) Explicitly set the workspace for cuBLAS handles #86645

Closed
wants to merge 15 commits into from
Closed

[CUBLAS][CUDA GRAPHS] (re-re-re-re-open of #83461) Explicitly set the workspace for cuBLAS handles #86645

wants to merge 15 commits into from

Conversation

eqy
Copy link
Collaborator

@eqy eqy commented Oct 10, 2022

re-opening (again) in hopes of working around failed/stuck CLA check

CC @ptrblck @ngimel @huydhn

eqy and others added 15 commits October 10, 2022 22:14
@eqy @pytorchmergebot
check in
86f9cfe
@eqy @pytorchmergebot
lint
b855540
@eqy @pytorchmergebot
skip for rocm
b21c29e
@eqy @pytorchmergebot
fix
67f3b1c
@eqy @pytorchmergebot
add stream to key
badf8dd
@eqy @pytorchmergebot
warmup cuBLAS workspaces for balance test
f9d5f12
@eqy @pytorchmergebot
use larger tensors to negate effect of potential workspace allocation
b8e8312
@eqy @pytorchmergebot
change expected memory usage due to cuBLAS workspace allocation
43ec041
@eqy @pytorchmergebot
lint
166af93
@eqy @pytorchmergebot
fix for 10.2
c07c4ee
@eqy @pytorchmergebot
clear cuBLAS workspaces for memory leak checks
0d10341
@eqy @pytorchmergebot
change naming due to namespace
b4a2da2
@pytorchmergebot
fix process shutdown crashes with cublas_handle_stream_to_workspace
071ebae 
Summary:
This was causing crashes in the cuda tests inside Meta.

Test Plan: Already verified this works inside Meta. Rely on OSS CI.

Reviewers:

Subscribers:

Tasks:

Tags:

[ghstack-poisoned]
@pytorchmergebot
improve performance of workspace lookup/insertion
3ece30e 
Summary:
Avoid multiple traversals of the map with correct std::map usage.

Test Plan: Rely on CI.

Reviewers:

Subscribers:

Tasks:

Tags:

[ghstack-poisoned]
@eqy @pytorchmergebot
remove leak check skip
0e4ca01
@pytorch-bot pytorch-bot bot added the release notes: distributed (fsdp) release notes category label Oct 10, 2022
@pytorch-bot
Copy link

pytorch-bot bot commented Oct 10, 2022 •
edited

🔗 Helpful Links

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

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

✅ No Failures

As of commit 0e4ca01:
💚 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
Copy link

CLA Not Signed

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 10, 2022
@zdevito
Copy link
Contributor

zdevito commented Oct 10, 2022

For the CLA, maybe try squashing all the commits in the eqy:cublasworkspace_graphs branch so that there is only a single commit authored by you. It looks like it is getting confused at 071ebae

@ngimel
Copy link
Collaborator

ngimel commented Oct 11, 2022

@xuzhao9 heads up I'm landing this PR, as my understanding is there are workarounds for torchbench.

@ngimel
Copy link
Collaborator

ngimel commented Oct 11, 2022

@pytorchbot merge -g

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks on your PR pass since you used the green (-g) flag (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

@github-actions
Copy link

Hey @eqy.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

@eqy
Copy link
Collaborator Author

eqy commented Oct 12, 2022

@xuzhao9 the link for the corresponding benchmark changes are here: pytorch/benchmark#1226

If the memory leak check (pytorch/benchmark#1228) is added, additional changes like the ones in this PR would also be needed.

@eqy eqy mentioned this pull request Feb 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zdevito zdevito zdevito approved these changes

@mrshenli mrshenli Awaiting requested review from mrshenli

@pritamdamania87 pritamdamania87 Awaiting requested review from pritamdamania87

@zhaojuanmao zhaojuanmao Awaiting requested review from zhaojuanmao

@rohan-varma rohan-varma Awaiting requested review from rohan-varma

@H-Huang H-Huang Awaiting requested review from H-Huang

@awgu awgu Awaiting requested review from awgu

@kwen2501 kwen2501 Awaiting requested review from kwen2501

@mingzhe09088 mingzhe09088 Awaiting requested review from mingzhe09088

Assignees
No one assigned
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged open source release notes: distributed (fsdp) release notes category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@eqy @zdevito @ngimel @pytorchmergebot @pytorchbot