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
Enable streaming audio in python client #5248
Conversation
🪼 branch checks and previews
Install Gradio from this PR pip install https://gradio-builds.s3.amazonaws.com/387da3b5846278da4ddbb56e0f2b669fa1925129/gradio-3.40.1-py3-none-any.whl Install Gradio Python Client from this PR pip install "gradio-client @ git+https://github.com/gradio-app/gradio@387da3b5846278da4ddbb56e0f2b669fa1925129#subdirectory=client/python" |
🦄 change detectedThis Pull Request includes changes to the following packages.
With the following changelog entry.
|
7c3ce18
to
ebfbfae
Compare
"is_stream": True, | ||
} | ||
if data[i]: | ||
data[i]["is_file"] = False |
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.
Preserve the orig_name
so that clients have an easier time downloading the byte stream
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.
But isn't ["name"] assigned to the same value as before in the next line?
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.
Yea we're using "name" to mean the url from which to download the stream so I'm keeping that the same. Just preserving orig_name
(if it's there) so that we can give a meaningful name when we go to download the byte stream to a file.
} | ||
if data[i]: | ||
data[i]["is_file"] = False | ||
data[i]["name"] = f"{session_hash}/{run}/{output_id}" |
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.
@aliabid94 Thoughts on including the stream
endpoint name in the route so that it doesn't have to be hardcoded in the client?
self.stream = self._setup_stream( | ||
root_url + "stream/" + x["name"], hf_token=hf_token | ||
) | ||
chunk = next(self.stream) |
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.
Sorry can you explain why these two lines are repeated from above?
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.
It was to determine when the stream changed but there's a better/clearer way. Will push up now!
for data in r.iter_bytes(): | ||
arr += data | ||
yield data | ||
yield arr |
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.
Am I understanding correctly that what's happening is that we are iterating chunk by chunk and then finally yielding the full array so that .predict()
returns the full data array? 2 questions:
(1) How come outputs() doesn't include this final full data array?
(2) Will r.iter_bytes()
work if the streaming response returns binary data (as in #5238) instead of file data?
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.
(1) outputs only includes the values returned from the server during the process_generating
event
(2) yea the stream
enpoint only serves bytes so it should work when #5238 is merged
Really nice @freddyaboulton! Just a few questions to understand the implementation -- will test more thoroughly tomorrow. |
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.
Very nice @freddyaboulton! LGTM
I'll go ahead and merge this in so that we can consolidate things before the release |
Description
Update the FileSerializable so that it can handle streaming responses introduced in #5077
🎯 PRs Should Target Issues
Before your create a PR, please check to see if there is an existing issue for this change. If not, please create an issue before you create this PR, unless the fix is very small.
Not adhering to this guideline will result in the PR being closed.
Tests
PRs will only be merged if tests pass on CI. To run the tests locally, please set up your Gradio environment locally and run the tests:
bash scripts/run_all_tests.sh
You may need to run the linters:
bash scripts/format_backend.sh
andbash scripts/format_frontend.sh