KEMBAR78
EmbeddingBag sort thrust->cub by zasdfgbnm · Pull Request #64498 · pytorch/pytorch · GitHub
Skip to content

EmbeddingBag sort thrust->cub #64498

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

EmbeddingBag sort thrust->cub #64498

wants to merge 3 commits into from

Conversation

@zasdfgbnm
Copy link
Collaborator

@zasdfgbnm zasdfgbnm commented Sep 3, 2021 •
edited
Loading

Partially fixes #57505

Also fixes a warning I found when compiling:

/home/gaoxiang/pytorch-cub/torch/csrc/distributed/c10d/quantization/quantization_gpu.cu(7): warning: inline qualifier ignored for "__global__" function

I also updated the bfloat16 guard to CUDA 11.5

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Sep 3, 2021 •
edited
Loading

🔗 Helpful links

💊 CI failures summary and remediations

As of commit 227aa33 (more details on the Dr. CI page):

  • 1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

See GitHub Actions build win-vs2019-cpu-py3 / test (default, 1, 2, windows.4xlarge) (1/1)

Step: "Run test scripts" (full log | diagnosis details | 🔁 rerun)

2021-09-07T17:12:51.3254771Z ERROR [0.006s]: test_poisson_sample (__main__.TestDistributions) 
2021-09-07T17:12:51.3248651Z   File "distributions/test_distributions.py", line 812, in _check_sampler_discrete
2021-09-07T17:12:51.3249430Z     chisq, p = scipy.stats.chisquare(counts[msk], pmf[msk] * num_samples)
2021-09-07T17:12:51.3250177Z   File "c:\jenkins\miniconda3\lib\site-packages\scipy\stats\stats.py", line 6852, in chisquare
2021-09-07T17:12:51.3250838Z     return power_divergence(f_obs, f_exp=f_exp, ddof=ddof, axis=axis,
2021-09-07T17:12:51.3251568Z   File "c:\jenkins\miniconda3\lib\site-packages\scipy\stats\stats.py", line 6694, in power_divergence
2021-09-07T17:12:51.3252168Z     raise ValueError(msg)
2021-09-07T17:12:51.3252972Z ValueError: For each axis slice, the sum of the observed frequencies must agree with the sum of the expected frequencies to a relative tolerance of 1e-08, but the percent differences are:
2021-09-07T17:12:51.3253865Z 0.008265582255680495
2021-09-07T17:12:51.3254045Z 
2021-09-07T17:12:51.3254313Z ======================================================================
2021-09-07T17:12:51.3254771Z ERROR [0.006s]: test_poisson_sample (__main__.TestDistributions)
2021-09-07T17:12:51.3255312Z ----------------------------------------------------------------------
2021-09-07T17:12:51.3255768Z Traceback (most recent call last):
2021-09-07T17:12:51.3256358Z   File "distributions/test_distributions.py", line 1352, in test_poisson_sample
2021-09-07T17:12:51.3256937Z     self._check_sampler_discrete(Poisson(rate),
2021-09-07T17:12:51.3257577Z   File "distributions/test_distributions.py", line 812, in _check_sampler_discrete
2021-09-07T17:12:51.3258267Z     chisq, p = scipy.stats.chisquare(counts[msk], pmf[msk] * num_samples)
2021-09-07T17:12:51.3258998Z   File "c:\jenkins\miniconda3\lib\site-packages\scipy\stats\stats.py", line 6852, in chisquare
2021-09-07T17:12:51.3259764Z     return power_divergence(f_obs, f_exp=f_exp, ddof=ddof, axis=axis,
2021-09-07T17:12:51.3260488Z   File "c:\jenkins\miniconda3\lib\site-packages\scipy\stats\stats.py", line 6694, in power_divergence
2021-09-07T17:12:51.3261078Z     raise ValueError(msg)
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@zasdfgbnm zasdfgbnm mentioned this pull request Sep 3, 2021
Closed
5 tasks
zasdfgbnm
zasdfgbnm commented Sep 3, 2021
View reviewed changes
aten/src/ATen/native/cuda/EmbeddingBag.cu
bool scale_grad_by_freq, int64_t mode,
const Tensor& per_sample_weights,
int64_t padding_idx) {
auto indices = indices_.contiguous();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Similar to #63427

@zasdfgbnm zasdfgbnm requested a review from ngimel September 3, 2021 17:54
@zasdfgbnm zasdfgbnm marked this pull request as ready for review September 3, 2021 17:54
@codecov
Copy link

codecov bot commented Sep 7, 2021

Codecov Report

Merging #64498 (227aa33) into master (6831d8e) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #64498      +/-   ##
==========================================
- Coverage   66.63%   66.62%   -0.01%     
==========================================
  Files         707      707              
  Lines       92366    92369       +3     
==========================================
- Hits        61551    61544       -7     
- Misses      30815    30825      +10

@mrshenli mrshenli added module: embedding triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module labels Sep 7, 2021
ngimel
ngimel approved these changes Sep 13, 2021
View reviewed changes
Copy link
Collaborator

@ngimel ngimel left a comment

Choose a reason for hiding this comment

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

Sorry for a delay, this looks great!

aten/src/ATen/cuda/cub.cuh
};

#if defined(CUDA_VERSION) && CUDA_VERSION >= 99999
#if defined(CUDA_VERSION) && CUDA_VERSION >= 11050
Copy link
Collaborator

Choose a reason for hiding this comment

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

awesome!

@facebook-github-bot
Copy link
Contributor

@ngimel has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ngimel merged this pull request in 816048e.

@zasdfgbnm zasdfgbnm deleted the ebtc branch September 14, 2021 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ngimel ngimel ngimel approved these changes

@cbalioglu cbalioglu Awaiting requested review from cbalioglu

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

@mingzhe09088 mingzhe09088 Awaiting requested review from mingzhe09088

@mrshenli mrshenli Awaiting requested review from mrshenli

@pritamdamania87 pritamdamania87 Awaiting requested review from pritamdamania87

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

@wayi1 wayi1 Awaiting requested review from wayi1

@zhaojuanmao zhaojuanmao Awaiting requested review from zhaojuanmao

Assignees

No one assigned

Labels

cla signed Merged module: embedding open source triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Reduce or eliminate thrust usage in pytorch

5 participants

@zasdfgbnm @facebook-github-bot @ngimel @mrshenli @pytorchbot