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 except declaration in Cython interface for regex_program::create #13054

Merged

Conversation

davidwendt
Copy link
Contributor

Description

Add the except + declaration to the cudf::strings::regex_program::create() function in the Cython regex_program.pxd interface since invalid regex patterns are thrown by this call. This allows the normal Cython exception handling to pass the exception to the Python logic without aborting the process.

Closes #13052

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 bug Something isn't working 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. Cython non-breaking Non-breaking change labels Apr 4, 2023
@davidwendt davidwendt requested a review from a team as a code owner April 4, 2023 15:46
@davidwendt davidwendt self-assigned this Apr 4, 2023
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Changes look good. Should we add a test?

@bdice
Copy link
Contributor

bdice commented Apr 4, 2023

Proposed test:

def test_invalid_regex():
    s = cudf.Series(["a"])
    with pytest.raises(RuntimeError):    # Might be the wrong error class, please verify
        s.str.extract("{\}")    # Should this be a raw string like `r"{\}"`?

@davidwendt
Copy link
Contributor Author

Changes look good. Should we add a test?

Yes, thanks.
Also, I was wondering about changing the libcudf exception to something more specific like std::invalid_argument for poorly formed regex pattern. I could do that in a follow up PR. Just requesting opinions.

Copy link
Contributor

@shwina shwina left a comment

Choose a reason for hiding this comment

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

LGTM, pending @bdice's suggestions for testing

@bdice
Copy link
Contributor

bdice commented Apr 4, 2023

Also, I was wondering about changing the libcudf exception to something more specific like std::invalid_argument for poorly formed regex pattern. I could do that in a follow up PR. Just requesting opinions.

I think that’d be fine. I am not up to date on the latest guidelines / changes for exception types so I don’t know exactly what to recommend. I know we’ve been pushing to improve that recently.

@davidwendt
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit 7a739ce into rapidsai:branch-23.06 Apr 5, 2023
@davidwendt davidwendt deleted the cython-regex-program-exception branch April 5, 2023 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team bug Something isn't working non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

[BUG] Exceptions thrown in regcomp.cpp seem to not propagate up to Cython (leads to SIGABRT)
3 participants