jina-ai / jina

☁️ Build multimodal AI applications with cloud-native stack
https://docs.jina.ai
Apache License 2.0
20.56k stars 2.21k forks source link

fix: loadbalance stream based on response #6122

Closed NarekA closed 7 months ago

NarekA commented 7 months ago

The current load balancer assumes all GET requests are streaming and all POST requests are not. This may not be true for user-added fast-api endpoints and in the past we have talked about using POST for streaming. (One of the benefits of this is the Swagger UI better documents the payload for POST endpoints).

It makes more sense to use the response content-type to determine when to stream.

Goals:

codecov[bot] commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (1c2a7c2) 73.86% compared to head (bc91179) 75.18%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #6122 +/- ## ========================================== + Coverage 73.86% 75.18% +1.32% ========================================== Files 152 152 Lines 14069 14062 -7 ========================================== + Hits 10392 10573 +181 + Misses 3677 3489 -188 ``` | [Flag](https://app.codecov.io/gh/jina-ai/jina/pull/6122/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jina-ai) | Coverage Δ | | |---|---|---| | [jina](https://app.codecov.io/gh/jina-ai/jina/pull/6122/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jina-ai) | `75.18% <100.00%> (+1.32%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jina-ai#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JoanFM commented 7 months ago

Hey @NarekA,

Thanks for the contribution, can you check the failed tests and add a test showing what u are intending to fix?

NarekA commented 7 months ago

@JoanFM I think the issue came about because I was using request.content_type instead of response.content_type. I think the latest commit should fix it.

JoanFM commented 7 months ago

Most of the errors on tests will be fixed by #6124

NarekA commented 7 months ago

@JoanFM I'm finding these tests to be prohibitively difficult to debug. Any idea what is going wrong? I need a better way of replicating them locally.

JoanFM commented 7 months ago

Thanks for this great contribution!