Skip to content
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

fix(stream): streaming enable to work with proxy #4330

Merged
merged 14 commits into from
Dec 22, 2023

Conversation

jianshen92
Copy link
Contributor

What does this PR address?

Relates to #4310

  1. streaming now just stream raw text instead of pickled data container
  2. provides to_sse and from_sse to be used at boundaries (api server and runner) if user wants to stream text in SSE format

Fixes #(issue)

Before submitting:

@jianshen92 jianshen92 requested a review from a team as a code owner December 11, 2023 19:36
@jianshen92 jianshen92 requested review from bojiang, frostming and aarnphm and removed request for a team December 11, 2023 19:36
@jianshen92 jianshen92 changed the title fix(sse): streaming enable to work with proxy fix(stream): streaming enable to work with proxy Dec 14, 2023
@aarnphm
Copy link
Contributor

aarnphm commented Dec 16, 2023

@jianshen92 I think might be worth merging this to 1.2

@jianshen92
Copy link
Contributor Author

@jianshen92 I think might be worth merging this to 1.2

@bojiang should I be merging to 1.2 instead?

@bojiang
Copy link
Member

bojiang commented Dec 18, 2023

‌‌‌@aarnphm This is a bug fix. It's affecting the deployment of openllm under certain circumstances. There's still some time for 1.2 to release. Let's merge to main first?

@bojiang
Copy link
Member

bojiang commented Dec 18, 2023

‌‌‌‌By the way, I think what Aaron meant might not be that we don't offer these utils, but rather that we provide utils which then return SSE objects. @jianshen92
I have the same thought, and I wrote some unit test accordingly. Let me push them here.
You can run them by

pip install pytest pytest-asyncio
pytest tests/unit/test_utils.py

@aarnphm
Copy link
Contributor

aarnphm commented Dec 19, 2023

@bojiang @jianshen92 what is the status of this PR?

@bojiang
Copy link
Member

bojiang commented Dec 20, 2023

@aarnphm Jianshen is still working on the utils.

@jianshen92
Copy link
Contributor Author

jianshen92 commented Dec 21, 2023

Usage:

runner

class Runner(bentoml.Runnable):
    ...
    @bentoml.Runnable.method()
    async def raw_text_streaming():
        yield "this is valid"


    @bentoml.Runnable.method()
    async def sse_streaming():
        yield SSE(data="my data", event="my event", id="a123cbx", retry=123)

api-server

# if we expect raw text, nothing changes. we dont care if they get chunked 
# if we expect a SSE text stream, we can use the `from_chunks` method to reconstruct the chunked data
async for event in SSE.from_chunks(runner_iterator):
    yield event.to_str()
    yield event.data

@bojiang bojiang enabled auto-merge (squash) December 22, 2023 09:23
@bojiang bojiang merged commit cf97460 into bentoml:main Dec 22, 2023
34 of 36 checks passed
aarnphm added a commit that referenced this pull request Dec 28, 2023
* Stream raw text without data container and pickling

* Helper function to and from sse

* Update src/bentoml/_internal/server/runner_app.py

Co-authored-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>

* remove utilities function

* misc

* Update src/bentoml/_internal/server/runner_app.py

Co-authored-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>

* misc

* fix: add unit test for sse utils

* ci: auto fixes from pre-commit.ci

For more information, see https://pre-commit.ci

* Bring back utilities function!

* Catching invalid SSE message

* comments

* Update test cases

* Remove coupling of implementation

* update test

* Using String IO over string concat

---------

Co-authored-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Co-authored-by: bojiang <bojiang_@outlook.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Signed-off-by: Aaron <29749331+aarnphm@users.noreply.github.com>
frostming pushed a commit that referenced this pull request Jan 17, 2024
* Stream raw text without data container and pickling

* Helper function to and from sse

* Update src/bentoml/_internal/server/runner_app.py

Co-authored-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>

* remove utilities function

* misc

* Update src/bentoml/_internal/server/runner_app.py

Co-authored-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>

* misc

* fix: add unit test for sse utils

* ci: auto fixes from pre-commit.ci

For more information, see https://pre-commit.ci

* Bring back utilities function!

* Catching invalid SSE message

* comments

* Update test cases

* Remove coupling of implementation

* update test

* Using String IO over string concat

---------

Co-authored-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Co-authored-by: bojiang <bojiang_@outlook.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Signed-off-by: Aaron <29749331+aarnphm@users.noreply.github.com>
frostming pushed a commit that referenced this pull request Feb 1, 2024
* Stream raw text without data container and pickling

* Helper function to and from sse

* Update src/bentoml/_internal/server/runner_app.py

Co-authored-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>

* remove utilities function

* misc

* Update src/bentoml/_internal/server/runner_app.py

Co-authored-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>

* misc

* fix: add unit test for sse utils

* ci: auto fixes from pre-commit.ci

For more information, see https://pre-commit.ci

* Bring back utilities function!

* Catching invalid SSE message

* comments

* Update test cases

* Remove coupling of implementation

* update test

* Using String IO over string concat

---------

Co-authored-by: Aaron Pham <29749331+aarnphm@users.noreply.github.com>
Co-authored-by: bojiang <bojiang_@outlook.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Signed-off-by: Aaron <29749331+aarnphm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants