Skip to content
This repository has been archived by the owner on Mar 21, 2024. It is now read-only.

Port unique by key from thrust #405

Merged
merged 4 commits into from
Jan 24, 2022
Merged

Port unique by key from thrust #405

merged 4 commits into from
Jan 24, 2022

Conversation

zasdfgbnm
Copy link
Contributor

No description provided.

@zasdfgbnm zasdfgbnm changed the title [WIP] Port unique by key from thrust Port unique by key from thrust Nov 13, 2021
@zasdfgbnm zasdfgbnm marked this pull request as ready for review November 13, 2021 01:41
@zasdfgbnm
Copy link
Contributor Author

@allisonvacanti This should be ready. The tests are passing locally on my computer. The only thing missing are docs.

@zasdfgbnm
Copy link
Contributor Author

doc is done

@zasdfgbnm
Copy link
Contributor Author

cc @ptrblck @ngimel

@alliepiper alliepiper self-assigned this Nov 15, 2021
@alliepiper alliepiper added P1: should have Necessary, but not critical. type: enhancement New feature or request. labels Nov 15, 2021
@alliepiper alliepiper added this to Inbox in PR Tracking via automation Nov 15, 2021
@alliepiper alliepiper moved this from Inbox to Need Review in PR Tracking Nov 15, 2021
@alliepiper alliepiper added this to the 1.16.0 milestone Nov 15, 2021
This was referenced Nov 15, 2021
Closed
Closed
alliepiper
alliepiper suggested changes Jan 12, 2022
View reviewed changes
Copy link
Collaborator

@alliepiper alliepiper left a comment

Choose a reason for hiding this comment

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

Apologies for taking so long to get to this -- I've pointed out some issues inline. Most of them are related to recent changes to cub, mainly #364 and #403. Otherwise this LGTM 👍

I'll start testing once:

  • Review comments are addressed.
  • Branch is rebased.
  • Branch is squashed to remove temporary WIP commits.

cub/agent/agent_unique_by_key.cuh Outdated Show resolved Hide resolved
cub/agent/agent_unique_by_key.cuh Outdated Show resolved Hide resolved
cub/agent/agent_unique_by_key.cuh Outdated Show resolved Hide resolved
cub/util_math.cuh Outdated Show resolved Hide resolved
cub/device/dispatch/dispatch_unique_by_key.cuh Outdated Show resolved Hide resolved
cub/device/dispatch/dispatch_unique_by_key.cuh Outdated Show resolved Hide resolved
test/test_device_select_unique_by_key.cu Outdated Show resolved Hide resolved
test/test_device_select_unique_by_key.cu Outdated Show resolved Hide resolved
test/test_device_select_unique_by_key.cu Outdated Show resolved Hide resolved
@alliepiper alliepiper moved this from Need Review to Drafts in PR Tracking Jan 12, 2022
@alliepiper alliepiper assigned zasdfgbnm and unassigned alliepiper Jan 12, 2022
@zasdfgbnm
Copy link
Contributor Author

All done

@alliepiper alliepiper moved this from Drafts to Need Testing in PR Tracking Jan 19, 2022
@alliepiper alliepiper assigned alliepiper and unassigned zasdfgbnm Jan 19, 2022
@alliepiper
Copy link
Collaborator

Rebased to bring in recent CI fixes.

alliepiper added a commit to alliepiper/thrust that referenced this pull request Jan 19, 2022
@alliepiper
Do not merge; Testing NVIDIA/cub#405.
9957580
@alliepiper
Copy link
Collaborator

gpuCI: NVIDIA/thrust#1596
DVS CL: 30893135

@zasdfgbnm
Copy link
Contributor Author

Just fixed an error on C++11 detected by the CI

alliepiper added a commit to alliepiper/thrust that referenced this pull request Jan 20, 2022
@alliepiper
Do not merge; Testing NVIDIA/cub#405.
91609b9
@alliepiper
Copy link
Collaborator

Restarted CI on new version:

gpuCI: NVIDIA/thrust#1596
DVS CL: 30897739

@alliepiper alliepiper moved this from Need Testing to Tests Pending in PR Tracking Jan 20, 2022
@alliepiper alliepiper added testing: gpuCI in progress Started gpuCI testing. testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). labels Jan 20, 2022
@zasdfgbnm
Copy link
Contributor Author

I fixed the IsPointer deprecation error in CI. It's strange that my local doesn't detect it.

@alliepiper
Copy link
Collaborator

alliepiper commented Jan 21, 2022 •
edited

I fixed the IsPointer deprecation error in CI.

Thanks, I'll restart the builds in a moment.

It's strange that my local doesn't detect it.

The default value for the CMake option THRUST_IGNORE_DEPRECATED_API changed about a month ago -- we used to ignore deprecation warnings while building tests, but now we're able to leave them on. Your CMake build has probably cached the old default value.

Running cmake -DTHRUST_IGNORE_DEPRECATED_API=OFF . from your build directory should update this setting and enable the warnings.

alliepiper added a commit to alliepiper/thrust that referenced this pull request Jan 21, 2022
@alliepiper
Do not merge; Testing NVIDIA/cub#405.
4a65112
@alliepiper
Copy link
Collaborator

gpuCI: NVIDIA/thrust#1596
DVS CL: 30902006

alliepiper
alliepiper approved these changes Jan 24, 2022
View reviewed changes
Copy link
Collaborator

@alliepiper alliepiper left a comment

Choose a reason for hiding this comment

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

Tests are passing 👍

@alliepiper alliepiper merged commit d195a4a into NVIDIA:main Jan 24, 2022
PR Tracking automation moved this from Tests Pending to Done Jan 24, 2022
@zasdfgbnm zasdfgbnm deleted the unique-by-key branch January 24, 2022 19:17
facebook-github-bot pushed a commit to pytorch/pytorch that referenced this pull request Mar 8, 2022
@zasdfgbnm @facebook-github-bot
Use cub::DeviceSelect::UniqueByKey for EmbeddingBackward (#68376)
68a69bb 
Summary:
NVIDIA/cub#405 is still under review, API might change before it finally lands into cub 1.16, please wait for NVIDIA/cub#405 before merging this. Tested locally and tests pass.

Pull Request resolved: #68376

Reviewed By: bdhirsh

Differential Revision: D34706782

Pulled By: ngimel

fbshipit-source-id: a465d39bc24354d1047af1ee85be05a1de361c86
pytorchmergebot pushed a commit to pytorch/pytorch that referenced this pull request Mar 8, 2022
@zasdfgbnm @pytorchmergebot
Use cub::DeviceSelect::UniqueByKey for EmbeddingBackward (#68376)
ee8d7d8 
Summary:
NVIDIA/cub#405 is still under review, API might change before it finally lands into cub 1.16, please wait for NVIDIA/cub#405 before merging this. Tested locally and tests pass.

Pull Request resolved: #68376

Reviewed By: bdhirsh

Differential Revision: D34706782

Pulled By: ngimel

fbshipit-source-id: a465d39bc24354d1047af1ee85be05a1de361c86
(cherry picked from commit 68a69bb)
cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 9, 2022
@zasdfgbnm @cyyever
Use cub::DeviceSelect::UniqueByKey for EmbeddingBackward (#68376)
910666c 
Summary:
NVIDIA/cub#405 is still under review, API might change before it finally lands into cub 1.16, please wait for NVIDIA/cub#405 before merging this. Tested locally and tests pass.

Pull Request resolved: pytorch/pytorch#68376

Reviewed By: bdhirsh

Differential Revision: D34706782

Pulled By: ngimel

fbshipit-source-id: a465d39bc24354d1047af1ee85be05a1de361c86
(cherry picked from commit 68a69bbc5093fd12b1fbfd561b3a10baf5d3e5ba)
cyyever pushed a commit to cyyever/pytorch_private that referenced this pull request Mar 9, 2022
@zasdfgbnm @cyyever
Use cub::DeviceSelect::UniqueByKey for EmbeddingBackward (#68376)
655e233 
Summary:
NVIDIA/cub#405 is still under review, API might change before it finally lands into cub 1.16, please wait for NVIDIA/cub#405 before merging this. Tested locally and tests pass.

Pull Request resolved: pytorch/pytorch#68376

Reviewed By: bdhirsh

Differential Revision: D34706782

Pulled By: ngimel

fbshipit-source-id: a465d39bc24354d1047af1ee85be05a1de361c86
(cherry picked from commit 68a69bbc5093fd12b1fbfd561b3a10baf5d3e5ba)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alliepiper alliepiper alliepiper approved these changes

Assignees

@alliepiper alliepiper

Labels
P1: should have Necessary, but not critical. testing: gpuCI in progress Started gpuCI testing. testing: internal ci in progress Currently testing on internal NVIDIA CI (DVS). type: enhancement New feature or request.
Projects
PR Tracking
Done
Milestone
1.16.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@zasdfgbnm @alliepiper