Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[train/docs] Restructure Ray Train docs with framework-specific guides #37892
[train/docs] Restructure Ray Train docs with framework-specific guides #37892
Changes from all commits
7a79003
de5582a
7d922c9
b34fe4e
13b4293
5926a7c
e8b78cd
6f1b4a8
a74cc4d
e9b6931
5a567af
32db1b2
5207568
803eb09
177afaf
ad26fff
7dfa8fe
01f5e8f
09237a8
6327fb2
74638bf
030da48
1219b6f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for this file. Can this info be moved to the Distributed PyTorch landing page or PyTorch Quickstart? Or can we prune a lot of it because things like Predictors and Checkpoints are being deemphasized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would advocate to keep this on the top level - this is the only conceptual glue that ties the Ray Train library together. We can remove Predictors when they are deprecated, Checkpoints will still continue exist in some form
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I'm not sure where this is configured, but would be good to have the drop-down caret show in the ToC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like that, too, and have no idea why it doesn't show up. I'll try a few more things...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look and think it's defined here:
ray/doc/source/_static/js/custom.js
Lines 32 to 55 in 672ce55
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! I'll update the list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we group the pages needed for production, together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean - what do you mean with "needed for production"? I don't think we should introduce more groups here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my mental model, there are two distinct jobs to be done that first time users can have:
It may be helpful to delineate what guides/steps are needed to achieve 1, and the next level of complexity would be to achieve 2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there no link anymore to framework specific Checkpoints? If these are deprecated, what's the recommended way to do prediction with models trained with Ray Train?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also wondering this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on @ericl's Slack comment, it sounds like references to framework-specific checkpoints should be removed from the docs. Could we replace "use one of the framework-specific Checkpoint classes." with the current recommendation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's decouple the removal from this PR. We'll update the content when we fully quarantined the framework-specific checkpoint classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but this PR makes the removal right? It's no longer documented with this PR merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's defer the "AIR" removals - "Ray Predictors" make it sound like a first class concept, which it's not. The AIR removal hasn't been fully done, yet - IMO this should be part of that. here we just remove the references