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

Revert "fix: add and use post endpoint for streaming" #6097

Closed NarekA closed 8 months ago

NarekA commented 8 months ago

Reverts jina-ai/jina#6093

@JoanFM, This PR had an issue, the change I made to the streaming delay test was a patch. It looks like streaming does not work when I use post with a gateway, however, I have been able to update the endpoint so get works with either query params or a json body. Let's revert this and I will push my new branch.

codecov[bot] commented 8 months ago

Codecov Report

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

Comparison is base (f2085a9) 75.98% compared to head (b462c90) 74.91%. Report is 3 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #6097 +/- ## ========================================== - Coverage 75.98% 74.91% -1.07% ========================================== Files 143 145 +2 Lines 13974 14007 +33 ========================================== - Hits 10618 10494 -124 - Misses 3356 3513 +157 ``` | [Flag](https://app.codecov.io/gh/jina-ai/jina/pull/6097/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/6097/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jina-ai) | `74.91% <71.42%> (-1.07%)` | :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/6097?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/6097?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/6097?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jina-ai#diff-amluYS9jbGllbnRzL2Jhc2UvaGVscGVyLnB5) | `85.14% <83.33%> (+2.20%)` | :arrow_up: | | [jina/serve/runtimes/worker/http\_fastapi\_app.py](https://app.codecov.io/gh/jina-ai/jina/pull/6097?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jina-ai#diff-amluYS9zZXJ2ZS9ydW50aW1lcy93b3JrZXIvaHR0cF9mYXN0YXBpX2FwcC5weQ==) | `82.14% <80.00%> (+12.72%)` | :arrow_up: | | [...ve/runtimes/gateway/http\_fastapi\_app\_docarrayv2.py](https://app.codecov.io/gh/jina-ai/jina/pull/6097?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 [26 files with indirect coverage changes](https://app.codecov.io/gh/jina-ai/jina/pull/6097/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.

JoanFM commented 8 months ago

Can you open in parallel a PR with your fix to the GET parameter, before we actually revert this?

JoanFM commented 8 months ago

Can you tell me how you reproduce the issue with a Gateway? I may be able to have it working?

NarekA commented 8 months ago

@JoanFM I was able to do this without adding a post endpoint in the following PR. That PR just uses the existing get endpoint but adds an optional body param.

NarekA commented 8 months ago

@JoanFM do you prefer to use the current endpoints with the gateway fixed or this?

JoanFM commented 8 months ago

I think the idea of keeping only the GET endpoint is good.