Fix Byte-Pair-Encoding usage of cuco static-map for storing merge-pairs #13807
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
davidwendt
added
bug
PointKernel
reviewed
Aug 2, 2023
There was a problem hiding this comment.
The code should build properly with the proposed changes. The problem with double_hashing is that we expect users to provide a hasher that can be constructed from an integer (or a seed). In that way, we can minimize the chance of secondary collisions during probing but now I realize it's a too-restrictive requirement. By all means, for a single map with 50% occupancy, linear_probing would probably outperform double_hashing all the time.
|
I'm looking into other issues (e.g. (should be solved in NVIDIA/cuCollections#345) |
PointKernel
added a commit
to NVIDIA/cuCollections
that referenced
this pull request
Aug 6, 2023
This PR fixes issues and adds new features requested by rapidsai/cudf#13807. It: - removes the requirement of the second hasher from double hashing must be constructible from an integer - fixes an issue in map iterator `!=` operator - overloads map iterator access operator - allows zero capacity container - adds `capacity_test` back since several corner cases need to be exercised
7 tasks
PointKernel
reviewed
Aug 6, 2023
There was a problem hiding this comment.
CMake changes to be reverted once rapidsai/rapids-cmake#452 is merged.
rapids-bot bot
pushed a commit
to rapidsai/rapids-cmake
that referenced
this pull request
Aug 11, 2023
This PR fetches the latest cuco to unblock rapidsai/cudf#13807. Authors: - Yunsong Wang (https://github.com/PointKernel) - Robert Maynard (https://github.com/robertmaynard) Approvers: - Bradley Dice (https://github.com/bdice) - Nghia Truong (https://github.com/ttnghia) - Robert Maynard (https://github.com/robertmaynard) URL: #452
PointKernel
approved these changes
Aug 11, 2023
…fix-bpe-static-map
davidwendt
changed the title
davidwendt
changed the title
bdice
reviewed
Aug 15, 2023
davidwendt
changed the title
bdice
approved these changes
Aug 16, 2023
|
/merge |
etseidl
pushed a commit
to etseidl/cudf
that referenced
this pull request
Aug 17, 2023
…rs (rapidsai#13807) Switching to use `cuco::experimental::static_map` for storing the unique merge-pair strings that can be looked up by `string_view`. This takes advantage of a feature of the `static_map` that allows storing with one key (index to a string entry) and lookup with a different type (string). The map uses a hash on the string for storing the index but allows lookup by string since the hash of string can resolve the entry and duplicates can be resolved by comparing the string with row entries. Authors: - David Wendt (https://github.com/davidwendt) - Yunsong Wang (https://github.com/PointKernel) Approvers: - Yunsong Wang (https://github.com/PointKernel) - Bradley Dice (https://github.com/bdice) URL: rapidsai#13807
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Switching to use
cuco::experimental::static_mapfor storing the unique merge-pair strings that can be looked up bystring_view. This takes advantage of a feature of thestatic_mapthat allows storing with one key (index to a string entry) and lookup with a different type (string). The map uses a hash on the string for storing the index but allows lookup by string since the hash of string can resolve the entry and duplicates can be resolved by comparing the string with row entries.Checklist