fix: use body for streaming instead of params #6098

NarekA · 2023-10-25T16:43:39Z

Making a new PR that follows conventions to replace #6091 and #6093

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 have the data passed in the request body rather than the params.

Goals:

resolves Issue 6090
check and update documentation. See guide and ask the team.

Also related: #6097

This reverts commit f1f9a88.

NarekA · 2023-10-25T16:57:07Z

jina/serve/runtimes/worker/http_fastapi_app.py

-            endpoint_path,
-            input_doc_model=None,
+        endpoint_path,
+        input_doc_model=None,
    ):
        from fastapi import Request

        @app.api_route(
            path=f'/{endpoint_path.strip("/")}',
            methods=['GET'],


The following code works, but I did not allow get since it doesn't work with gateway deployments.

methods=['GET', 'POST'],

codecov · 2023-10-25T16:59:48Z

Codecov Report

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

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6098      +/-   ##
==========================================
+ Coverage   75.98%   76.89%   +0.91%     
==========================================
  Files         143      145       +2     
  Lines       13974    14014      +40     
==========================================
+ Hits        10618    10776     +158     
+ Misses       3356     3238     -118

Flag	Coverage Δ
jina	`76.89% <76.00%> (+0.91%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
jina/__init__.py	`56.00% <100.00%> (ø)`
jina/clients/base/helper.py	`85.38% <100.00%> (+2.43%)`	⬆️
jina/serve/runtimes/gateway/request_handling.py	`86.75% <100.00%> (+0.64%)`	⬆️
...ve/runtimes/gateway/http_fastapi_app_docarrayv2.py	`0.00% <0.00%> (ø)`
jina/serve/runtimes/worker/http_fastapi_app.py	`80.86% <72.72%> (+11.44%)`	⬆️

... and 9 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JoanFM · 2023-10-25T17:18:10Z

I am not sure if I would prefer to keep the 2 endpoints separate and fix the underlying issue. @alaeddine-13 is the integration with SSE as in documentation possible still?

NarekA · 2023-10-25T18:07:23Z

@JoanFM I'm just taking the existing endpoint and allowing it to accept either parameters or a request body. I'm happy to add a post endpoint as well, but I need your help with the gateway.

NarekA · 2023-10-25T18:24:39Z

Here is a branch on my fork that will reproduce the gateway error

NarekA · 2023-10-25T23:25:11Z

tests/integration/docarray_v2/test_streaming.py

-        stream = client.stream_doc(
+        start_time = time.time()
+        async for doc in client.stream_doc(
            on='/hello',
            inputs=MyDocument(text='hello world', number=i),
            return_type=MyDocument,
-        )
-        start_time = None
-        async for doc in stream:
-            start_time = start_time or time.time()
+        ):
            assert doc.text == f'hello world {i}'
            i += 1
-            delay = time.time() - start_time
+
            # 0.5 seconds between each request + 0.5 seconds tolerance interval
-            assert delay < (0.5 * i), f'Expected delay to be less than {0.5 * i}, got {delay} on iteration {i}'
+            assert time.time() - start_time < (0.5 * i) + 0.5


At the very least, we should merge this and fix the gateway.

alaeddine-13 · 2023-10-26T09:02:06Z

@NarekA why do you want to change the GET endpoint method ?
I thought the agreement was that, if you have basic requirements and would like to use client libraries, you use GET endpoint and if you need customized schemas and long input, you use the POST method.
What is wrong with this approach ?

alaeddine-13 · 2023-10-26T09:25:03Z

@NarekA why do you want to change the GET endpoint method ? I thought the agreement was that, if you have basic requirements and would like to use client libraries, you use GET endpoint and if you need customized schemas and long input, you use the POST method. What is wrong with this approach ?

Actually now I realize you want to allow passing either of body or query parameters.
I'm not sure if that's a good practice when building HTTP services but I think it's a nice idea actually. Yes @JoanFM this should keep compatibility with the documentation section where we use a standard js client.
A test would be great to have though or at least we need to try out the documentation on this PR before merging

NarekA · 2023-10-26T13:49:15Z

@alaeddine-13 The current implementation with the POST method is fine, but it seems to break when using a deployment with a gateway. The stream all comes out at once. I modified this test (and am changing it back) not realizing that I was fixing the very thing it was testing for. The parameterized test passes for all the scenarios except for http+gateway. Using get instead of post seems to fix the gateway and we have only one endpoint so it's less confusing.

JoanFM

Is there or can we have a test where the API is called with the query params?

NarekA · 2023-10-26T14:00:11Z

If we do allow post, I like the idea of just using the same route with 2 methods: methods=['GET', 'POST'],. That way we have consistency between the endpoints, and don't have to maintain 2 routes.

NarekA · 2023-10-26T14:00:47Z

jina/serve/runtimes/gateway/http_fastapi_app_docarrayv2.py

-        async def streaming_get(request: Request):
-            query_params = dict(request.query_params)
+        async def streaming_get(request: Request, body: input_doc_model = None):
+            if not body:


We can and request.method == 'GET' to the condition here.

I think we may not need POST at all then.

That's the current state of this PR, and it fixes everything. Can't tell if tests need to be re-run or if they're actually failing.

you can see a test failing here https://github.com/jina-ai/jina/actions/runs/6647328275/job/18084538375?pr=6098

I fixed that issue, I think the current failures just need re-runs, can you take a look?

Nevermind, found one that's not flakyness

NarekA · 2023-10-26T18:00:53Z

tests/integration/docarray_v2/test_issues.py

+async def test_issue_6090_get_params(streaming_deployment):
+    """Tests if streaming works with pydantic models with complex fields which are not
+    str, int, or float.
+    """
+
+    docs = []
+    url = f"htto://localhost:{streaming_deployment.port}/stream-simple?text=my_input_text"
+    async with aiohttp.ClientSession() as session:
+
+        async with session.get(url) as resp:
+            async for doc, _ in resp.content.iter_chunks():
+                if b"event: end" in doc:
+                    break
+                parsed = doc.decode().split("data:", 1)[-1].strip()
+                parsed = SimpleInput.parse_raw(parsed)
+                docs.append(parsed)
+
+    assert [d.text for d in docs] == [
+        'hello world my_input_text 0',
+        'hello world my_input_text 1',
+        'hello world my_input_text 2',
+        'hello world my_input_text 3',
+    ]


Added this test for passing documents as query params

NarekA · 2023-10-27T01:09:21Z

jina/serve/runtimes/gateway/request_handling.py

-                    async with session.get(target_url) as response:
+                    request_kwargs = {}
+                    if request.body_exists():
+                        payload = await request.json()
+                        if payload:
+                            request_kwargs['json'] = payload
+
+                    async with session.get(
+                        url=target_url, **request_kwargs
+                    ) as response:


Found out where the gateway forwarding is happening.

NarekA · 2023-10-27T03:25:51Z

Only one test job failing, and I can't find a clear stack trace in it so it might be flakiness.

NarekA and others added 18 commits October 20, 2023 14:15

fix: add post endpoint for streaming

94348f6

test: stream complex docs

b645afb

docs: include docstring for issue test

72037b5

Merge branch 'master' into fix-streaming-6091

99cdd7d

Merge branch 'master' into fix-streaming-6091

cc315c9

fix: use random port

8bc5124

fix: remove import

804b665

fix: simplify client code

3ce6e1e

fix: use json field

a5376af

fix: use to_dict for docarray v1

025413f

docs: add docs about http get

60c9b5e

fix: typo in deployment

03cc4c4

fix: don't use body.data

b89c01a

fix: do unpack body

cf2f145

fix: docarray v2 cast model

fd30b0e

fix: change start time delay

f1f9a88

Revert "fix: change start time delay"

3aa6534

This reverts commit f1f9a88.

fix: use get only

57af50a

NarekA mentioned this pull request Oct 25, 2023

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

Closed

Merge remote-tracking branch 'origin/master' into fix-streaming-2-6091

5f27f0a

NarekA commented Oct 25, 2023

View reviewed changes

NarekA added 2 commits October 25, 2023 10:05

fix: delay test

a72a797

fix: fix get and post endpoints

ee77d13

NarekA force-pushed the fix-streaming-2-6091 branch from 10a0e5a to ee77d13 Compare October 25, 2023 17:06

fix: remove post

524aba4

JoanFM requested a review from alaeddine-13 October 25, 2023 17:16

fix: remove endpoint tags

5cfb423

NarekA force-pushed the fix-streaming-2-6091 branch 2 times, most recently from e87a684 to 5cfb423 Compare October 25, 2023 23:22

Merge branch 'master' into fix-streaming-2-6091

3d1b07f

NarekA commented Oct 25, 2023

View reviewed changes

JoanFM requested changes Oct 26, 2023

View reviewed changes

NarekA commented Oct 26, 2023

View reviewed changes

NarekA added 3 commits October 26, 2023 10:49

test: use get with url params

ca3a707

docs: fix docs on streaming endpoints

92405db

test: increase tolerance

2c626bf

NarekA commented Oct 26, 2023

View reviewed changes

NarekA added 6 commits October 26, 2023 13:47

fix: iteration over chunks

38a577b

fix: gateway forwarding

c4afd99

fix: pre-commit changes

c654c14

fix: don't check for docarray_v2

bb90d14

fix: remove type-hint

08bfd6b

fix: update test output

3eff072

NarekA commented Oct 27, 2023

View reviewed changes

NarekA added 2 commits October 26, 2023 18:30

fix: adding payload

46af5fa

fix: use json payload

bf3cbe2

JoanFM approved these changes Oct 27, 2023

View reviewed changes

JoanFM changed the title ~~fix: Use body for streaming instead of params~~ fix: use body for streaming instead of params Oct 27, 2023

JoanFM merged commit ab2cc19 into jina-ai:master Oct 27, 2023
128 of 130 checks passed

JoanFM mentioned this pull request Oct 31, 2023

Release Note #6101

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use body for streaming instead of params #6098

fix: use body for streaming instead of params #6098

NarekA commented Oct 25, 2023

NarekA Oct 25, 2023 •

edited

Loading

codecov bot commented Oct 25, 2023 •

edited

Loading

JoanFM commented Oct 25, 2023

NarekA commented Oct 25, 2023

NarekA commented Oct 25, 2023 •

edited

Loading

NarekA Oct 25, 2023

alaeddine-13 commented Oct 26, 2023

alaeddine-13 commented Oct 26, 2023

NarekA commented Oct 26, 2023

JoanFM left a comment

NarekA commented Oct 26, 2023

NarekA Oct 26, 2023 •

edited

Loading

JoanFM Oct 26, 2023

NarekA Oct 26, 2023

JoanFM Oct 26, 2023

NarekA Oct 26, 2023

NarekA Oct 26, 2023

NarekA Oct 26, 2023

NarekA Oct 27, 2023

NarekA commented Oct 27, 2023

fix: use body for streaming instead of params #6098

fix: use body for streaming instead of params #6098

Conversation

NarekA commented Oct 25, 2023

NarekA Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 25, 2023 • edited Loading

Codecov Report

JoanFM commented Oct 25, 2023

NarekA commented Oct 25, 2023

NarekA commented Oct 25, 2023 • edited Loading

Choose a reason for hiding this comment

alaeddine-13 commented Oct 26, 2023

alaeddine-13 commented Oct 26, 2023

NarekA commented Oct 26, 2023

JoanFM left a comment

Choose a reason for hiding this comment

NarekA commented Oct 26, 2023

NarekA Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NarekA commented Oct 27, 2023

NarekA Oct 25, 2023 •

edited

Loading

codecov bot commented Oct 25, 2023 •

edited

Loading

NarekA commented Oct 25, 2023 •

edited

Loading

NarekA Oct 26, 2023 •

edited

Loading