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

[AIR][Train][Doc] Restructure API reference #32360

Merged
merged 21 commits into from
Feb 14, 2023

Conversation

justinvyu
Copy link
Contributor

@justinvyu justinvyu commented Feb 9, 2023

Why are these changes needed?

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.

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 :(

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Copy link
Contributor

@maxpumperla maxpumperla left a comment

Choose a reason for hiding this comment

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

great stuff!

@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 9, 2023
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.

Looks good by spot-checking several doc page. cc @matthewdeng for approval.

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
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.

Very nice!

Comment on lines +9 to +10
.. toctree::
:maxdepth: 2
Copy link
Contributor

Choose a reason for hiding this comment

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

Love this structure so much!

Dataset Ingest (Ray Data)<dataset-ingest.rst>
Trainers (Ray Train)<../../train/api/api>
Tuner (Ray Tune)<../../tune/api_docs/execution>
Results (Ray Train + Ray Tune)<../../tune/api_docs/result_grid>
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem to be rendering properly on the base page or table of contents. I think it might be that the result_grid page is missing a title header or something?

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.

This will be fixed by merging in Tune PR.

Comment on lines 75 to 101
Ray Train Built-in Predictors
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. autosummary::
:toctree: doc/

~xgboost.XGBoostPredictor
~lightgbm.LightGBMPredictor
~tensorflow.TensorflowPredictor
~torch.TorchPredictor
~huggingface.HuggingFacePredictor
~sklearn.SklearnPredictor
~rl.RLPredictor

Ray Train Framework-specific Checkpoints
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. autosummary::
:toctree: doc/

~xgboost.XGBoostCheckpoint
~lightgbm.LightGBMCheckpoint
~tensorflow.TensorflowCheckpoint
~torch.TorchCheckpoint
~huggingface.HuggingFaceCheckpoint
~sklearn.SklearnCheckpoint
~rl.RLCheckpoint
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally feel like we should move these to the predictor/checkpoint pages even though they're in the train package...

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 think checkpoints should stay in train, since they are still needed by users of Train only (w/o prediction). I can include another reference in the air checkpoint page for framework specific ckpts, but without toctree.

Predictors I agree should be moved out.

@@ -0,0 +1,18 @@
.. _air-serve-integration:

Model Serving in AIR
Copy link
Contributor

Choose a reason for hiding this comment

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

Add ray.serve.air_integrations.PredictorWrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PredictorWrapper should be on this page already.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhh woops the rendering looked a bit strange because of the alias

image

Copy link
Contributor

Choose a reason for hiding this comment

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

My mistake!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, to be fair this page is kinda a mess. The see also tooltip + autosummary + autofunction is a bit much. But I had to do switch predictor deployment to an autofunction because it's an empty class that was erroring if I put it in autosummary.

AIR Checkpoint (All Libraries)<checkpoint.rst>
Predictors (Ray Data + Ray Train)<predictor.rst>
Model Serving in AIR (Ray Serve)<serve.rst>
External Library Integrations<../../tune/api_docs/integration>
Copy link
Contributor

Choose a reason for hiding this comment

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

Just double checking that we're intentionally keeping these unchanged?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The external libraries one will be updated with the Tune PR. The other ones are updated.

…n/api_restructure

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
…in air

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
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.

cc @richardliaw - can you merge this?

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@justinvyu
Copy link
Contributor Author

@richardliaw Let's wait to merge after tests pass.

@richardliaw
Copy link
Contributor

richardliaw commented Feb 13, 2023 via email

Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
Signed-off-by: Justin Yu <justinvyu@berkeley.edu>
@krfricke krfricke merged commit d87d86f into ray-project:master Feb 14, 2023
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 <justinvyu@berkeley.edu>
@justinvyu justinvyu deleted the train/api_restructure branch February 16, 2023 16:53
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 <justinvyu@berkeley.edu>
Signed-off-by: Edward Oakes <ed.nmi.oakes@gmail.com>
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 <justinvyu@berkeley.edu>
Signed-off-by: elliottower <elliot@elliottower.com>
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.

6 participants