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

[REVIEW] Fix a NameError in meta dispatch API #7996

Merged
merged 2 commits into from
Apr 20, 2021

Conversation

galipremsagar
Copy link
Contributor

This PR fixes an incorrect usage with the correct usage of column(cudf.core.column).

cc: @charlesbluca

@galipremsagar galipremsagar added bug Something isn't working 3 - Ready for Review Ready for review by team dask Dask issue ! - Release non-breaking Non-breaking change labels Apr 19, 2021
@galipremsagar galipremsagar self-assigned this Apr 19, 2021
@galipremsagar galipremsagar requested a review from a team as a code owner April 19, 2021 20:48
@github-actions github-actions bot added the Python Affects Python cuDF API. label Apr 19, 2021
@charlesbluca
Copy link
Member

Does this try block at the bottom of the file

try:
from dask.dataframe.utils import group_split_dispatch, hash_object_dispatch
from cudf.core.column import column

Have anything to do with the original use of column? Wondering if _get_non_empty_data() can/should be placed inside this block.

@galipremsagar
Copy link
Contributor Author

Have anything to do with the original use of column? Wondering if _get_non_empty_data() can/should be placed inside this block.

_get_non_empty_data is also being used by other methods that are not related to group_split_dispatch & hash_object_dispatch. So it has to stay outside this try/catch block. But I did remove the column import in this block which will not make it confusing when someone works on an API in this file.

@kkraus14 kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs Dask Reviewer labels Apr 19, 2021
@codecov
Copy link

codecov bot commented Apr 19, 2021

Codecov Report

Merging #7996 (0542d32) into branch-0.19 (8e1ffd4) will decrease coverage by 0.00%.
The diff coverage is 80.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.19    #7996      +/-   ##
===============================================
- Coverage        82.74%   82.74%   -0.01%     
===============================================
  Files              103      103              
  Lines            17703    17702       -1     
===============================================
- Hits             14649    14648       -1     
  Misses            3054     3054              
Impacted Files Coverage Δ
python/dask_cudf/dask_cudf/backends.py 89.51% <80.00%> (-0.08%) ⬇️

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 8e1ffd4...0542d32. Read the comment docs.

@kkraus14 kkraus14 merged commit cdf7704 into rapidsai:branch-0.19 Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working dask Dask issue 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