-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
[Serve][Doc] Direct Ingress #29149
[Serve][Doc] Direct Ingress #29149
Conversation
f467f70
to
db4fc65
Compare
Signed-off-by: Sihan Wang <[email protected]>
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.
Looks great! I made some edits and had a few minor questions.
One structural nit is that if the codegen part is required for both Serve schema and BYO schema, perhaps it shouldn't be a subsection of Serve schema. But this is very minor, it's up to you
Please lmk once you addressed @architkulkarni's comments |
Signed-off-by: Sihan Wang <[email protected]>
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.
One major issue we didn't explain is why would people use Serve's gRPC or bring their own gRPC schema. We should add the following paragraphs:
- What's the benefit of using gRPC: performance, standardization, binary protocol
- gRPC (builtin or BYO) an entrypoint to Serve's model scaling and deployment graph feature. (We should have examples about that here. Can we have the custom gRPC call a deployment graph?)
- What does
is_driver_deployment
mean? (We should say it is experimental)
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.
Left minor nit
|
||
* use Serve's internal gRPC schema to send traffic | ||
* bring your own gRPC schema into your Serve application | ||
|
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.
Or have a link to the gRPC documentation.
Hi @sihanwang41 - could you address the comment? |
Signed-off-by: Sihan Wang <[email protected]>
ping on my suggested edits... |
Signed-off-by: Sihan Wang <[email protected]>
Done! Sorry I miss the comments you left in the second round |
Signed-off-by: Weichen Xu <[email protected]>
Signed-off-by: Sihan Wang [email protected]
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.