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: add and use post endpoint for streaming #6093

Closed NarekA closed 8 months ago

NarekA commented 8 months ago

Making a new PR that follows conventions to replace https://github.com/jina-ai/jina/pull/6091

Http streaming breaks when the input doc schema has fields which are not str, int, or float. This includes dicts, bools, and nested objects (See Issue 6090). In addition it caps the input size to 2000 characters (Much less when you factor in URL encoding)

This PR changes the get endpoints to post endpoints. Not sure if we want to keep the get endpoints for backwards compatibility.

Goals:

codecov[bot] commented 8 months ago

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (2c3d7d3) 76.36% compared to head (f1f9a88) 76.17%. Report is 3 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #6093 +/- ## ========================================== - Coverage 76.36% 76.17% -0.20% ========================================== Files 145 145 Lines 14007 14020 +13 ========================================== - Hits 10697 10680 -17 - Misses 3310 3340 +30 ``` | [Flag](https://app.codecov.io/gh/jina-ai/jina/pull/6093/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/6093/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jina-ai) | `76.17% <38.46%> (-0.20%)` | :arrow_down: | 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. | [Files](https://app.codecov.io/gh/jina-ai/jina/pull/6093?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jina-ai) | Coverage Δ | | |---|---|---| | [jina/\_\_init\_\_.py](https://app.codecov.io/gh/jina-ai/jina/pull/6093?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jina-ai#diff-amluYS9fX2luaXRfXy5weQ==) | `56.00% <100.00%> (ø)` | | | [jina/clients/base/helper.py](https://app.codecov.io/gh/jina-ai/jina/pull/6093?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jina-ai#diff-amluYS9jbGllbnRzL2Jhc2UvaGVscGVyLnB5) | `82.94% <100.00%> (-2.21%)` | :arrow_down: | | [jina/serve/runtimes/worker/http\_fastapi\_app.py](https://app.codecov.io/gh/jina-ai/jina/pull/6093?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jina-ai#diff-amluYS9zZXJ2ZS9ydW50aW1lcy93b3JrZXIvaHR0cF9mYXN0YXBpX2FwcC5weQ==) | `69.42% <61.53%> (-12.73%)` | :arrow_down: | | [...ve/runtimes/gateway/http\_fastapi\_app\_docarrayv2.py](https://app.codecov.io/gh/jina-ai/jina/pull/6093?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jina-ai#diff-amluYS9zZXJ2ZS9ydW50aW1lcy9nYXRld2F5L2h0dHBfZmFzdGFwaV9hcHBfZG9jYXJyYXl2Mi5weQ==) | `0.00% <0.00%> (ø)` | | ... and [10 files with indirect coverage changes](https://app.codecov.io/gh/jina-ai/jina/pull/6093/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jina-ai)

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

NarekA commented 8 months ago

@JoanFM I addressed the your comments. Please let me know If there's anything else I can do.

JoanFM commented 8 months ago

Hey @NarekA ,

Thanks for the contrubution, could yiu add a note in the documentation https://docs.jina.ai/concepts/serving/executor/add-endpoints/#streaming-endpoints making explicit note that SSE can only be used with flat schemas?

JoanFM commented 8 months ago

@JoanFM I addressed the your comments. Please let me know If there's anything else I can do.

It looks good to me, but there seems to be a failing test

NarekA commented 8 months ago

@JoanFM I am trying to fix the tests, but I'm having issues recreating CI tests locally, or even seeing what the failure is in CI. (Will go back and read the docs to see what I missed)

JoanFM commented 8 months ago

Thanks @NarekA for the great contribution