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.

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

cuDF error handling document #7917

Merged
merged 21 commits into from
Aug 6, 2022
Merged

cuDF error handling document #7917

merged 21 commits into from
Aug 6, 2022

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Apr 8, 2021 •
edited
Loading

This document aims to give instruction the following two things:

  • What to throw given invalid user inputs
  • How should cuDF handle exceptions from libcudf

@isVoid isVoid added feature request New feature or request doc Documentation non-breaking Non-breaking change labels Apr 8, 2021
@isVoid isVoid self-assigned this Apr 8, 2021
@isVoid isVoid added 4 - Needs cuDF (Python) Reviewer 4 - Needs Review Waiting for reviewer to review or respond and removed feature request New feature or request labels Apr 8, 2021
vyasr
vyasr requested changes Apr 14, 2021
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Looks like a great start. Left some comments to keep the discussion moving along.

docs/cudf/source/errors.md Outdated Show resolved Hide resolved
docs/cudf/source/errors.md Outdated Show resolved Hide resolved
docs/cudf/source/errors.md Outdated Show resolved Hide resolved
docs/cudf/source/errors.md Outdated Show resolved Hide resolved
docs/cudf/source/errors.md Outdated Show resolved Hide resolved
docs/cudf/source/errors.md Outdated Show resolved Hide resolved
docs/cudf/source/errors.md Outdated Show resolved Hide resolved
docs/cudf/source/errors.md Outdated Show resolved Hide resolved
@isVoid isVoid changed the base branch from branch-21.06 to branch-21.08 July 16, 2021 18:50
@galipremsagar
Copy link
Contributor

Moving this PR to 21.12

@isVoid isVoid changed the base branch from branch-21.08 to branch-21.12 September 28, 2021 16:10
@sevagh sevagh added DO NOT MERGE Hold off on merging; see PR for details and removed DO NOT MERGE Hold off on merging; see PR for details labels Dec 15, 2021
vyasr
vyasr reviewed Jul 12, 2022
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

I think we ultimately want to merge this into #11235.

I'll try and put together a POC for the exception mapping.

docs/cudf/source/developer_guide/errors.md Outdated Show resolved Hide resolved
docs/cudf/source/developer_guide/errors.md Outdated Show resolved Hide resolved
docs/cudf/source/developer_guide/errors.md Outdated Show resolved Hide resolved
docs/cudf/source/developer_guide/errors.md Outdated Show resolved Hide resolved
docs/cudf/source/developer_guide/errors.md Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Python Affects Python cuDF API. label Jul 12, 2022
@isVoid isVoid added the DO NOT MERGE Hold off on merging; see PR for details label Jul 12, 2022
@bdice bdice changed the base branch from branch-22.08 to branch-22.10 August 2, 2022 21:43
@vyasr
Copy link
Contributor

vyasr commented Aug 4, 2022

@isVoid once #11235 is merged I would modify this PR to instead fill out the empty section in contributing_guide.md. Then I think this is probably pretty close to good (we can revise it as we update our exception handling policies).

@vyasr
Copy link
Contributor

vyasr commented Aug 4, 2022

@isVoid that PR is merged. You should be good to go now. I might also consider merging the options page in, but that can be done in a separate PR (if you agree that it would be better that way).

@isVoid isVoid removed the DO NOT MERGE Hold off on merging; see PR for details label Aug 5, 2022
isVoid
isVoid commented Aug 5, 2022
View reviewed changes
docs/cudf/source/developer_guide/index.md
@@ -21,6 +21,7 @@ Additionally, it includes longer sections on more specific topics like testing a
:maxdepth: 2

library_design
contributing_guide
Copy link
Contributor Author

@isVoid isVoid Aug 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@vyasr I assume you did not expose the contributing guide because it's incomplete. It should be ready to publish after we merged this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, thanks

vyasr
vyasr requested changes Aug 5, 2022
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

A few minor comments, then I think we can go ahead and merge. This section is pretty minimal for now, but still important to indicate our policies around what exceptions we throw when and what level of pandas compatibility we promise. Once we have custom exception mapping I expect this section to get a little more full.

docs/cudf/source/developer_guide/contributing_guide.md Outdated Show resolved Hide resolved
docs/cudf/source/developer_guide/contributing_guide.md Outdated Show resolved Hide resolved
docs/cudf/source/developer_guide/contributing_guide.md Outdated Show resolved Hide resolved
docs/cudf/source/developer_guide/contributing_guide.md Outdated Show resolved Hide resolved
docs/cudf/source/developer_guide/contributing_guide.md Outdated Show resolved Hide resolved
docs/cudf/source/developer_guide/contributing_guide.md Outdated Show resolved Hide resolved
docs/cudf/source/developer_guide/index.md
@@ -21,6 +21,7 @@ Additionally, it includes longer sections on more specific topics like testing a
:maxdepth: 2

library_design
contributing_guide
Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, thanks

@isVoid @vyasr
Adopt review changes
f88a2f2 
Co-authored-by: Vyas Ramasubramani <[email protected]>
@isVoid isVoid requested a review from vyasr August 5, 2022 18:00
bdice
bdice approved these changes Aug 5, 2022
View reviewed changes
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.

A few minor suggestions, otherwise good!

docs/cudf/source/developer_guide/contributing_guide.md Outdated Show resolved Hide resolved
docs/cudf/source/developer_guide/contributing_guide.md Outdated Show resolved Hide resolved
docs/cudf/source/developer_guide/contributing_guide.md Outdated Show resolved Hide resolved
vyasr
vyasr approved these changes Aug 5, 2022
View reviewed changes
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

Approving assuming @bdice's comments are addressed.

@isVoid @bdice
Address review comments.
afcd7ef 
Co-authored-by: Bradley Dice <[email protected]>
@isVoid
Copy link
Contributor Author

isVoid commented Aug 5, 2022

rerun tests

@isVoid
Copy link
Contributor Author

isVoid commented Aug 6, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit d695129 into rapidsai:branch-22.10 Aug 6, 2022
@isVoid isVoid deleted the fea-error-types branch August 6, 2022 01:34
@vyasr vyasr mentioned this pull request Dec 20, 2022
Merged
3 tasks
rapids-bot bot pushed a commit that referenced this pull request Jan 5, 2023
@vyasr
Remove arguments for checking exception messages in Python (#12424)
cf26425 
This PR removes support for checking exception messages from `assert_exceptions_equal`. See #12076 for the same change made in C++, #10200 for more context, and #7917 for the change in cudf Python's developer documentation where we officially changed our policy to not consider exception payloads part of the stable API.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - Ashwin Srinath (https://github.com/shwina)

URL: #12424
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vyasr vyasr vyasr approved these changes

@bdice bdice bdice approved these changes

@shwina shwina Awaiting requested review from shwina

Assignees

@isVoid isVoid

Labels
3 - Ready for Review Ready for review by team doc Documentation non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@isVoid @galipremsagar @vyasr @bdice @sevagh