-
Notifications
You must be signed in to change notification settings - Fork 6k
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 new guides for developing and deploying an ML application in S… #36700
Conversation
|
||
These are all the properties available on a Ray Serve deployment. This is also available in the [API reference](../serve/api/doc/ray.serve.deployment_decorator.rst) | ||
|
||
The following can be configured through either the serve config file or on the `@serve.deployment` decorator: |
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.
A few updates should be made to this content:
- Show a Python code sample and an equivalent config file.
- Show the usage of
.options()
. - Explain the precedence (config > what's in the code).
I think we should also consolidate the content in the "app builder" page here to make it clear how to compose them. WDYT?
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.
Agreed I'll add 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.
Added the 3 (actually moved them over from the serve config page because I think they're more relevant here). Also updated the serve config page. For the app builder, do you propose merging app builder content entirely into this page?
Some comments on the "configuring deployments" page. I'm OK with those changes being a follow-up, but I think they'll significantly reduce confusion. |
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.
Mostly copy edits. There are a few suggestions for clarification mixed in. Great work!!
Some content suggestions:
|
Addressed all the comments and updated the examples in the Serve config files page to use the multi-app schema |
c195441
to
76ddc36
Compare
f5bfb46
to
5b387cc
Compare
Updated with new feedback |
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.
LGTM, great improvements, thanks
ce6d98b
to
0b7511d
Compare
…erve and configuring serve deployments. The change also includes refactoring information in some existing guides. Signed-off-by: Akshay Malik <akshay@anyscale.com>
0b7511d
to
2f98c3a
Compare
ray-project#36700) The change also includes refactoring information in some existing guides. Signed-off-by: e428265 <arvind.chandramouli@lmco.com>
…erve and configuring serve deployments.
The change also includes refactoring information in some existing guides.
Why are these changes needed?
Related issue number
Checks
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.