Generate unified Python/C++ docs #1324

vyasr · 2023-08-10T00:54:19Z

Description

This PR leverages Breathe to pull the rmm C++ API documentation into the python Sphinx docs build, generating a single unified build of the documentation that supports cross-linking between language libraries and also simplifies cross-linking from other libraries that wish to link here (such as higher-level RAPIDS libraries that use both rmm's Python and C++ APIs).

Using Breathe requires changing the doxygen build to generate XML in addition to the usual HTML. It turns out that doxygen catches more doc issues (of the types fixed in #1317) when more build outputs are turned on, which is indicative of some bugs/limitations in doxygen, but nonetheless I've fixed the additional issues in this PR as well.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

harrism · 2023-08-15T23:25:12Z

include/rmm/mr/device/failure_callback_resource_adaptor.hpp

@@ -55,7 +55,7 @@ using failure_callback_t = std::function<bool(std::size_t, void*)>;
 * When implementing a callback function for allocation retry, care must be taken to avoid an
 * infinite loop. The following example makes sure to only retry the allocation once:
 *
- * @code{c++}
+ * @code{.cpp}


Does it work to just leave off the {.cpp}? By default, Doxygen uses the language of the file the @code is found in. Will that work for the unified case?

Unfortunately doxygen-generated XML does not correctly identify hpp files as C++ files. It does work for cpp files, but since rmm's files are all headers I needed to specify this. I do wonder if updating to a newer version of doxygen would improve the situation, and I may test that. I think #1317 needs merging before an y PR to update doxygen, so if the test is promising I'll sequence that work appropriately.

Try modifying the Doxyfile EXTENSION_MAPPING setting?

EXTENSION_MAPPING = cu=C++ \ cuh=C++ \ hpp=C++

That doesn't seem to work, but I'm not sure why. Oddly, specifying @code{.hpp} works (whether or not hpp is added to the EXTENSION_MAPPING), but the default inference of just @code doesn't.

python/docs/Makefile

harrism

This is a start, but I think this PR is mostly tackling the mechanics of getting RMM Python and C++ APIs unified.

I think the main thing to review here is the generated docs. The RMM C++ API reference is here (requires NVIDIA VPN):
https://downloads.rapids.ai/ci/rmm/pull-request/1324/61abbbc/docs/rmm/html/librmm_api/

I find that very hard to scroll through and read, and there's no table of contents or other useful organization / navigation. It's a wall of (somewhat tiny) text, with very little formatting, font sizes, or HRs to break it up. Also, the order is weird. Ideally this would be broken up conceptually into separate pages about Memory Resources, Containers, Execution Policies, and other concepts. Can we do that with doxygen groups and pages?

Also, there is no differentiation in the TOC to the left between the Python and C++ API References. They both are named "API Reference" (same in the main page here: https://downloads.rapids.ai/ci/rmm/pull-request/1324/61abbbc/docs/rmm/html/)

Also, the page "https://downloads.rapids.ai/ci/rmm/pull-request/1324/61abbbc/docs/rmm/html/basics/" is only about Python, but gets prominent placement as the very first thing. If we are going to unify docs, I think we should give C++ and Python equal footing, like we do in the RMM README.md (which is pretty comprehensive).

I definitely like the idea of integrating the docs, but I think it needs quite a bit of thought and work (and I'm happy to help more).

dependencies.yaml

include/rmm/mr/device/logging_resource_adaptor.hpp

python/docs/conf.py

python/docs/librmm_api.rst

vyasr · 2023-09-21T17:13:34Z

@harrism you beat me to the punch, time zones are tricky 😄 I had intended to start exactly that conversation during the US workday today. I've kicked that off on Slack, where we can discuss some of these ideas more before coming to any final conclusions here. I completely agree that the current layout is insufficient and we need to do more work.

vyasr · 2023-09-25T16:58:12Z

I've split out the doxygen fixes uncovered by enabling XML builds into #1348. I'll make a separate PR adding doxygen groups. Once both of those are merged we can revisit this PR using those groupings.

doxygen catches more doc issues (of the types fixed in #1317) when more build outputs are turned on, which is indicative of some bugs/limitations in doxygen. XML builds will be necessary to leverage Breathe (see #1324) so this PR enables XML builds and fixes the associated issues. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Rong Ou (https://github.com/rongou) URL: #1348

This PR adds doxygen groups for the various parts of the C++ API to help provide more context. This will also help with improving the docs experience in #1324. Authors: - Vyas Ramasubramani (https://github.com/vyasr) Approvers: - Mark Harris (https://github.com/harrism) URL: #1358

vyasr · 2023-10-27T23:23:10Z

In the following image you can see "Detailed Description" at the bottom of the page with nothing after it. Every page seems to have this. Can we suppress it?

(This should probably be for a follow up PR) I would like each of the groups to have some summary text at the top of the page explaining what the group contains with definitions and usage overview

All of libcudf's docs have the same issue. I just opened rapidsai/cudf#14341 to remove the extra section. However, that's the section where you would include the summary text that you want I think. So it sounds like what you'd prefer is to instead create a DoxygenLayout file in rmm and move the detailed description for the group to the top of the page. Those are our options, either we remove that section or we fill it in for each group. WDYT? Also feel free to comment on the cudf PR since I assume we'll want similar solutions for both repos.

This image shows the modules are sorted alphabetically. I think they should be sorted by "importance", or by logical order of introducing concepts. E.g. I think Memory Resources and Data Containers should be first, since that's what people will use most.

We control the order, I just alphabetized by default. Please propose an ordering! I'm happy to go with what you think makes sense.

Also you can see namespace rmm making an appearance on this page without explanation. This seems out of place.

That's coming from this line. I included that because everywhere in the docs where there is anything within the rmm namespace Sphinx tries to create a link, so without this Sphinx throws warnings about missing references. We have three options here:

Leave this empty namespace doc as is.
Find something useful to include along with this namespace doc.
Remove it and find a way to suppress a warning

I'm guessing you prefer 2 or 3, WDYT?

This image highlights one of my common issues with Python documentation sites. ...

I'd prefer to address this in a follow-up PR. The best ways to fix it would involve using an alternate theme for the documentation, but switching themes will affect more than just the C++ documentation and could involve other changes. I'd prefer to merge this PR largely as is since it makes the Sphinx C++ documentation no worse than the existing Sphinx Python documentation, then come back to improving the overall formatting in a follow-up PR that affects the entire Sphinx docs. In all likelihood this will involve something like https://github.com/rapidsai/sphinx-theme/.

There seems to be something wrong with this logging page. This must be the documentation for a function?

This was caused by the same issue you noticed in the doxygen docs, a mistake in the doxygen. I've fixed that now.

The search page is empty...

In Sphinx the Search page is empty until you actually perform a search for something

Clicking on "Module Index" under "librmm documentation" (which should be C++) takes me to the "Python Module Index" page, which probably doesn't need to be grouped by letter of the alphabet since it only has one letter. :)

Turns out the module index is specifically designed for Python, so I'll just remove that here. That's an autogenerated page, so not sure how much control we have over it, but if we can modify it I'd say that's a task for a follow-up PR since it's not specific to the unification here.

If I navigate to "Index" under "Indices and Tables", it collapses the TOC on the left so I can't see where I am. In general the TOC doesn't seem to have as many levels as it should, so I can't always tell where I am.

I agree that we should improve this, but I see similar issues even at the very top level of the documentation so I don't think it's something we should address in this PR. Let's do that in a follow-up to improve the docs overall.

harrism

Here's what I suggest for the ordering.

python/docs/librmm_docs/index.rst

harrism · 2023-10-31T00:10:18Z

group to the top of the page. Those are our options, either we remove that section or we fill it in for each group. WDYT?

I think we should fill it in for each group and move it under the title for the page. We can do that in a followup.

We control the order, I just alphabetized by default. Please propose an ordering! I'm happy to go with what you think makes sense.

Done.

We have three options here:

1. Leave this empty namespace doc as is.

2. Find something useful to include along with this namespace doc.

3. Remove it and find a way to suppress a warning

I think (2). I always err on the side of more documentation. :)

Turns out the module index is specifically designed for Python, so I'll just remove that here.

Yes please.

Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>

vyasr · 2023-10-31T17:14:46Z

I think (2). I always err on the side of more documentation. :)

Since that's focusing on content, let's do that in a follow-up PR where we also add documentation for the groups.

harrism · 2023-10-31T20:49:02Z

I think (2). I always err on the side of more documentation. :)

Since that's focusing on content, let's do that in a follow-up PR where we also add documentation for the groups.

Agree with all the followups, but can you file an issue with a checklist to capture them all? (can convert the checklist items to individual issues if needed).

vyasr · 2023-10-31T21:10:24Z

Yup, opened #1368.

… group

bdice

I have a few related comments on organization. This is such a dramatic improvement that I'm approving and want to ship it. I would support doing all further work in separate PRs.

bdice · 2023-11-01T22:22:47Z

python/docs/cpp.rst

+Welcome to the rmm C++ documentation!
+========================================


This shows up in the navigation bar (they call these "breadcrumbs").

I would prefer to do something like:

Suggested change

Welcome to the rmm C++ documentation!

========================================

rmm C++

=======

I agree -- never liked the "Welcome to" prefix on practically every Python package's docs.

bdice · 2023-11-01T22:24:10Z

python/docs/librmm_docs/index.rst

+librmm Documentation
+====================


This heading makes the breadcrumbs deeply nested. The navigation is awkward as a result.

Can we eliminate this level of the hierarchy entirely, so that it's something more like Home / rmm C++ / API Reference / Memory Resources? See if you can fuse this page's toctree into python/docs/cpp.rst instead.

bdice · 2023-11-01T22:26:40Z

python/docs/python.rst

+Welcome to the rmm Python documentation!
+========================================


As above, this makes for an awkward breadcrumb.

Suggested change

Welcome to the rmm Python documentation!

========================================

rmm Python

==========

ajschmidt8

Looks good to me.

Seems like we no longer need to generate/upload separate librmm HTML docs, right?

If that's correct, then we should remove the lines below from build_docs.sh:

https://github.com/vyasr/rmm/blob/9e81c7303816ef53419e8754e9dc5416044eea56/ci/build_docs.sh#L34-L35

Additionally, we'll want to set nightly: 0 in the file below on our documentation site:

https://github.com/rapidsai/docs/blob/4bbb6af839079b4ae0b16a162951ef67445b156e/_data/docs.yml#L219-L229

vyasr · 2023-11-02T22:31:13Z

/merge

vyasr added feature request New feature or request non-breaking Non-breaking change labels Aug 10, 2023

vyasr self-assigned this Aug 10, 2023

github-actions bot added Python Related to RMM Python API conda cpp Pertains to C++ code labels Aug 10, 2023

harrism reviewed Aug 15, 2023

View reviewed changes

vyasr force-pushed the feat/unify_docs branch from 141e98e to c5f082d Compare September 20, 2023 22:11

vyasr marked this pull request as ready for review September 20, 2023 23:49

vyasr requested review from a team as code owners September 20, 2023 23:49

vyasr requested review from wence- and bdice September 20, 2023 23:49

vyasr commented Sep 20, 2023

View reviewed changes

python/docs/Makefile Outdated Show resolved Hide resolved

harrism requested changes Sep 21, 2023

View reviewed changes

bdice reviewed Sep 21, 2023

View reviewed changes

vyasr mentioned this pull request Sep 25, 2023

Enable doxygen XML and fix issues #1348

Merged

3 tasks

vyasr mentioned this pull request Oct 11, 2023

Add groups to the doxygen docs #1358

Merged

3 tasks

vyasr force-pushed the feat/unify_docs branch from 7f2e35f to ca428fe Compare October 25, 2023 17:50

vyasr requested a review from a team as a code owner October 25, 2023 17:50

github-actions bot added CMake ci labels Oct 25, 2023

vyasr changed the base branch from branch-23.10 to branch-23.12 October 25, 2023 17:51

vyasr force-pushed the feat/unify_docs branch from ca428fe to 5e65212 Compare October 25, 2023 22:07

github-actions bot removed the CMake label Oct 25, 2023

vyasr added 2 commits October 26, 2023 09:30

Add errors group

8a42763

Add all memory resource subgroups to breathe

6ec93b4

vyasr force-pushed the feat/unify_docs branch from 5e65212 to 6ec93b4 Compare October 26, 2023 16:31

vyasr mentioned this pull request Oct 27, 2023

Remove unnecessary section rapidsai/cudf#14341

Closed

3 tasks

harrism reviewed Oct 31, 2023

View reviewed changes

python/docs/librmm_docs/index.rst Outdated Show resolved Hide resolved

vyasr and others added 3 commits October 31, 2023 10:11

Update python/docs/librmm_docs/index.rst

229a37f

Co-authored-by: Mark Harris <783069+harrism@users.noreply.github.com>

Fix incorrect grouping for logger

1c77fa7

Remove modindex

851661c

Merge remote-tracking branch 'origin/branch-23.12' into feat/unify_docs

d08a2cd

vyasr mentioned this pull request Oct 31, 2023

[FEA] Improve doxygen documentation #1368

Open

Add per_device_resource to memory_resources group

89ce300

harrism approved these changes Oct 31, 2023

View reviewed changes

vyasr added 5 commits October 31, 2023 16:53

Fix thrown error

3cc30f4

Add documentation of per_device_resource functions

6fdf76f

Properly implement on missing ref

ead9947

Breathe doesn't seem to like having the function show up twice in the…

4200358

… group

Go back to warnings as errors

9e81c73

vyasr requested a review from bdice November 1, 2023 00:44

bdice approved these changes Nov 1, 2023

View reviewed changes

ajschmidt8 reviewed Nov 2, 2023

View reviewed changes

ajschmidt8 approved these changes Nov 2, 2023

View reviewed changes

rapids-bot bot merged commit f4afa6a into rapidsai:branch-23.12 Nov 2, 2023
47 checks passed

vyasr mentioned this pull request Nov 2, 2023

[FEA] Disable doxygen documentation publication #1372

Closed

vyasr deleted the feat/unify_docs branch November 2, 2023 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate unified Python/C++ docs #1324

Generate unified Python/C++ docs #1324

vyasr commented Aug 10, 2023 •

edited

Loading

harrism Aug 15, 2023

vyasr Aug 17, 2023

harrism Aug 22, 2023

vyasr Sep 11, 2023

harrism left a comment

vyasr commented Sep 21, 2023

vyasr commented Sep 25, 2023

vyasr commented Oct 27, 2023

harrism left a comment

harrism commented Oct 31, 2023

vyasr commented Oct 31, 2023

harrism commented Oct 31, 2023

vyasr commented Oct 31, 2023

bdice left a comment

bdice Nov 1, 2023

harrism Nov 2, 2023

bdice Nov 1, 2023

bdice Nov 1, 2023

ajschmidt8 left a comment

vyasr commented Nov 2, 2023

		Welcome to the rmm C++ documentation!
		========================================

		Welcome to the rmm Python documentation!
		========================================

		librmm Documentation
		====================

Generate unified Python/C++ docs #1324

Generate unified Python/C++ docs #1324

Conversation

vyasr commented Aug 10, 2023 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harrism left a comment

Choose a reason for hiding this comment

vyasr commented Sep 21, 2023

vyasr commented Sep 25, 2023

vyasr commented Oct 27, 2023

harrism left a comment

Choose a reason for hiding this comment

harrism commented Oct 31, 2023

vyasr commented Oct 31, 2023

harrism commented Oct 31, 2023

vyasr commented Oct 31, 2023

bdice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajschmidt8 left a comment

Choose a reason for hiding this comment

vyasr commented Nov 2, 2023

vyasr commented Aug 10, 2023 •

edited

Loading