8000 fix(flow): raise error when using grpc data runtime with shards by RaghavPrabhakar66 · Pull Request #3364 · jina-ai/serve · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(flow): raise error when using grpc data runtime with shards #3364

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Sep 16, 2021
Merged

fix(flow): raise error when using grpc data runtime with shards #3364

merged 9 commits into from
Sep 16, 2021

Conversation

RaghavPrabhakar66
Copy link
Contributor

Fixes Issue: #3197

Expected Behavior: Flow(grpc_data_requests=True).add(shards=2) should raise an error.

Implemented Behavior:

>>> Flow(grpc_data_requests=True).add(shards=2)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/workspaces/jina/jina/flow/builder.py", line 33, in arg_wrapper
    return func(self, *args, **kwargs)
  File "/workspaces/jina/jina/flow/base.py", line 716, in add
    raise NotImplementedError("GRPC data runtime does not support sharding")
NotImplementedError: GRPC data runtime does not support sharding
>>> Flow(grpc_data_requests=True).add(shards=None)
<jina.flow.base.Flow object at 0x7fb19e5c8b80>
>>> Flow(grpc_data_requests=True).add()
<jina.flow.base.Flow object at 0x7fb19e5c8b80>

@RaghavPrabhakar66 RaghavPrabhakar66 requested a review from a team as a code owner September 7, 2021 15:23
jacobowitz
jacobowitz suggested changes Sep 7, 2021
View reviewed changes
Copy link
Contributor
@jacobowitz jacobowitz left a comment

Choose a reason for hiding this comment

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

Thank you very much for your contribution!

Can you maybe also add a unit test, which covers this case? It should go somewhere in tests/unit/flow-construct/ I believe

@AMSAJ0
Copy link
AMSAJ0 commented Sep 7, 2021

Ok

@RaghavPrabhakar66
Copy link
Contributor Author

Can you maybe also add a unit test, which covers this case? It should go somewhere in tests/unit/flow-construct/ I believe

@jacobowitz Should I write test in a new file or add into an existing file? For now I have added it to test_flow.py.

bdr2486
bdr2486 previously approved these changes Sep 7, 2021
View reviewed changes
agarwalrounak
agarwalrounak reviewed Sep 8, 2021
View reviewed changes
jacobowitz
jacobowitz suggested changes Sep 8, 2021
View reviewed changes
Copy link
Contributor
@jacobowitz jacobowitz left a comment

Choose a reason for hiding this comment

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

Thanks for adding the test :)
Mayebe also add anothe one testing the good case? shards=1 should work (even though shards is specified).

jacobowitz
jacobowitz reviewed Sep 8, 2021
View reviewed changes
jacobowitz
jacobowitz reviewed Sep 10, 2021
View reviewed changes
@jacobowitz
Copy link
Contributor

@RaghavPrabhakar66
Looks good now :)
One last thing that we should add is the K8s check. Recently we added support for k8s deployment and is this case grpc/shards is actually a working combination.

You can check it with something like this
args.infrastructure == InfrastructureType.K8S

@RaghavPrabhakar66
Copy link
Contributor Author

@jacobowitz We can add something like and self.args.infrastructure != InfrastructureType.K8S to if-condition. I checked, there are only two Infrastructure Types K8s and Jina, so I think this could work.

jacobowitz
jacobowitz suggested changes Sep 15, 2021
View reviewed changes
Copy link
Contributor
@jacobowitz jacobowitz left a comment

Choose a reason for hiding this comment

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

One minor comment left, lets address it and then we can check if CI runs through.

@jacobowitz
Copy link
Contributor

@RaghavPrabhakar66
thx for the update :) lets get CI to pass now. Atm its complaining about the commit message being too long:

input: fix(flow): update grpc with shard test and grpc with shard check condition ✖ header must not be longer than 72 characters, current length is 74 [header-max-length]

https://github.com/jina-ai/jina/pull/3364/checks?check_run_id=3609792590

The rules are defined here: https://github.com/jina-ai/jina/blob/master/CONTRIBUTING.md

We probably will need multiple iterations until CI passes, I will run it whenever you update the PR

@codecov
Copy link
codecov bot commented Sep 16, 2021 •
edited
Loading

Codecov Report

Merging #3364 (8611626) into master (6a272d9) will increase coverage by 0.63%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3364      +/-   ##
==========================================
+ Coverage   89.68%   90.32%   +0.63%     
==========================================
  Files         152      152              
  Lines       10843    10910      +67     
==========================================
+ Hits         9725     9854     +129     
+ Misses       1118     1056      -62
Flag Coverage Δ
daemon 45.70% <60.00%> (+0.13%) ⬆️
jina 90.31% <100.00%> (+0.63%) ⬆️

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

Impacted Files Coverage Δ
jina/flow/base.py 89.94% <100.00%> (+0.90%) ⬆️
jina/jaml/parsers/executor/legacy.py 89.70% <0.00%> (-2.61%) ⬇️
jina/hubble/hubio.py 88.07% <0.00%> (-0.74%) ⬇️
jina/types/document/__init__.py 96.07% <0.00%> (-0.47%) ⬇️
jina/parsers/__init__.py 97.87% <0.00%> (-0.03%) ⬇️
jina/helper.py 83.33% <0.00%> (ø)
jina/parsers/base.py 100.00% <0.00%> (ø)
jina/parsers/flow.py 100.00% <0.00%> (ø)
jina/hubble/helper.py 90.06% <0.00%> (ø)
jina/proto/jina_pb2.py 100.00% <0.00%> (ø)
... and 20 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 6a272d9...8611626. Read the comment docs.

@jacobowitz jacobowitz self-assigned this Sep 16, 2021
jacobowitz
jacobowitz approved these changes Sep 16, 2021
View reviewed changes
Copy link
Contributor
@jacobowitz jacobowitz left a comment

Choose a reason for hiding this comment

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

all green and change looks good to me! thx a lot for this PR :)

@jacobowitz jacobowitz merged commit c777989 into jina-ai:master Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agarwalrounak agarwalrounak agarwalrounak left review comments

@AMSAJ0 AMSAJ0 AMSAJ0 left review comments

@jacobowitz jacobowitz jacobowitz approved these changes

@bdr2486 bdr2486 bdr2486 left review comments

@JoanFM JoanFM Awaiting requested review from JoanFM

@slettner slettner Awaiting requested review from slettner

Assignees

@jacobowitz jacobowitz

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@RaghavPrabhakar66 @AMSAJ0 @jacobowitz @agarwalrounak @bdr2486
0