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

Remove macros that inspect the contents of exceptions #12076

Merged
merged 8 commits into from
Nov 8, 2022
Merged

Remove macros that inspect the contents of exceptions #12076

merged 8 commits into from
Nov 8, 2022

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Nov 4, 2022 •
edited
Loading

Description

We should not be encouraging users to rely specific error messages. Anywhere that is currently doing so is likely an indication that libcudf should be throwing a more specific type of exception instead of just a cudf::logic_error. This PR removes the testing utilities that were previously used for this purpose and reworks the relevant tests.

Related to #10200.

Checklist

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

@vyasr vyasr added 3 - Ready for Review Ready for review by team code quality libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 4, 2022
@vyasr vyasr requested a review from jrhemstad November 4, 2022 20:48
@vyasr vyasr self-assigned this Nov 4, 2022
@vyasr vyasr requested a review from a team as a code owner November 4, 2022 20:48
@vyasr vyasr requested a review from karthikeyann November 4, 2022 20:48
bdice
bdice requested changes Nov 4, 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.

For all the cases where the message is being removed and lacks context, we should retain an inline code comment that copies the message. That helps explain to readers what is happening in the test and why the error is supposed to occur.

cpp/tests/groupby/count_scan_tests.cpp Show resolved Hide resolved
@jrhemstad
Copy link
Contributor

Note that @codereport had started a similar thing a while back #10637

@vyasr
Copy link
Contributor Author

vyasr commented Nov 4, 2022

Note that @codereport had started a similar thing a while back #10637

He started out the removal of parsing exceptions in Python and removing these C++ macros from tests was identified as a task in that PR, but it is out of scope for that PR. I've isolated just that change here without touching cases where Python is actually using these right now. I'll address those separately.

@vyasr vyasr requested a review from bdice November 4, 2022 22:39
@vyasr vyasr changed the title Refactor/remove throw message macros Remove macros that inspect the contents of exceptions Nov 4, 2022
@codecov
Copy link

codecov bot commented Nov 4, 2022 •
edited
Loading

Codecov Report

Base: 87.47% // Head: 88.05% // Increases project coverage by +0.57% 🎉

Coverage data is based on head (cbca6c4) compared to base (f817d96).
Patch has no changes to coverable lines.

❗ Current head cbca6c4 differs from pull request most recent head 0948b46. Consider uploading reports for the commit 0948b46 to get more accurate results

Additional details and impacted files 
@@               Coverage Diff                @@
##           branch-22.12   #12076      +/-   ##
================================================
+ Coverage         87.47%   88.05%   +0.57%     
================================================
  Files               133      135       +2     
  Lines             21826    22025     +199     
================================================
+ Hits              19093    19394     +301     
+ Misses             2733     2631     -102
Impacted Files Coverage Δ
python/cudf/cudf/core/column/interval.py 85.45% <0.00%> (-9.10%) ⬇️
python/cudf/cudf/io/text.py 91.66% <0.00%> (-8.34%) ⬇️
python/strings_udf/strings_udf/__init__.py 76.47% <0.00%> (-7.85%) ⬇️
python/cudf/cudf/core/_base_index.py 81.28% <0.00%> (-4.27%) ⬇️
python/cudf/cudf/io/json.py 92.06% <0.00%> (-2.68%) ⬇️
python/cudf/cudf/utils/utils.py 89.91% <0.00%> (-0.69%) ⬇️
python/cudf/cudf/core/column/timedelta.py 90.17% <0.00%> (-0.58%) ⬇️
python/cudf/cudf/core/column/datetime.py 89.21% <0.00%> (-0.51%) ⬇️
python/cudf/cudf/core/column/column.py 87.96% <0.00%> (-0.46%) ⬇️
python/dask_cudf/dask_cudf/core.py 73.72% <0.00%> (-0.41%) ⬇️
... and 39 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

ttnghia
ttnghia approved these changes Nov 6, 2022
View reviewed changes
Copy link
Contributor

@ttnghia ttnghia left a comment

Choose a reason for hiding this comment

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

Support this work. Many times I had issue with fixing failed tests due to the error messages.

davidwendt
davidwendt approved these changes Nov 7, 2022
View reviewed changes
Copy link
Contributor

@davidwendt davidwendt left a comment

Choose a reason for hiding this comment

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

I liked how you turned the message text into a comment in the tests so that information is not lost.

bdice
bdice approved these changes Nov 7, 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 couple minor suggestions, feel free to accept or reject.

cpp/tests/io/nested_json_test.cpp Outdated Show resolved Hide resolved
karthikeyann
karthikeyann approved these changes Nov 7, 2022
View reviewed changes
Copy link
Contributor

@karthikeyann karthikeyann left a comment

Choose a reason for hiding this comment

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

LGTM 👍
The number of expect message throws, are much less than I expected.

@vyasr
Copy link
Contributor Author

vyasr commented Nov 7, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit b16b4ff into rapidsai:branch-22.12 Nov 8, 2022
@vyasr vyasr deleted the refactor/remove_throw_message_macros branch November 29, 2022 22:51
This was referenced Dec 16, 2022
Merged
Merged
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
@vyasr vyasr mentioned this pull request Mar 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidwendt davidwendt davidwendt approved these changes

@ttnghia ttnghia ttnghia approved these changes

@karthikeyann karthikeyann karthikeyann approved these changes

@bdice bdice bdice approved these changes

@jrhemstad jrhemstad Awaiting requested review from jrhemstad

Assignees

@vyasr vyasr

Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vyasr @jrhemstad @bdice @karthikeyann @ttnghia @davidwendt