Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: return result from flow and client #1541

Merged
merged 5 commits into from
Dec 26, 2020
Merged

feat: return result from flow and client #1541

merged 5 commits into from
Dec 26, 2020

Conversation

hanxiao
Copy link
Member

@hanxiao hanxiao commented Dec 25, 2020

add return_results (by default = False) in client CLI, so that when turning on, results are returned as a list. Can be used with Flow and AsyncFlow, e.g.

AsyncFlow:

with AsyncFlow(return_results=True).add() as f:
    r = await f.index_ndarray(np.random.random([10, 2]))
    assert isinstance(r, list)
    assert isinstance(r[0], Response)

SyncFlow

with Flow(return_results=True).add() as f:
    r = f.index_ndarray(np.random.random([10, 2]))
    assert isinstance(r, list)
    assert isinstance(r[0], Response)

Design decision:

  • dropping asyncio generator. Complicated, can not unify the Sync & AsyncFlow interface in short time.
  • people who wants to use this function is more likely to fetch all result at once, otherwise, on_done callback is sufficient.
  • complicated usage e.g. fully delegating async generator to users is possible, but sounds overengeering to me atm, can be future work. Out of scope of this PR.

@hanxiao hanxiao requested a review from a team as a code owner December 25, 2020 20:59
@hanxiao hanxiao linked an issue Dec 25, 2020 that may be closed by this pull request
@github-actions
Copy link

This PR closes: #1540

1 similar comment
@github-actions
Copy link

This PR closes: #1540

@jina-bot jina-bot added size/S area/cli This issue/PR affects the command line interface area/core This issue/PR affects the core codebase area/helper This issue/PR affects the helper functionality area/testing This issue/PR affects testing component/client component/flow labels Dec 25, 2020
@@ -151,6 +154,8 @@ async def _get_results(self, input_fn: Callable,
'please double check your input iterator') from rpc_ex
else:
raise BadClient(msg) from rpc_ex
if self.args.return_results:
return result
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would it make sense to return a generator to make it more general?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tried. too complicated., async generator is depreciated in python asyncio

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, if async generator is desired, why not directly use on_done?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see design decision in PR body

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is true. Just a comment that felt more natural, but no big deal

@github-actions
Copy link

github-actions bot commented Dec 25, 2020

Latency summary

Current PR yields:

  • 😶 index QPS at 1858, delta to last 3 avg.: +0%
  • 😶 query QPS at 32, delta to last 3 avg.: -4%

Breakdown

Version Index QPS Query QPS
current 1858 32
0.8.13 1851 32
0.8.12 1850 32
0.8.11 1852 35

Backed by latency-tracking. Further commits will update this comment.

@jina-bot jina-bot added size/M area/network This issue/PR affects network functionality component/proto component/type and removed size/S labels Dec 25, 2020
@jina-bot jina-bot added the area/entrypoint This issue/PR affects the entrypoint codebase label Dec 25, 2020
@codecov
Copy link

codecov bot commented Dec 26, 2020

Codecov Report

Merging #1541 (4a481c0) into master (822d075) will increase coverage by 0.09%.
The diff coverage is 60.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1541      +/-   ##
==========================================
+ Coverage   84.42%   84.51%   +0.09%     
==========================================
  Files         108      108              
  Lines        6305     6368      +63     
==========================================
+ Hits         5323     5382      +59     
- Misses        982      986       +4     
Impacted Files Coverage Δ
jina/proto/serializer.py 100.00% <ø> (ø)
jina/flow/asyncio.py 68.29% <12.50%> (ø)
jina/clients/asyncio.py 75.00% <33.33%> (ø)
jina/clients/base.py 80.00% <100.00%> (+1.21%) ⬆️
jina/clients/helper.py 79.03% <100.00%> (+0.34%) ⬆️
jina/parser.py 86.44% <100.00%> (+0.03%) ⬆️
jina/logging/sse.py 91.42% <0.00%> (-3.89%) ⬇️
jina/logging/profile.py 68.33% <0.00%> (-0.58%) ⬇️
jina/flow/base.py 88.52% <0.00%> (-0.43%) ⬇️
jina/executors/decorators.py 90.75% <0.00%> (-0.16%) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6e0d1d1...4a481c0. Read the comment docs.

@hanxiao hanxiao merged commit 3f5a588 into master Dec 26, 2020
@hanxiao hanxiao deleted the feat-1540 branch December 26, 2020 08:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli This issue/PR affects the command line interface area/core This issue/PR affects the core codebase area/entrypoint This issue/PR affects the entrypoint codebase area/helper This issue/PR affects the helper functionality area/network This issue/PR affects network functionality area/testing This issue/PR affects testing component/client component/flow component/proto component/type size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Return results for asyncFlow.search
3 participants