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.

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

Propose extract_bfs_paths C API #1955

Merged
merged 9 commits into from
Dec 1, 2021
Merged

Propose extract_bfs_paths C API #1955

merged 9 commits into from
Dec 1, 2021

Conversation

ChuckHastings
Copy link
Collaborator

@ChuckHastings ChuckHastings commented Nov 18, 2021 •
edited
Loading

This PR defines the C API for extract_bfs_paths. Implementation will follow once the API is approved.

Partially addresses #1891

@ChuckHastings ChuckHastings requested a review from a team as a code owner November 18, 2021 17:38
@ChuckHastings ChuckHastings self-assigned this Nov 18, 2021
@ChuckHastings ChuckHastings added 3 - Ready for Review feature request New feature or request non-breaking Non-breaking change labels Nov 18, 2021
@ChuckHastings ChuckHastings added this to the 22.02 milestone Nov 18, 2021
@ChuckHastings
Copy link
Collaborator Author

rerun tests

1 similar comment
@ChuckHastings
Copy link
Collaborator Author

rerun tests

@rlratzel
Copy link
Contributor

rerun tests

Rerunning to get latest updates that should fix build problem.

@rlratzel
Copy link
Contributor

I talked with @ChuckHastings offline, and something we discussed is that these new APIs could be used for general path extraction - not just for extracting paths from BFS results - and should be renamed without _bfs in the names (and other changes to decouple from BFS, if possible). For example, a general path extraction API could be immediately useful when we add SSSP to the C API, which returns results in the same format.

rlratzel
rlratzel reviewed Nov 20, 2021
View reviewed changes
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.

Just added a reminder for a FIXME we discussed.

Also, this PR could add a unit test too, which calls a stubbed out implementation. I personally would benefit from seeing how the API is meant to be used in the context of an actual BFS call.

@rlratzel
Copy link
Contributor

rerun tests

CI failed due to what looks like a problem Jenkins had in accessing github. Rerunning in case this was temporary.

@codecov-commenter
Copy link

codecov-commenter commented Nov 20, 2021 •
edited
Loading

Codecov Report

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

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-22.02    #1955   +/-   ##
===============================================
  Coverage                ?   70.28%           
===============================================
  Files                   ?      143           
  Lines                   ?     8922           
  Branches                ?        0           
===============================================
  Hits                    ?     6271           
  Misses                  ?     2651           
  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 550685c...6f1b475. Read the comment docs.

@ChuckHastings ChuckHastings requested a review from a team as a code owner November 22, 2021 18:55
rlratzel
rlratzel approved these changes Nov 22, 2021
View reviewed changes
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.

Thanks, LGTM. I had some small suggestions which need not hold up my approval.

ChuckHastings and others added 3 commits November 23, 2021 12:23
@ChuckHastings
make references to resource handle consistent
ad92178
@rlratzel
Adding workaround for build problem with CUDA 1.5 Enhanced Compat. th…
002defc 
…at will force building of the python package in the cpu build script.
@rlratzel
Updates to channel option to hopefully resolve CI problem where its i…
aa8acb1 
…nterpreted as a URL, saving the work dir for the python builds too.
@ChuckHastings ChuckHastings requested a review from a team as a code owner November 24, 2021 22:06
@BradReesWork
Copy link
Member

rerun tests

@rlratzel
set UCX_HANDLE_ERRORS to assist in debugging, added lines to also ins…
9fdd8de 
…tall pylibcugraph from the Proj Flash tarfile.
@rlratzel
Reverted Project Flash changes after discovering seg fault was due to…
da773cd 
… a cudf bug and not the way CI was building packages. Also updated cuda11.5 conda dev env file for 22.02 packages.
@rlratzel
Merge remote-tracking branch 'upstream/branch-22.02' into fea_extract…
6f1b475 
…_bfs_paths_api
seunghwak
seunghwak approved these changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@seunghwak seunghwak left a comment

Choose a reason for hiding this comment

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

Looks good to me, I have just a minor comment to think about.

cpp/include/cugraph_c/algorithms.h
* @brief Opaque paths result type
*
* Store the output of BFS or SSSP, computing predecessors and distances
* from a seed.
*/
typedef struct {
int align_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we better use int32_t instead of int if the intention here is to really enforce 4 byte alignment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Will fix in the implementation PR.

@ChuckHastings
Copy link
Collaborator Author

@gpucibot merge

@rapids-bot rapids-bot bot merged commit bf7cfec into rapidsai:branch-22.02 Dec 1, 2021
@ChuckHastings ChuckHastings deleted the fea_extract_bfs_paths_api branch February 1, 2022 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rlratzel rlratzel rlratzel approved these changes

@jjacobelli jjacobelli jjacobelli approved these changes

@seunghwak seunghwak seunghwak approved these changes

Assignees

@ChuckHastings ChuckHastings

Labels
feature request New feature or request non-breaking Non-breaking change
Projects
None yet
Milestone
22.02
Development

Successfully merging this pull request may close these issues.
6 participants
@ChuckHastings @rlratzel @codecov-commenter @BradReesWork @jjacobelli @seunghwak