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
fix: conditional args #1682
fix: conditional args #1682
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be completed with a robust chanhe also on executor/indexer size. I remember the logic there was robust to this condition.
Codecov Report
@@ Coverage Diff @@
## master #1682 +/- ##
==========================================
- Coverage 84.42% 84.17% -0.25%
==========================================
Files 132 132
Lines 6831 6831
==========================================
- Hits 5767 5750 -17
- Misses 1064 1081 +17
Continue to review full report at Codecov.
|
Latency summaryCurrent PR yields:
Breakdown
Backed by latency-tracking. Further commits will update this comment. |
@@ -227,6 +227,8 @@ def _set_conditional_args(args): | |||
args.runtime_cls = 'RESTRuntime' | |||
else: | |||
args.runtime_cls = 'GRPCRuntime' | |||
if 'parallel' in args and args.parallel == 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parallel is now guaranteed to be in args
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and it would be good to add a warning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parallel is now guaranteed to be in args
not in case the gateway is added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, is a smell but good point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and it would be good to add a warning
Thanks, good idea!
However, this is actually expected behaviour:
The documentation for --separated-workspace
in mixin_zed_runtime_parser
says: the data and config files are separated for each pea in this pod, only effective when BasePod\'s parallel > 1
.
In addition there is no logging used in jina/peapods/pods/__init__.py
for all similar cases.
In case we have a separated workspace and set shards to 1, search does not work.
We get rid of this case by forcing to have
separated_workspace=False
ifparallel==1