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.

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

Add seed parameter to cudf hash_character_ngrams #17994

Open
wants to merge 7 commits into
base: branch-25.04
Choose a base branch
from

Conversation

davidwendt
Copy link
Contributor

Description

The seed parameter was added to nvtext::hash_character_ngrams in #17643 but was not added to the Python interface. This PR adds the parameter to the pylibcudf function and Python interface methods.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@davidwendt davidwendt added 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 12, 2025
@davidwendt davidwendt self-assigned this Feb 12, 2025
@davidwendt davidwendt requested a review from a team as a code owner February 12, 2025 19:23
@davidwendt davidwendt requested review from vyasr and Matt711 February 12, 2025 19:23
@github-actions github-actions bot added the pylibcudf Issues specific to the pylibcudf package label Feb 12, 2025
Copy link
Contributor

@mroeschke mroeschke left a comment

Choose a reason for hiding this comment

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

One minor comment otherwise LGTM

python/cudf/cudf/core/column/string.py Outdated Show resolved Hide resolved
@Matt711 Matt711 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Feb 13, 2025
@Matt711
Copy link
Contributor

Matt711 commented Feb 13, 2025

/merge

@davidwendt
Copy link
Contributor Author

I could use some help with the failures here.
It appears the pylibcudf pytest is running without the code changes for this PR.
https://github.com/rapidsai/cudf/actions/runs/13317529148/job/37197516084?pr=17994#step:9:9218
I mentions line 84 of generate_ngrams.pyx which has been updated and is now line 86: https://github.com/rapidsai/cudf/pull/17994/files#diff-3a8b3c8197dbbf4a83d6fa5f9bb36c95a6c74dec1ef180b69cb1415c1cf3fc8bL84
Line 84 no longer exists.

@Matt711
Copy link
Contributor

Matt711 commented Feb 13, 2025

I could use some help with the failures here. It appears the pylibcudf pytest is running without the code changes for this PR. https://github.com/rapidsai/cudf/actions/runs/13317529148/job/37197516084?pr=17994#step:9:9218 I mentions line 84 of generate_ngrams.pyx which has been updated and is now line 86: https://github.com/rapidsai/cudf/pull/17994/files#diff-3a8b3c8197dbbf4a83d6fa5f9bb36c95a6c74dec1ef180b69cb1415c1cf3fc8bL84 Line 84 no longer exists.

Looking at the logs. Its using the nightly version of cudf. I'm not sure why though.

@mroeschke
Copy link
Contributor

I could use some help with the failures here. It appears the pylibcudf pytest is running without the code changes for this PR. https://github.com/rapidsai/cudf/actions/runs/13317529148/job/37197516084?pr=17994#step:9:9218 I mentions line 84 of generate_ngrams.pyx which has been updated and is now line 86: https://github.com/rapidsai/cudf/pull/17994/files#diff-3a8b3c8197dbbf4a83d6fa5f9bb36c95a6c74dec1ef180b69cb1415c1cf3fc8bL84 Line 84 no longer exists.

Looking at the logs. Its using the nightly version of cudf. I'm not sure why though.

As posted offline, I suspect #17995 somehow changed the behavior of libcudf/pylibcudf/cudf not being installed from source

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge improvement Improvement / enhancement to an existing function non-breaking Non-breaking change pylibcudf Issues specific to the pylibcudf package Python Affects Python cuDF API.
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

3 participants