Rework `Scalar` imports #10791

brandon-b-miller · 2022-05-04T19:57:58Z

This PR changes the way things are imported in our scalar code such that it's less prone to circular import issues. For instance before this we can't make NA the default value of a kwarg for, say, anything in column.py.

codecov · 2022-05-04T22:37:09Z

Codecov Report

Merging #10791 (6aa2de2) into branch-22.06 (027c34a) will decrease coverage by 0.11%.
The diff coverage is 94.11%.

@@               Coverage Diff                @@
##           branch-22.06   #10791      +/-   ##
================================================
- Coverage         86.40%   86.29%   -0.12%     
================================================
  Files               143      144       +1     
  Lines             22444    22491      +47     
================================================
+ Hits              19393    19408      +15     
- Misses             3051     3083      +32

Impacted Files	Coverage Δ
python/cudf/cudf/core/scalar.py	`89.01% <90.90%> (-0.31%)`	⬇️
python/cudf/cudf/__init__.py	`90.69% <100.00%> (+0.22%)`	⬆️
python/cudf/cudf/core/missing.py	`100.00% <100.00%> (ø)`
python/cudf/cudf/core/udf/typing.py	`97.54% <100.00%> (ø)`
...thon/dask_cudf/dask_cudf/tests/test_distributed.py	`18.86% <0.00%> (-67.93%)`	⬇️
python/cudf/cudf/core/indexed_frame.py	`91.70% <0.00%> (ø)`
python/cudf/cudf/testing/_utils.py	`94.05% <0.00%> (+0.06%)`	⬆️
python/cudf/cudf/core/dataframe.py	`93.77% <0.00%> (+0.08%)`	⬆️
python/cudf/cudf/core/reshape.py	`90.11% <0.00%> (+0.11%)`	⬆️
python/cudf/cudf/core/column/string.py	`89.21% <0.00%> (+0.12%)`	⬆️
... and 3 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 6128e0d...6aa2de2. Read the comment docs.

vyasr

One request that can be addressed in a follow-up, otherwise LGTM.

vyasr · 2022-05-05T20:53:48Z

python/cudf/cudf/core/scalar.py

+                isinstance(other, np.generic) or other.is_valid
+            )
+            if not valid:
+                return Scalar(None, dtype=out_dtype)


I realize it's not in scope, so I'd be perfectly happy with a follow-up, but this seems like an important case to have tested somewhere. It's basically for any binary op between two scalars where one is NA, right? I'm surprised that's not tested, I guess because we only test column + scalar?

(Approving but would also like to see tests for this at some point.)

bdice

I thought I'd leave a review summary but then I was like NA.

python/cudf/cudf/core/missing.py

python/cudf/cudf/__init__.py

bdice · 2022-05-06T20:46:38Z

python/cudf/cudf/core/scalar.py

 from cudf.core.dtypes import ListDtype, StructDtype
-from cudf.core.index import BaseIndex
+from cudf.core.missing import NA


Since cudf is already imported, do we want to use cudf.NA rather than import this name? I see quite a few files that use cudf.NA rather than importing NA separately. I don't think any other files use this convention.

This is a really great question. We use cudf.NA almost everywhere, but should we? In some ways it feels like it might fit the "onion model" a bit better if scalar.py didn't mention the top level cudf namespace anywhere.

I think there's probably plenty of places I have used cudf.NA throughout the codebase as a convenience since those places already have cudf imported, but I realize now that it has the negative consequence of further entrenching the need for import cudf in those places, which is part of what I think gives rise to these import issues.

Is this way off base? I am happy to put in a PR that addresses this for NA across the codebase as a follow up if that's what we think should happen, and just imports from missing.

In the long run I would love to get rid of import cudf almost everywhere. It is a code smell that almost always (there are always exceptions) indicates circular dependencies that should not exist. Unfortunately a lot of those circular dependencies are baked deeply into our code right now and are hard to excise. If you could get rid of these on a case-by-case basis (in this instance, for cudf.NA), I think that would be good for us in the long run.

thanks @vyasr I think that lines up with my understanding. I am going to leave this as-is for now and look into making the change everywhere. I'll leave this thread open though and let @bdice resolve if this conclusion seems satisfactory.

Removing import cudf sounds fine to me! Could we make an issue to document the plan before resolving this? (I would do it but I’m on mobile at the moment.)

I raised #10820 regarding this. I'll tackle this for NA first after this PR is merged, after which it should be importable from missing. From there I'll start probing what it takes to address this more generally.

python/cudf/cudf/core/missing.py

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

bdice

Approving — would like to see a test added (in a later PR, perhaps) and an issue filed for the plan to remove internal uses of import cudf.

bdice · 2022-05-09T23:30:08Z

python/cudf/cudf/core/scalar.py

+                isinstance(other, np.generic) or other.is_valid
+            )
+            if not valid:
+                return Scalar(None, dtype=out_dtype)


(Approving but would also like to see tests for this at some point.)

brandon-b-miller · 2022-05-10T14:39:30Z

@gpucibot merge

…0821) This PR changes cuDF so `NA` isn't used around the codebase from the top level `cudf` namespace and rather is imported directly from `missing`. This is part of #10820 and comes as a follow up to #10791 (comment) Authors: - https://github.com/brandon-b-miller Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) URL: #10821

Add tests for binaryops between two null scalars as per #10791 (comment) This is technically `1307` tests but they only take about 5 seconds to run. Authors: - https://github.com/brandon-b-miller Approvers: - GALI PREM SAGAR (https://github.com/galipremsagar) URL: #10828

rework imports

a0fd101

brandon-b-miller added 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 4, 2022

brandon-b-miller requested a review from a team as a code owner May 4, 2022 19:57

brandon-b-miller requested review from trxcllnt and bdice May 4, 2022 19:57

vyasr approved these changes May 5, 2022

View reviewed changes

bdice requested changes May 6, 2022

View reviewed changes

brandon-b-miller and others added 3 commits May 9, 2022 11:39

Apply suggestions from code review

5358172

Co-authored-by: Bradley Dice <bdice@bradleydice.com>

address reviews, fix tests

f63a958

fix style

6aa2de2

bdice approved these changes May 9, 2022

View reviewed changes

brandon-b-miller mentioned this pull request May 10, 2022

Reduce usage of import cudf internally #10820

Open

rapids-bot bot merged commit 19c5bad into rapidsai:branch-22.06 May 10, 2022

brandon-b-miller mentioned this pull request May 10, 2022

Import NA from missing rather than using cudf.NA everywhere #10821

Merged

brandon-b-miller mentioned this pull request May 11, 2022

Add tests for null scalar binaryops #10828

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework `Scalar` imports #10791

Rework `Scalar` imports #10791

brandon-b-miller commented May 4, 2022

codecov bot commented May 4, 2022 •

edited

Loading

vyasr left a comment

vyasr May 5, 2022

bdice May 9, 2022

bdice left a comment

bdice May 6, 2022

brandon-b-miller May 9, 2022

vyasr May 9, 2022 •

edited

Loading

brandon-b-miller May 9, 2022

bdice May 9, 2022

brandon-b-miller May 10, 2022

bdice left a comment

bdice May 9, 2022

brandon-b-miller commented May 10, 2022

Rework Scalar imports #10791

Rework Scalar imports #10791

Conversation

brandon-b-miller commented May 4, 2022

codecov bot commented May 4, 2022 • edited Loading

Codecov Report

vyasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vyasr May 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandon-b-miller commented May 10, 2022

Rework `Scalar` imports #10791

Rework `Scalar` imports #10791

codecov bot commented May 4, 2022 •

edited

Loading

vyasr May 9, 2022 •

edited

Loading