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

[DOC] Improved deprecation warnings. #9347

Merged
merged 3 commits into from
Oct 5, 2021
Merged

[DOC] Improved deprecation warnings. #9347

merged 3 commits into from
Oct 5, 2021

Conversation

bdice
Copy link
Contributor

@bdice bdice commented Sep 30, 2021 •
edited
Loading

This PR improves deprecation warnings. I fixed some typos to make it easier to grep for deprecations, and added the warning type DeprecationWarning where appropriate. (Found these issues while looking for examples of deprecations for #9291.)

@bdice bdice requested review from a team as code owners September 30, 2021 14:23
@bdice bdice added the 3 - Ready for Review Ready for review by team label Sep 30, 2021
@github-actions github-actions bot added the Python Affects Python cuDF API. label Sep 30, 2021
@bdice bdice self-assigned this Sep 30, 2021
@bdice bdice added doc Documentation non-breaking Non-breaking change labels Sep 30, 2021
@bdice
Copy link
Contributor Author

bdice commented Sep 30, 2021

Another important consideration is that DeprecationWarning is hidden by default. We may wish to use a custom deprecation warning class that inherits from UserWarning or FutureWarning. I've used that strategy in the past (example).

@codecov
Copy link

codecov bot commented Sep 30, 2021 •
edited
Loading

Codecov Report

Merging #9347 (53f16f6) into branch-21.12 (ab4bfaa) will decrease coverage by 0.02%.
The diff coverage is 2.59%.

❗ Current head 53f16f6 differs from pull request most recent head 8aff51e. Consider uploading reports for the commit 8aff51e to get more accurate results
Impacted file tree graph

@@               Coverage Diff                @@
##           branch-21.12    #9347      +/-   ##
================================================
- Coverage         10.79%   10.76%   -0.03%     
================================================
  Files               116      116              
  Lines             18869    18995     +126     
================================================
+ Hits               2036     2045       +9     
- Misses            16833    16950     +117
Impacted Files Coverage Δ
python/cudf/cudf/core/_base_index.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/datetime.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/column/string.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/dataframe.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/frame.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/index.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/join/_join_helpers.py 0.00% <ø> (ø)
python/cudf/cudf/core/multiindex.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/series.py 0.00% <0.00%> (ø)
python/cudf/cudf/core/tools/datetimes.py 0.00% <0.00%> (ø)
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e63f455...8aff51e. Read the comment docs.

@vyasr
Copy link
Contributor

vyasr commented Sep 30, 2021 •
edited
Loading

Another important consideration is that DeprecationWarning is hidden by default. We may wish to use a custom deprecation warning class that inherits from UserWarning or FutureWarning. I've used that strategy in the past (example).

I think I included some discussion of how we should plan to handle deprecations in the future in the roadmap, but perhaps not in sufficient detail yet. FutureWarning is the appropriate choice here, as it is described here:

Base category for warnings about deprecated features when those warnings are intended for end users of applications that are written in Python.

Defining a custom class would work too but is probably not necessary and doesn't offer much benefit relative to standardizing across the Python ecosystem.

@bdice
Copy link
Contributor Author

bdice commented Sep 30, 2021

I have standardized all deprecations to use FutureWarning and this is ready for review.

@bdice bdice mentioned this pull request Oct 1, 2021
Merged
rapids-bot bot pushed a commit that referenced this pull request Oct 1, 2021
@bdice
Deprecate method parameters to DataFrame.join, DataFrame.merge. (#9291)
34b54ca 
The `method` parameter to `DataFrame.join` and `DataFrame.merge` [isn't used internally](https://github.com/rapidsai/cudf/blob/e2098e56f0cb209b1d916ce617c04533444a056a/python/cudf/cudf/core/join/join.py#L90) after changes in #7454. This PR updates the docstrings and adds deprecation notices via `FutureWarning` as discussed in #9347.

The parameter is now deprecated in the public API. I removed all internal uses of the `method` parameter.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - Charles Blackmon-Luca (https://github.com/charlesbluca)

URL: #9291
vyasr
vyasr approved these changes Oct 5, 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.

This LGTM. Technically we could mark this as breaking since it's modifying warnings that users might be catching, but since they're warnings and not errors and we've been really inconsistent about warnings so far anyway I'm choosing not to.

@vyasr
Copy link
Contributor

vyasr commented Oct 5, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 25f8363 into rapidsai:branch-21.12 Oct 5, 2021
@bdice bdice mentioned this pull request Oct 6, 2021
Merged
rapids-bot bot pushed a commit that referenced this pull request Oct 7, 2021
@bdice
Change all DeprecationWarnings to FutureWarning. (#9392)
187ab65 
Following up with some extra replacements of `DeprecationWarning` with `FutureWarning` that were missed in #9347 (these were added after I started that PR). From here forward, all PRs introducing deprecations of user APIs should use `FutureWarning`.

Authors:
  - Bradley Dice (https://github.com/bdice)

Approvers:
  - GALI PREM SAGAR (https://github.com/galipremsagar)

URL: #9392
@bdice bdice mentioned this pull request Jan 31, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VibhuJawa VibhuJawa VibhuJawa left review comments

@vyasr vyasr vyasr approved these changes

@shwina shwina shwina approved these changes

@galipremsagar galipremsagar galipremsagar approved these changes

@brandon-b-miller brandon-b-miller Awaiting requested review from brandon-b-miller brandon-b-miller is a code owner automatically assigned from rapidsai/cudf-python-codeowners

Assignees

@bdice bdice

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

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