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

Replace custom cached_property implementation with functools #10272

Merged
merged 4 commits into from
Feb 14, 2022

Conversation

shwina
Copy link
Contributor

@shwina shwina commented Feb 11, 2022

Replaces our custom cached_property with that provided by functools since Python 3.8. This uncovered a couple of typing bugs that previously eluded us.

@shwina shwina requested a review from a team as a code owner February 11, 2022 18:34
@github-actions github-actions bot added the Python Affects Python cuDF API. label Feb 11, 2022
@shwina shwina added tech debt non-breaking Non-breaking change Python Affects Python cuDF API. and removed Python Affects Python cuDF API. labels Feb 11, 2022
@@ -91,7 +91,7 @@ def _num_rows(self) -> int:

@property
def _column_names(self) -> List[Any]: # TODO: List[str]?
return self._data.names
return list(self._data.names)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We previously returned a tuple while hinting that we were returning a list. Which one should we return?

Copy link
Contributor

Choose a reason for hiding this comment

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

From the perspective of immutability, a tuple. However, I have come across a few developer mistakes where the result of this property was used as if it were a list, perhaps because of the incorrect typing. cc: @isVoid who ran into this with me in this commit: a4d88c3

@bdice bdice added the improvement Improvement / enhancement to an existing function label Feb 11, 2022
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.

Copyright checks are failing. I commented on the thread about tuple/list. Pending resolution of that thread, this looks good.

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.

LGTM.

@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #10272 (6768825) into branch-22.04 (a7d88cd) will increase coverage by 0.25%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff                @@
##           branch-22.04   #10272      +/-   ##
================================================
+ Coverage         10.42%   10.67%   +0.25%     
================================================
  Files               119      122       +3     
  Lines             20603    20866     +263     
================================================
+ Hits               2148     2228      +80     
- Misses            18455    18638     +183     
Impacted Files Coverage Δ
python/cudf/cudf/_fuzz_testing/fuzzer.py 0.00% <ø> (ø)
python/cudf/cudf/_fuzz_testing/io.py 0.00% <ø> (ø)
python/cudf/cudf/_fuzz_testing/main.py 0.00% <ø> (ø)
python/cudf/cudf/_version.py 0.00% <ø> (ø)
python/cudf/cudf/comm/gpuarrow.py 0.00% <ø> (ø)
python/cudf/cudf/core/_base_index.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/categorical.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/column.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/datetime.py 0.00% <ø> (ø)
python/cudf/cudf/core/column/methods.py 0.00% <ø> (ø)
... and 63 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 48c4dc3...6768825. Read the comment docs.

@shwina
Copy link
Contributor Author

shwina commented Feb 14, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 374b387 into rapidsai:branch-22.04 Feb 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants