-
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] Integrate and Document Bring-Your-Own Gradio Applications #26403
Conversation
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.
Awesome work on the documentation!
python/ray/serve/gradio.py
Outdated
|
||
class GradioIngress: | ||
def __init__(self, io: gr.Blocks): | ||
self.app = gr.routes.App.create_app(io) |
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 this a public API that's guaranteed by gradio to remain stable?
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.
Not that I know of. Should we sync with Gradio team on 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.
Yes, definitely. We need to make sure any APIs we use are public & stable otherwise our integration could break at any point without warning.
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.
Abubakar confirmed that this API will remain stable, and they will add tests around gr.routes.App.create_app()
to make sure the API signature doesn't change!
def fanout(self, text): | ||
[result1, result2] = ray.get([self._d1.remote(text), self._d2.remote(text)]) | ||
return f"{result1}\n------------\n{result2}" |
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: use async def
as best practice :)
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.
Unfortunately Gradio doesn't support async functions in gr.Interface yet :(
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.
Great work so far on this integration! I've added some comments.
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.
Nice updates! I added some comments for the documentation.
- ./ci/ci.sh build | ||
- bazel test --config=ci $(./ci/run/bazel_export_options) | ||
--test_tag_filters=team:serve | ||
python/ray/serve/test_gradio |
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.
you should also filter out this test from regular test suites both in windows (ci.sh) and here.
doc/source/serve/tutorials/index.md
Outdated
@@ -16,6 +16,7 @@ batch | |||
web-server-integration | |||
rllib | |||
gradio |
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.
remove this page plz
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.
Got it, I'll remove the actual file as well.
Signed-off-by: Cindy Zhang <[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.
Nice work so far! I had a small nit about the section titling, but other that that, this change looks pretty good.
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
Signed-off-by: Cindy Zhang <[email protected]>
…y-project#26403) Integration between Ray Serve and Gradio. Users of Gradio can wrap their Gradio app in a Serve deployment by using `GradioIngress`, and scale it up through more replicas or more CPU/GPU resources. Signed-off-by: Cindy Zhang <[email protected]>
…Applications (#26403)" (#27587)" This reverts commit 64c550a. Signed-off-by: Cindy Zhang <[email protected]>
…y-project#26403) Integration between Ray Serve and Gradio. Users of Gradio can wrap their Gradio app in a Serve deployment by using `GradioIngress`, and scale it up through more replicas or more CPU/GPU resources. Signed-off-by: Stefan van der Kleij <[email protected]>
…ions (ray-project#26403)" (ray-project#27587) This reverts commit 8a9d994. Signed-off-by: Stefan van der Kleij <[email protected]>
Why are these changes needed?
Integration between Ray Serve and Gradio. Users of Gradio can wrap their Gradio app in a Serve deployment by using
GradioIngress
, and scale it up through more replicas or more CPU/GPU resources.Related issue number
Checks
scripts/format.sh
to lint the changes in this PR.