fix: add and use post endpoint for streaming #6093

NarekA · 2023-10-20T21:15:13Z

Making a new PR that follows conventions to replace #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:

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

codecov · 2023-10-20T21:23:44Z

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

@@            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	Coverage Δ
jina	`76.17% <38.46%> (-0.20%)`	⬇️

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	`82.94% <100.00%> (-2.21%)`	⬇️
jina/serve/runtimes/worker/http_fastapi_app.py	`69.42% <61.53%> (-12.73%)`	⬇️
...ve/runtimes/gateway/http_fastapi_app_docarrayv2.py	`0.00% <0.00%> (ø)`

... and 10 files with indirect coverage changes

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

NarekA · 2023-10-20T23:44:34Z

tests/integration/docarray_v2/test_issues.py

+@pytest.mark.asyncio
+async def test_issue_6090():
+    """Tests if streaming works with pydantic models with complex fields which are not
+    str, int, or float.
+    """
+
+    class NestedFieldSchema(BaseDoc):
+        name: str = "test_name"
+        dict_field: Dict = Field(default_factory=dict)
+
+    class InputWithComplexFields(BaseDoc):
+        text: str = "test"
+        nested_field: NestedFieldSchema = Field(default_factory=NestedFieldSchema)
+        dict_field: Dict = Field(default_factory=dict)
+        bool_field: bool = False
+
+    class MyExecutor(Executor):
+        @requests(on="/stream")
+        async def stream(
+            self, doc: InputWithComplexFields, parameters: Optional[Dict] = None, **kwargs
+        ) -> InputWithComplexFields:
+            for i in range(4):
+                yield InputWithComplexFields(text=f"hello world {doc.text} {i}")
+
+    docs = []
+    protocol = "http"
+    with Deployment(uses=MyExecutor, protocol=protocol, port=11112) as dep:
+        client = Client(port=11112, protocol=protocol, asyncio=True)
+        example_doc = InputWithComplexFields(text="my input text")
+        async for doc in client.stream_doc(
+            on="/stream",
+            inputs=example_doc,
+            input_type=InputWithComplexFields,
+            return_type=InputWithComplexFields,
+        ):
+            docs.append(doc)
+
+    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",
+    ]
+    assert docs[0].nested_field.name == "test_name"


@JoanFM I added this test to make sure complex Docs can be streamed

jina/serve/runtimes/gateway/http_fastapi_app_docarrayv2.py

tests/integration/docarray_v2/test_issues.py

JoanFM

I like how the solution looks, I just have some small comments.

jina/serve/runtimes/worker/http_fastapi_app.py

JoanFM · 2023-10-23T07:45:58Z

jina/clients/base/helper.py

-            req_dict = doc.dict()
-        else:
-            req_dict = doc.to_dict()
+        req_dict = doc.json()


I would not do this change, I think the signature of json may return a str. Why is there a need to change it?

In order to be passed as request_kwargs['data'] rather than request_kwargs['params'], it needs to be serialized as a string.

Was able to keep this by using json instead of data

but where does the need for passing as 'str' come from?

If we were using data, data needs to be a string, but since I swithced to json, I am able to use dict again. Here is the current code:

'json': doc.dict() if docarray_v2 else doc.to_dict(),

NarekA · 2023-10-24T16:27:15Z

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

JoanFM · 2023-10-24T17:22:18Z

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?

docs/concepts/serving/executor/add-endpoints.md

JoanFM · 2023-10-24T19:22:23Z

@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 · 2023-10-24T22:33:09Z

@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)

NarekA · 2023-10-24T23:11:35Z

tests/integration/docarray_v2/test_streaming.py

+        )
+        start_time = None
+        async for doc in stream:
+            start_time = start_time or time.time()


Looks like streaming takes longer to initialize, but has all of the data emitted at once:

Expected delay to be less than 0.5, got 7.867813110351562e-06 on iteration 1 Expected delay to be less than 1.0, got 0.00012993812561035156 on iteration 2 Expected delay to be less than 1.5, got 0.0001659393310546875 on iteration 3 Expected delay to be less than 2.0, got 0.00019884109497070312 on iteration 4 Expected delay to be less than 2.5, got 0.00022912025451660156 on iteration 5 ``

All these times do not include a 2.5 second initialization time.

Looks like the gateway might be breaking streaming.

JoanFM · 2023-10-25T08:06:03Z

Thanks @NarekA for the great contribution

This reverts commit f2085a9.

fix: add post endpoint for streaming

94348f6

NarekA force-pushed the fix-streaming-6091 branch from c817617 to 94348f6 Compare October 20, 2023 21:16

NarekA mentioned this pull request Oct 20, 2023

Use Post for Streaming Endpoints #6091

Closed

1 task

NarekA added 2 commits October 20, 2023 16:37

test: stream complex docs

b645afb

docs: include docstring for issue test

72037b5

NarekA force-pushed the fix-streaming-6091 branch from 359dccb to 72037b5 Compare October 20, 2023 23:38

Merge branch 'master' into fix-streaming-6091

99cdd7d

NarekA commented Oct 20, 2023

View reviewed changes

JoanFM requested changes Oct 21, 2023

View reviewed changes

jina/serve/runtimes/gateway/http_fastapi_app_docarrayv2.py Show resolved Hide resolved

tests/integration/docarray_v2/test_issues.py Outdated Show resolved Hide resolved

JoanFM requested changes Oct 21, 2023

View reviewed changes

jina/serve/runtimes/worker/http_fastapi_app.py Outdated Show resolved Hide resolved

JoanFM requested changes Oct 23, 2023

View reviewed changes

Merge branch 'master' into fix-streaming-6091

cc315c9

NarekA added 3 commits October 24, 2023 09:48

fix: use random port

8bc5124

fix: remove import

804b665

fix: simplify client code

3ce6e1e

NarekA force-pushed the fix-streaming-6091 branch from c80d8af to 3ce6e1e Compare October 24, 2023 16:48

NarekA commented Oct 24, 2023

View reviewed changes

docs/concepts/serving/executor/add-endpoints.md Outdated Show resolved Hide resolved

NarekA added 3 commits October 24, 2023 10:59

fix: use json field

a5376af

fix: use to_dict for docarray v1

025413f

docs: add docs about http get

60c9b5e

NarekA force-pushed the fix-streaming-6091 branch from 2d6b949 to 60c9b5e Compare October 24, 2023 17:59

JoanFM requested changes Oct 24, 2023

View reviewed changes

docs/concepts/serving/executor/add-endpoints.md Outdated Show resolved Hide resolved

fix: typo in deployment

03cc4c4

NarekA requested a review from JoanFM October 24, 2023 18:31

NarekA added 2 commits October 24, 2023 12:52

fix: don't use body.data

b89c01a

fix: do unpack body

cf2f145

fix: docarray v2 cast model

fd30b0e

NarekA commented Oct 24, 2023

View reviewed changes

fix: change start time delay

f1f9a88

NarekA force-pushed the fix-streaming-6091 branch from 9f262a7 to f1f9a88 Compare October 24, 2023 23:16

JoanFM changed the title ~~fix: Add and use post endpoint for streaming~~ fix: add and use post endpoint for streaming Oct 25, 2023

JoanFM merged commit f2085a9 into jina-ai:master Oct 25, 2023
125 of 130 checks passed

JoanFM mentioned this pull request Oct 25, 2023

Release Note #6095

Closed

NarekA added a commit to NarekA/jina that referenced this pull request Oct 25, 2023

Revert "fix: add and use post endpoint for streaming (jina-ai#6093)"

b462c90

This reverts commit f2085a9.

This was referenced Oct 25, 2023

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

Closed

fix: use body for streaming instead of params #6098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add and use post endpoint for streaming #6093

fix: add and use post endpoint for streaming #6093

NarekA commented Oct 20, 2023

codecov bot commented Oct 20, 2023 •

edited

Loading

NarekA Oct 20, 2023

JoanFM left a comment

JoanFM Oct 23, 2023

NarekA Oct 24, 2023

NarekA Oct 24, 2023

JoanFM Oct 24, 2023

NarekA Oct 24, 2023

NarekA commented Oct 24, 2023

JoanFM commented Oct 24, 2023

JoanFM commented Oct 24, 2023

NarekA commented Oct 24, 2023 •

edited

Loading

NarekA Oct 24, 2023

NarekA Oct 24, 2023

NarekA Oct 24, 2023

JoanFM commented Oct 25, 2023

fix: add and use post endpoint for streaming #6093

fix: add and use post endpoint for streaming #6093

Conversation

NarekA commented Oct 20, 2023

codecov bot commented Oct 20, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

JoanFM left a 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 24, 2023

JoanFM commented Oct 24, 2023

JoanFM commented Oct 24, 2023

NarekA commented Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoanFM commented Oct 25, 2023

codecov bot commented Oct 20, 2023 •

edited

Loading

NarekA commented Oct 24, 2023 •

edited

Loading