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

adding cugraph-ops #3488

Merged
merged 9 commits into from
Apr 19, 2023
Merged

adding cugraph-ops #3488

merged 9 commits into from
Apr 19, 2023

Conversation

BradReesWork
Copy link
Member

@BradReesWork BradReesWork commented Apr 14, 2023

The goal is to have a unified docs area for all graph work. This PR is the first step in getting cugraph-ops included in the main graph doc area

@BradReesWork BradReesWork requested a review from a team as a code owner April 14, 2023 21:01
@BradReesWork BradReesWork marked this pull request as draft April 14, 2023 21:01
@BradReesWork BradReesWork added doc Documentation non-breaking Non-breaking change labels Apr 14, 2023
@BradReesWork BradReesWork marked this pull request as ready for review April 18, 2023 21:07
@BradReesWork BradReesWork requested a review from a team as a code owner April 18, 2023 21:07
Copy link
Member

@tingyu66 tingyu66 left a comment

Choose a reason for hiding this comment

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

Just need to rename pytorch.rst.txt -> pytorch.rst. LGTM otherwise.

Copy link
Contributor

@MatthiasKohl MatthiasKohl left a comment

Choose a reason for hiding this comment

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

Like @tingyu66 pointed out, the pytorch file naming is incorrect.

Additionally, I'm not sure what the plan is, but the C++ API docs are not found here: is the plan to just not add them at all, add them in a later PR, or should they be added immediately here?

:maxdepth: 2
:caption: API Documentation

graph_types
Copy link
Contributor

Choose a reason for hiding this comment

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

should also add pytorch here at the top IMO (just noticed that our own docs have them at the end, but pytorch is the main API for end-users so this should be the first thing to see)

Suggested change
graph_types
pytorch
graph_types

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding the C++ API is an upcoming step. Working on covering from Doxogen to Sphinx. Once I have the process setup for the other cugraph C code, then I'll be rolling in cugraph-ops.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you now added the pytorch entry twice, as well as not adding it at the top as suggested above.
Let's just remove one of these instances, or completely remove this if it doesn't pass CI

@BradReesWork
Copy link
Member Author

Just need to rename pytorch.rst.txt -> pytorch.rst. LGTM otherwise.

I was getting an error in the pytorch.rst file and left it out so that I could get the basic framework in place and verify that and external python package could be in the docs

Copy link
Member

@tingyu66 tingyu66 left a comment

Choose a reason for hiding this comment

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

👍

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.

Overall looks good to me. I do think it's a little odd to add docs to components that live in another repo here, but I can't think of a better solution now.

docs/cugraph/source/index.rst Show resolved Hide resolved
@BradReesWork
Copy link
Member Author

a little odd to add docs to components that live in another repo

I have been pondering how to create a unified docs page that brings in all the various sections. Most examples from other products use a central repo that clones all the other repos into one so that docs can be created. I'm trying to avoid that.

Yes it is odd to have stuff from another repo - but it all falls under Graph

Copy link
Contributor

@acostadon acostadon 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

@rlratzel
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit 814b1d6 into rapidsai:branch-23.06 Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Documentation non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants