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

Refactor Python factories and remove usage of Table for libcudf output handling #8687

Merged
merged 56 commits into from
Aug 3, 2021

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Jul 8, 2021

This PR serves two purposes:

  1. Provide a simpler mode of interaction with libcudf APIs that does not involve constructing and then deconstructing a Table object.
  2. Provide a single fast path (the _from_data method) for constructing Frame-like objects from other Frame-like objects that uses the lowest common denominator (a dict of columns and an optional index) for all other current methods (e.g. _from_table and _copy_construct).

@vyasr vyasr added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jul 26, 2021
@vyasr vyasr requested review from shwina and isVoid and removed request for marlenezw and skirui-source July 26, 2021 22:10
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.

I don't see any red flags. Small comments on doc below

python/cudf/cudf/_lib/utils.pyx Outdated Show resolved Hide resolved
Co-authored-by: Michael Wang <isVoid@users.noreply.github.com>
@shwina
Copy link
Contributor

shwina commented Jul 29, 2021

Before merging, I'd check with cuML (cc: @dantegd) as well as cuGraph (cc: @BradReesWork) if/how this will break them.

@shwina
Copy link
Contributor

shwina commented Jul 29, 2021

cc: @thomcom for cuSpatial as well

@vyasr
Copy link
Contributor Author

vyasr commented Jul 29, 2021

I've already opened rapidsai/cuspatial#437 for when this PR gets merged, happy to help out if this affects cuML or cuGraph.

Copy link
Contributor

@rlratzel rlratzel left a comment

Choose a reason for hiding this comment

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

I mostly checked the API changes and did not dive too much into details. These changes do not look like they'll break cuGraph.

@shwina
Copy link
Contributor

shwina commented Aug 3, 2021

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 7f704d6 into rapidsai:branch-21.10 Aug 3, 2021
rapids-bot bot pushed a commit to rapidsai/cuspatial that referenced this pull request Aug 4, 2021
rapidsai/cudf#8687 changes the internals of cuDF and removes the means by which libcudf table objects are converted to cuDF Frames. This PR should be merged after that one to match those APIs. I have verified locally that this PR's Cython code compiles and runs against that branch of cuDF.

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

Approvers:
  - Paul Taylor (https://github.com/trxcllnt)

URL: #437
@vyasr vyasr deleted the refactor/factories branch January 14, 2022 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team 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.

5 participants