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

[Tune][Doc] Restructure API reference #32311

Merged
merged 27 commits into from
Feb 10, 2023

Conversation

justinvyu
Copy link
Contributor

Why are these changes needed?

This PR splits up long API refs into individual pages, one dedicated to each method/class. There are now 3 layers: an index page (ex: doc/tune/api/api.rst), a summary page for each group of APIs (ex: doc/tune/api/trainable.rst), and an individual page for each API (ex: doc/tune/api/docs/tune.Trainable.rst).

This PR is a followup to #31204, which made the same changes for Ray Data docs.

Related issue number

Checks

  • I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
  • I've run scripts/format.sh to lint the changes in this PR.
  • I've included any doc changes needed for https://docs.ray.io/en/master/.
  • I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
  • Testing Strategy
    • Unit tests
    • Release tests
    • This PR is not tested :(

.gitignore Outdated
Comment on lines 122 to 123
/doc/source/*/api/doc/
/doc/source/cluster/running-applications/job-submission/doc/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can these be combined somehow? Special case for the newly added cluster autogenerated files.

Copy link
Contributor

Choose a reason for hiding this comment

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

@c21 should we conform to one structure? Though it does mess with backwards compatibility for this job submission page.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think we should conform to one structure (non-blocking for this PR). Let me take a look how easy the change is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was also a discussion about whether we should do reference/api or api/doc as the standard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also looks like I can use /doc/source/**/doc as wildcard for nested dirs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Talked to @jjyao separately. We can use api/doc here. To rename to reference/api, we plan to have a separate discussion.

Copy link
Contributor

Choose a reason for hiding this comment

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

just keep in mind that api leads to shorter URLs, too. but consistency is more important

@justinvyu justinvyu added release-blocker P0 Issue that blocks the release P0 Issues that should be fixed in short order docs An issue or change related to documentation labels Feb 8, 2023
Copy link
Contributor

@matthewdeng matthewdeng 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 great, most of my comments are about changes we can make in the future 😄

@@ -240,7 +240,7 @@ Trainables can themselves be distributed. If your trainable function / class cre
that also consume CPU / GPU resources, you will want to add more bundles to the :class:`PlacementGroupFactory`
Copy link
Contributor

Choose a reason for hiding this comment

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

It's already mentioned at the top of this file, but most/all of this page should exist in a user guide instead. We don't have to make the change in this PR though!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, I am thinking:

  • Move high level trainable stuff to key concepts trainable section
  • Move checkpointing guides to a new "Tune Checkpointing" user guide (the old one didn't have much to do with checkpointing, and a new one needs to be made)
  • Move the advanced resource allocation section to the tune resources user guide
  • Reuse actors section can probably go to tune lifecycle user guide.

doc/source/tune/api/trainable.rst Outdated Show resolved Hide resolved
doc/source/tune/api/execution.rst Outdated Show resolved Hide resolved
.. autosummary::
:toctree: doc/

flaml.BlendSearch
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing in master but we should fix this import.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this is because doc requirements != Tune requirements? How should we go about fixing this?


Searcher
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: @c21 any suggested practice for separating out the constructor? I saw in Datasets APIs we explicitly do.

Copy link
Contributor

Choose a reason for hiding this comment

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

@matthewdeng - I think it's fine to keep it as it is here. I was following other system, such as Pandas - https://pandas.pydata.org/docs/reference/series.html .

.. autosummary::
:toctree: doc/

Stopper
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we explicitly set this include _call_ as was done previously?

Currently this shows __init__ instead and feels a bit mismatched.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's actually possible to include specific members with the autosummary. Best thing I could do was have it right below.

@@ -282,7 +282,7 @@ parts:
- file: tune/examples/exercises
title: "Exercises"
- file: tune/faq
- file: tune/api_docs/overview.rst
- file: tune/api/api.rst
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking, are you keeping Tune Internals and Tune Client API unchanged intentionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, for Tune Internals, I wasn't sure how to list out all members/methods with their docstrings if I used autosummary. And since that section is not really meant for external users to search for and use as API calls, I decided to leave it fully expanded.

For Tune Client, there was only 1 class, and I think it is fine as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ack, though we should probably move Callback and PlacementGroupFactory out of Tune Internals.

I think it would make sense to move PlacementGroupFactory directly into the Trainable Utilities section, next to with_resources.

Callback is a bit strange... it's a public tune API, which exposes internals about the trials, and is used by air.RunConfig... we should probably revisit this down the line.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved Callback + PGF to better places.

Copy link
Contributor

@c21 c21 left a comment

Choose a reason for hiding this comment

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

Clicked through several docs, and looks great to me, thanks @justinvyu!

@c21
Copy link
Contributor

c21 commented Feb 9, 2023

Just FYI #32307 for Serve API is just merged, so rebasing to master should resvole the conflict here.

@justinvyu
Copy link
Contributor Author

justinvyu commented Feb 10, 2023

@maxpumperla Do you know why these errors are showing up?

/ray/doc/source/tune/api/doc/ray.tune.sklearn.TuneGridSearchCV.rst:48:<autosummary>:1: WARNING: term not in glossary: fit
/ray/doc/source/tune/api/doc/ray.tune.sklearn.TuneSearchCV.rst:48:<autosummary>:1: WARNING: term not in glossary: fit

The links all work fine, and fit is not missing in either of the generated autosummary/autofunctions.

See here and here.

Update: For now, I have changed this section back to what it was before (not showing any inherited members).

Copy link
Contributor

@matthewdeng matthewdeng left a comment

Choose a reason for hiding this comment

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

@richardliaw can you merge?

@richardliaw richardliaw merged commit 613f4b0 into ray-project:master Feb 10, 2023
justinvyu added a commit to justinvyu/ray that referenced this pull request Feb 10, 2023
cadedaniel pushed a commit that referenced this pull request Feb 13, 2023
* [Tune][Doc] Restructure API reference (#32311)

* [Docs] Fix broken Tune links to overview and intergration (#32442)

* Remove can_restore (exists on master but not releases/2.3.0)

Signed-off-by: Justin Yu <[email protected]>

---------

Signed-off-by: Justin Yu <[email protected]>
Co-authored-by: Artur Niederfahrenhorst <[email protected]>
krfricke pushed a commit that referenced this pull request Feb 14, 2023
This PR splits up long API refs in AIR and Train into individual pages, one dedicated to each method/class.

This PR is a followup to #31204 and #32311, which made the same changes for Ray Data/Tune docs.

Signed-off-by: Justin Yu <[email protected]>
justinvyu added a commit to justinvyu/ray that referenced this pull request Feb 14, 2023
This PR splits up long API refs in AIR and Train into individual pages, one dedicated to each method/class.

This PR is a followup to ray-project#31204 and ray-project#32311, which made the same changes for Ray Data/Tune docs.

Signed-off-by: Justin Yu <[email protected]>
justinvyu added a commit that referenced this pull request Feb 14, 2023
This PR splits up long API refs in AIR and Train into individual pages, one dedicated to each method/class.

This PR is a followup to #31204 and #32311, which made the same changes for Ray Data/Tune docs.

Signed-off-by: Justin Yu <[email protected]>
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
edoakes pushed a commit to edoakes/ray that referenced this pull request Mar 22, 2023
This PR splits up long API refs in AIR and Train into individual pages, one dedicated to each method/class.

This PR is a followup to ray-project#31204 and ray-project#32311, which made the same changes for Ray Data/Tune docs.

Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: Edward Oakes <[email protected]>
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
This PR splits up long API refs in AIR and Train into individual pages, one dedicated to each method/class.

This PR is a followup to ray-project#31204 and ray-project#32311, which made the same changes for Ray Data/Tune docs.

Signed-off-by: Justin Yu <[email protected]>
Signed-off-by: elliottower <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs An issue or change related to documentation P0 Issues that should be fixed in short order release-blocker P0 Issue that blocks the release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants