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

Remove Cython APIs for table view generation #9199

Merged

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Sep 8, 2021

This PR removes the Table methods used to generate cudf::table_view objects in Cython and switches all code paths to use the underlying cdef function that those functions originally called under the hood. Many of the view-generating functions were actually never used (all the mutable view generators and index_view), so those have been removed. Moving in this direction has two benefits. First, it allows us to change our Cython APIs to accept iterables of columns rather than Table objects, which in turn means that we can get rid of various Column->Table->Column round-trips that are currently in place just to call Cython APIs on Columns that are only defined on Tables. Second, the Table class is now pure Python, so its logic can be merged into the Frame class.

@vyasr vyasr added 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. Cython improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 8, 2021
@vyasr vyasr added this to the CuDF Python Refactoring milestone Sep 8, 2021
@vyasr vyasr self-assigned this Sep 8, 2021
@vyasr vyasr requested a review from a team as a code owner September 8, 2021 22:20
@vyasr
Copy link
Contributor Author

vyasr commented Sep 8, 2021

This only affects packages relying on our Cython directly rather than using public Python APIs, but just in case CCing @rlratzel for cuGraph, @dantegd for cuML, and @thomcom for cuSpatial.

@vyasr vyasr force-pushed the refactor/make_table_pure_python branch from 958bd9f to 582ef25 Compare September 10, 2021 16:53
Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

Same issue as above.

python/cudf/cudf/_lib/interop.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/strings/combine.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/transform.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/transform.pyx Outdated Show resolved Hide resolved
python/cudf/cudf/_lib/transpose.pyx Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 13, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.10@4349232). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.10    #9199   +/-   ##
===============================================
  Coverage                ?   10.82%           
===============================================
  Files                   ?      115           
  Lines                   ?    19166           
  Branches                ?        0           
===============================================
  Hits                    ?     2074           
  Misses                  ?    17092           
  Partials                ?        0           

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 4349232...5b487c6. Read the comment docs.

@vyasr vyasr requested a review from isVoid September 13, 2021 21:35
@vyasr
Copy link
Contributor Author

vyasr commented Sep 13, 2021

@isVoid OK I went through and checked all the files. I think I got all the places where I was passing through a raw boolean.

Copy link
Contributor

@isVoid isVoid left a comment

Choose a reason for hiding this comment

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

Thanks, this mostly looks good from my end. But I think we should wait until the maintainers mentioned above comment on this.

@vyasr
Copy link
Contributor Author

vyasr commented Sep 13, 2021

Thanks, this mostly looks good from my end. But I think we should wait until the maintainers mentioned above comment on this.

Agreed, I don't plan to merge until other maintainers can take a look.

@vyasr
Copy link
Contributor Author

vyasr commented Sep 13, 2021

Got the OK on this from @dantegd offline

@rlratzel
Copy link
Contributor

I re-checked cuGraph and didn't see anything that's using these Cython APIs, so this change should be safe for cuGraph. I believe we're only using public cuDF python APIs in the pure python layer.

@vyasr
Copy link
Contributor Author

vyasr commented Sep 14, 2021

OK great. I've opened rapidsai/cuspatial#449 to make cuspatial work after this PR, so we should be good to go once this PR is merged.

@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge breaking Breaking change and removed 3 - Ready for Review Ready for review by team non-breaking Non-breaking change labels Sep 15, 2021
@galipremsagar
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 70bc3f0 into rapidsai:branch-21.10 Sep 15, 2021
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request Sep 17, 2021
This makes cuspatial compatible with the changes in rapidsai/cudf#9199.

Authors:
  - Vyas Ramasubramani (https://github.com/vyasr)

Approvers:
  - H. Thomson Comer (https://github.com/thomcom)

URL: #449
@vyasr vyasr deleted the refactor/make_table_pure_python branch January 14, 2022 18:04
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 breaking Breaking change improvement Improvement / enhancement to an existing function Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants