feat: add option to return in order in client and streamer #5404

JoanFM · 2022-11-17T11:13:48Z

Goals:
Try returning in the order of requests from Streamer

codecov · 2022-11-17T11:22:22Z

Codecov Report

Merging #5404 (d9eb599) into master (38c86a1) will decrease coverage by 1.29%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5404      +/-   ##
==========================================
- Coverage   86.99%   85.69%   -1.30%     
==========================================
  Files         101      101              
  Lines        6611     6607       -4     
==========================================
- Hits         5751     5662      -89     
- Misses        860      945      +85

Flag	Coverage Δ
jina	`85.69% <100.00%> (-1.30%)`	⬇️

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

Impacted Files	Coverage Δ
jina/clients/base/helper.py	`83.06% <ø> (ø)`
jina/clients/mixin.py	`97.41% <ø> (ø)`
jina/serve/runtimes/gateway/http/app.py	`97.85% <ø> (-0.02%)`	⬇️
jina/clients/base/grpc.py	`90.66% <100.00%> (+0.52%)`	⬆️
jina/clients/base/http.py	`95.83% <100.00%> (ø)`
jina/clients/base/websocket.py	`90.19% <100.00%> (ø)`
jina/serve/runtimes/gateway/grpc/gateway.py	`97.26% <100.00%> (ø)`
jina/serve/streamer.py	`95.74% <100.00%> (-0.18%)`	⬇️
jina/jaml/parsers/flow/v1.py	`76.11% <0.00%> (-23.89%)`	⬇️
jina/orchestrate/flow/base.py	`84.12% <0.00%> (-6.65%)`	⬇️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Signed-off-by: Joan Fontanals Martinez <joan.martinez@jina.ai>

docs/fundamentals/client/client.md

Co-authored-by: Alex Cureton-Griffiths <alexcg1@users.noreply.github.com>

alexcg1

LGTM 👍

docs/fundamentals/client/client.md

alaeddine-13 · 2022-11-18T07:51:51Z

jina/serve/stream/__init__.py

+                    responses_ids[response_request_id] = len(responses_list)
+                    responses_list.append(response)


I don't see a need for responses_list here. I think we can directly put the response in the responses_ids map (actually let'ts map from ids to response objects and rename the variable)

alaeddine-13 · 2022-11-18T07:55:35Z

jina/serve/stream/__init__.py

+                        response_index = responses_ids[next_request_id]
+                        del responses_ids[next_request_id]
+                        request_ids.pop(0)
+                        yield responses_list[response_index]


assuming we no more need responses_list, we can change the code like so:

Suggested change

response_index = responses_ids[next_request_id]

del responses_ids[next_request_id]

request_ids.pop(0)

yield responses_list[response_index]

request_ids.pop(0)

yield responses_ids.pop(next_request_id)

alaeddine-13 · 2022-11-18T07:56:28Z

jina/serve/stream/__init__.py

+                stop_yielding = False
+                while not stop_yielding and len(request_ids) > 0:
+                    next_request_id = request_ids[0]
+                    if next_request_id in responses_ids:


what happens if the next_request id is not received ? we don't yield anymore ?

alaeddine-13 · 2022-11-18T07:56:50Z

tests/integration/test_return_order/test_return_order.py

+
+@pytest.mark.parametrize('protocol', ['grpc'])
+def test_return_order_in_client(protocol):
+    class ExecutorRandomSleepExecutor(Executor):


Suggested change

class ExecutorRandomSleepExecutor(Executor):

class RandomSleepExecutor(Executor):

alaeddine-13 · 2022-11-18T07:56:59Z

tests/integration/test_return_order/test_return_order.py

+            rand_sleep = random.uniform(0.1, 1.3)
+            time.sleep(rand_sleep)
+
+    f = Flow(protocol=protocol).add(uses=ExecutorRandomSleepExecutor, replicas=2)


Suggested change

f = Flow(protocol=protocol).add(uses=ExecutorRandomSleepExecutor, replicas=2)

f = Flow(protocol=protocol).add(uses=RandomSleepExecutor, replicas=2)

JohannesMessner · 2022-11-18T08:35:48Z

jina/serve/streamer.py

+def _create_topology_graph(
+        graph_description,
+    graph_conditions,
+    deployments_metadata,
+    deployments_no_reduce,
+    timeout_send,
+    retries,
+):
+    # check if it should be in K8s, maybe ConnectionPoolFactory to be created
+    return TopologyGraph(
+        graph_representation=graph_description,
+        graph_conditions=graph_conditions,
+        deployments_metadata=deployments_metadata,
+        deployments_no_reduce=deployments_no_reduce,
+        timeout_send=timeout_send,
+        retries=retries,
+    )
+
+


I don't understand the purpose of this, it seems like it is the same as just calling the constructor directly?

JohannesMessner · 2022-11-18T08:39:35Z

jina/serve/stream/__init__.py

        :yield: responses
        """
        result_queue = asyncio.Queue()
+        request_ids = []
+        responses_list = []
+        responses_ids = {}  # map from id to index in responses_list


I would rename to response_id_to_index or something like that

JohannesMessner · 2022-11-18T08:45:03Z

docs/fundamentals/client/client.md

@@ -760,6 +760,34 @@ None

 ````

+A Client connects to a Flow that processes Documents in an asynchronous and very distributed way. This means that the order of the Flow processing the requests may not be the same order as the Client sending the requests.


Is there a way to pass this parameter from a 3rd party client?

No, this is gonna be ready in the streamer for the CustomGateway. We can add it later I would say

Mostly external clients do not have this streaming capability so easily implemented, so I think is not so so important

Co-authored-by: AlaeddineAbdessalem <alaeddine-13@live.fr>

alexcg1

LGTM 👍

github-actions · 2022-11-18T09:42:11Z

📝 Docs are deployed on https://feat_return_order--jina-docs.netlify.app 🎉

github-actions bot added size/S area/core This issue/PR affects the core codebase labels Nov 17, 2022

JoanFM force-pushed the feat_return_order branch 3 times, most recently from 905fcee to fed6f31 Compare November 17, 2022 11:16

feat: add option to return in order in streamer

fe91c49

Signed-off-by: Joan Fontanals Martinez <joan.martinez@jina.ai>

JoanFM force-pushed the feat_return_order branch from fed6f31 to fe91c49 Compare November 17, 2022 12:01

JoanFM linked an issue Nov 17, 2022 that may be closed by this pull request

What is the order of multi-replicas flow's result? #5399

Closed

Merge branch 'master' into feat_return_order

07c34b7

github-actions bot added size/M component/client labels Nov 17, 2022

test: add test for return order

94e4673

github-actions bot added area/testing This issue/PR affects testing component/type labels Nov 17, 2022

test: add test in unit

af4eb8c

Signed-off-by: Joan Fontanals Martinez <joan.martinez@jina.ai>

JoanFM force-pushed the feat_return_order branch from f2628b6 to af4eb8c Compare November 17, 2022 13:38

feat: allow return_order for grpc client

6ffdc61

JoanFM requested a review from alaeddine-13 November 17, 2022 14:11

JoanFM marked this pull request as ready for review November 17, 2022 14:11

github-actions bot added the area/docs This issue/PR affects the docs label Nov 17, 2022

docs: add documentation about results_in_order

88b502f

Signed-off-by: Joan Fontanals Martinez <joan.martinez@jina.ai>

JoanFM force-pushed the feat_return_order branch from d8eebaa to 88b502f Compare November 17, 2022 14:22

JoanFM requested review from JohannesMessner and alexcg1 November 17, 2022 15:45

JoanFM changed the title ~~feat: add option to return in order in streamer~~ feat: add option to return in order in client and streamer Nov 17, 2022

alexcg1 requested changes Nov 17, 2022

View reviewed changes

docs/fundamentals/client/client.md Outdated Show resolved Hide resolved

docs/fundamentals/client/client.md Outdated Show resolved Hide resolved

docs: apply suggestions from code review

c91ebf3

Co-authored-by: Alex Cureton-Griffiths <alexcg1@users.noreply.github.com>

JoanFM requested a review from alexcg1 November 17, 2022 16:40

alexcg1 previously approved these changes Nov 17, 2022

View reviewed changes

alaeddine-13 reviewed Nov 18, 2022

View reviewed changes

JohannesMessner reviewed Nov 18, 2022

View reviewed changes

refactor: implement order in much nicer way

c697fc7

JoanFM dismissed alexcg1’s stale review via c697fc7 November 18, 2022 09:27

github-actions bot removed the component/type label Nov 18, 2022

JoanFM and others added 2 commits November 18, 2022 10:28

docs: apply suggestions from code review

3331a6f

Co-authored-by: AlaeddineAbdessalem <alaeddine-13@live.fr>

refactor: avoid useless private func

d9eb599

JoanFM requested review from JohannesMessner and alaeddine-13 November 18, 2022 09:33

alexcg1 approved these changes Nov 18, 2022

View reviewed changes

JoanFM merged commit d3feb66 into master Nov 18, 2022

JoanFM deleted the feat_return_order branch November 18, 2022 10:10

alexcg1 mentioned this pull request Nov 24, 2022

chore: draft release note 3.12.0 #5441

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add option to return in order in client and streamer #5404

feat: add option to return in order in client and streamer #5404

JoanFM commented Nov 17, 2022

codecov bot commented Nov 17, 2022 •

edited

alexcg1 left a comment

alaeddine-13 Nov 18, 2022

alaeddine-13 Nov 18, 2022

alaeddine-13 Nov 18, 2022

alaeddine-13 Nov 18, 2022

alaeddine-13 Nov 18, 2022

JohannesMessner Nov 18, 2022

JohannesMessner Nov 18, 2022

JohannesMessner Nov 18, 2022

JoanFM Nov 18, 2022

JoanFM Nov 18, 2022

alexcg1 left a comment

github-actions bot commented Nov 18, 2022

		responses_ids[response_request_id] = len(responses_list)
		responses_list.append(response)

	class ExecutorRandomSleepExecutor(Executor):
	class RandomSleepExecutor(Executor):

	f = Flow(protocol=protocol).add(uses=ExecutorRandomSleepExecutor, replicas=2)
	f = Flow(protocol=protocol).add(uses=RandomSleepExecutor, replicas=2)

		@@ -760,6 +760,34 @@ None

		````

		A Client connects to a Flow that processes Documents in an asynchronous and very distributed way. This means that the order of the Flow processing the requests may not be the same order as the Client sending the requests.

feat: add option to return in order in client and streamer #5404

feat: add option to return in order in client and streamer #5404

Conversation

JoanFM commented Nov 17, 2022

codecov bot commented Nov 17, 2022 • edited

Codecov Report

alexcg1 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

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

alexcg1 left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 18, 2022

codecov bot commented Nov 17, 2022 •

edited