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

[REVIEW] Fix `list` and `struct` meta generation issue in `dask-cudf` #10434

Merged

rapids-bot merged 11 commits into rapidsai:branch-22.04 from galipremsagar:8913

Mar 16, 2022

Contributor

galipremsagar commented Mar 14, 2022 •

edited

Loading

Fixes: #8913

This PR fixes multiple code-paths that incorrectly handle list and struct types for metadata generation in dask-cudf.

>>> ddf = dask_cudf.from_cudf(df, 1)
>>> ddf._meta_nonempty
   a         b                       c
0  0  [[0, 1]]  {'a': None, 'b': None}
1  1  [[0, 1]]  {'a': None, 'b': None}
>>> df
   a         b                        c
0  1  [[1, 2]]  {'a': 1, 'b': [[1, 2]]}
1  2  [[2, 3]]  {'a': 2, 'b': [[2, 3]]}
2  3      None      {'a': 3, 'b': None}
>>> ddf._meta_nonempty.c.dtype
StructDtype({'a': dtype('int64'), 'b': ListDtype(ListDtype(int64))})
>>> df.c.dtype
StructDtype({'a': dtype('int64'), 'b': ListDtype(ListDtype(int64))})
>>> ddf._meta_nonempty.b.dtype
ListDtype(ListDtype(int64))
>>> df.b.dtype
ListDtype(ListDtype(int64))

galipremsagar added 2 commits

March 14, 2022 16:31


          fix list and struct meta generation issue

da97131


          newline

f0daa52

galipremsagar added bug Python 4 - Needs cuDF (Python) Reviewer dask non-breaking labels

galipremsagar self-assigned this

galipremsagar requested review from a team as code owners

March 14, 2022 23:34

galipremsagar requested review from bdice and rgsl888prabhu

March 14, 2022 23:34

galipremsagar requested review from quasiben, rjzamora and jakirkham

March 14, 2022 23:36

codecov bot commented Mar 15, 2022 •

edited

Loading

Codecov Report

Merging #10434 (e6b2fa2) into branch-22.04 (4596244) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@               Coverage Diff                @@
##           branch-22.04   #10434      +/-   ##
================================================
+ Coverage         86.13%   86.17%   +0.03%     
================================================
  Files               139      139              
  Lines             22438    22466      +28     
================================================
+ Hits              19328    19361      +33     
+ Misses             3110     3105       -5

Impacted Files	Coverage Δ
python/cudf/cudf/core/tools/numeric.py	`89.24% <100.00%> (+0.11%)`	⬆️
python/dask_cudf/dask_cudf/backends.py	`86.32% <100.00%> (+1.34%)`	⬆️
...ython/dask_cudf/dask_cudf/io/tests/test_parquet.py	`100.00% <100.00%> (ø)`
python/cudf/cudf/core/column/string.py	`88.39% <0.00%> (+0.12%)`	⬆️
python/cudf/cudf/core/groupby/groupby.py	`91.57% <0.00%> (+0.22%)`	⬆️
python/cudf/cudf/core/column/numerical.py	`95.28% <0.00%> (+0.29%)`	⬆️
python/cudf/cudf/core/tools/datetimes.py	`84.49% <0.00%> (+0.30%)`	⬆️
python/cudf/cudf/core/column/lists.py	`90.56% <0.00%> (+0.47%)`	⬆️

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 deb39db...e6b2fa2. Read the comment docs.

galipremsagar added 2 commits

March 15, 2022 07:57


          Merge remote-tracking branch 'upstream/branch-22.04' into 8913

4eb0f50


          add more coverage and remove threshold

fa129d8

galipremsagar commented

View reviewed changes

python/dask_cudf/dask_cudf/backends.py Outdated Show resolved Hide resolved


          Update python/dask_cudf/dask_cudf/backends.py

f53b4b2

rjzamora approved these changes

View reviewed changes

Member

rjzamora left a comment

This looks great - Thanks @galipremsagar !

Just a few questions/suggestions related to comments.

python/dask_cudf/dask_cudf/backends.py Outdated Show resolved Hide resolved

python/cudf/cudf/core/dtypes.py Outdated Show resolved Hide resolved

python/dask_cudf/dask_cudf/backends.py Outdated Show resolved Hide resolved


          address reviews

da6b97b

bdice requested changes

View reviewed changes

Contributor

bdice left a comment

Suggestions attached. 👍

python/cudf/cudf/core/dtypes.py Outdated Show resolved Hide resolved

python/dask_cudf/dask_cudf/backends.py Outdated Show resolved Hide resolved

python/dask_cudf/dask_cudf/backends.py Outdated Show resolved Hide resolved

python/dask_cudf/dask_cudf/backends.py Show resolved Hide resolved

python/dask_cudf/dask_cudf/io/tests/test_parquet.py Outdated Show resolved Hide resolved

galipremsagar and others added 4 commits

March 15, 2022 12:24


          Update python/dask_cudf/dask_cudf/io/tests/test_parquet.py

079c996

Co-authored-by: Bradley Dice <[email protected]>


          Update python/dask_cudf/dask_cudf/backends.py

a6311a9

Co-authored-by: Bradley Dice <[email protected]>


          address reviews

c2c721a


          address reviews

27ff53f

galipremsagar requested a review from bdice

March 15, 2022 17:35

galipremsagar removed the 4 - Needs cuDF (Python) Reviewer label


          Merge remote-tracking branch 'upstream/branch-22.04' into 8913

e6b2fa2

bdice approved these changes

View reviewed changes

Contributor

bdice left a comment

LGTM, I have one comment to double-check. I'm not sure if I made an error in my previous suggestion with respect to number of nesting levels.

python/dask_cudf/dask_cudf/backends.py

+                          data = ["cat", "dog"]
+                      else:
+                          data = np.array([0, 1], dtype=leaf_type).tolist()
+                      data = _nest_list_data(data, s.dtype) * 2

Contributor

bdice Mar 16, 2022

Does s.dtype need to be s.dtype.leaf_type? I'm not sure if this is nesting an extra level or if this is the right number of levels. It might have been changed with my previous round of suggestions.

Contributor Author

galipremsagar Mar 16, 2022

leaf_type corresponds to the terminal type of ListDtype, i.e., like int32, int64..

by passing s.dtype(ListDtype) we traverse through each inner type by accessing .element_type in _nest_list_data such that we get the true nesting levels.

Contributor Author

galipremsagar Mar 16, 2022

For example:

>>> s.dtype
ListDtype(ListDtype(int64))
>>> s.leaf_type
int64
>>> s.element_type
ListDtype(int64)

galipremsagar removed the 4 - Needs Dask Reviewer label

Contributor Author

galipremsagar commented Mar 16, 2022

@gpucibot merge

rapids-bot bot merged commit 1649955 into rapidsai:branch-22.04

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

rjzamora rjzamora approved these changes

bdice bdice approved these changes

rgsl888prabhu Awaiting requested review from rgsl888prabhu rgsl888prabhu was automatically assigned from rapidsai/cudf-python-codeowners

quasiben Awaiting requested review from quasiben

jakirkham Awaiting requested review from jakirkham

Labels

bug dask non-breaking Python