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

Update calls to cuDF Series ctors, bug fix to cugraph.subgraph() for handling non-renumbered Graphs #1901

Merged

Conversation

rlratzel
Copy link
Contributor

@rlratzel rlratzel commented Oct 23, 2021

  • Updates for latest cuDF which changes how Series objects are constructed from Buffer types.
  • Updated ci/gpu/build.sh to add the conda env bin dir to PATH, since it was not finding the correct cmake during a local run.
  • Cleaned up various cython imports.
  • Added test and bug fix to ensure a graph that has not been renumbered is handled correctly by cugraph.subgraph().
  • Updated pytest.ini for users that don't have the required extra pytest plugins installed, left comments in place to for users to re-enable.
  • Minor test and code cleanup.

closes #1908
closes #1899

Note: this PR was originally intended to be two PRs: one for the subgraph bug fix, and another for the cudf.Series updates. I accidentally pushed the Series updates to the wrong branch, hence this combined PR. I can separate them if reviewing is too difficult.

@rlratzel rlratzel added bug Something isn't working non-breaking Non-breaking change labels Oct 23, 2021
@rlratzel rlratzel added this to the 21.12 milestone Oct 23, 2021
@rlratzel rlratzel self-assigned this Oct 23, 2021
@rlratzel rlratzel requested a review from a team as a code owner October 23, 2021 03:12
@codecov-commenter
Copy link

codecov-commenter commented Oct 23, 2021

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.12    #1901   +/-   ##
===============================================
  Coverage                ?   70.13%           
===============================================
  Files                   ?      143           
  Lines                   ?     8812           
  Branches                ?        0           
===============================================
  Hits                    ?     6180           
  Misses                  ?     2632           
  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 61b99df...f375e75. Read the comment docs.

…ted from Buffers. Also cleaned up various cython imports and some minor test cleanup.
…ore cleanup to cython imports (still testing).
@rlratzel rlratzel requested a review from a team as a code owner October 27, 2021 02:06
@rlratzel rlratzel changed the title Added test and bug fix to ensure a graph that has not been renumbered is handled correctly by cugraph.subgraph() Update calls to cuDF Series ctors, bug fix to cugraph.subgraph() for handling non-renumbered Graphs Oct 27, 2021
Copy link
Collaborator

@ChuckHastings ChuckHastings left a comment

Choose a reason for hiding this comment

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

This solves the problem and hopefully will unblock CI. One question added. I'm OK either way... although it might be better if we resolve the question and make the changes if the answer is it would be better with the change.

from libc.stdint cimport uintptr_t


cdef move_device_buffer_to_series(unique_ptr[device_buffer] device_buffer_unique_ptr,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Question... This implementation generates a column and converts it into a Series. In many of the algorithms we generate a Series using this function to create a column in a data frame.

Would it be better to generate a column and leave it as a column? Perhaps we should have a move_device_buffer_to_series and a move_device_buffer_to_column method?

Or am I reading too much into this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ChuckHastings , I like that idea and I'm testing out your suggestion now. In the interest of unblocking our other PRs by fixing the CI failures here, I'll ask @BradReesWork to merge this now and then I'll create a follow-up PR that implements your suggestion.

@BradReesWork
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit e2af5de into rapidsai:branch-21.12 Oct 27, 2021
rapids-bot bot pushed a commit that referenced this pull request Nov 3, 2021
…eries objects (#1915)

This is a follow-up PR to address [this feedback](#1901 (comment)) from #1901 

This adds a separate cython helper for moving a buffer to a column to allow for a slightly more efficient call where column objects can be used.

Authors:
  - Rick Ratzel (https://github.com/rlratzel)

Approvers:
  - Brad Rees (https://github.com/BradReesWork)
  - Chuck Hastings (https://github.com/ChuckHastings)
  - Joseph Nke (https://github.com/jnke2016)

URL: #1915
@rlratzel rlratzel deleted the branch-21.12-subgraphrenumberfix branch June 17, 2022 00:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working non-breaking Non-breaking change
Projects
None yet
5 participants