8000 fix: read with args in flow.save_config by deepankarm · Pull Request #3256 · jina-ai/serve · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix: read with args in flow.save_config #3256

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 2 commits into from
Aug 24, 2021
Merged

fix: read with args in flow.save_config #3256

merged 2 commits into from
Aug 24, 2021

Conversation

deepankarm
Copy link
Contributor
@deepankarm deepankarm commented Aug 24, 2021 •
edited
Loading
Before After 
from jina import Flow
from jina.jaml import JAML

f = Flow.load_config('''
jtype: Flow
version: 1
with:
  port_expose: 9000
  protocol: http
executors:
  - name: executor1
    port_in: 45678
''')

print(JAML.dump(f))
"""
!Flow
version: '1'
executors:
- name: executor1
  workspace: /home/ubuntu//jina/
  zmq_identity: 7309d5e9-b0e3-41ff-93ba-892e2eb1f786
  port_ctrl: 57077
  port_in: 45678
  port_out: 60949
  socket_in: "ROUTER_BIND"
  socket_out: "ROUTER_BIND"
  port_expose: 9000
  upload_files: []
"""
 
from jina import Flow
from jina.jaml import JAML

f = Flow.load_config('''
jtype: Flow
version: 1
with:
  name: abc
  port_expose: 9000
  protocol: http
  abc: def
executors:
  - name: executor1
    port_in: 45678
''')

print(JAML.dump(f))
"""
!Flow
version: '1'
with:
  name: abc
  port_expose: 9000
  protocol: http
  abc: def
executors:
- name: executor1
  workspace: /home/ubuntu/jina
  zmq_identity: 289fb04c-dd7b-4b34-baae-f51bf84127f1
  port_ctrl: 46333
  port_in: 45678
  port_out: 44367
  socket_in: "ROUTER_BIND"
  socket_out: "ROUTER_BIND"
  port_expose: 12345
  upload_files: []
"""

@deepankarm deepankarm requested a review from a team as a code owner August 24, 2021 11:22
@github-actions github-actions bot added size/S area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing component/jaml labels Aug 24, 2021
@codecov
Copy link
codecov bot commented Aug 24, 2021 •
edited
Loading

Codecov Report

Merging #3256 (b42c889) into master (d8d8070) will increase coverage by 1.18%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3256      +/-   ##
==========================================
+ Coverage   89.11%   90.30%   +1.18%     
==========================================
  Files         148      148              
  Lines       10392    10394       +2     
==========================================
+ Hits         9261     9386     +125     
+ Misses       1131     1008     -123
Flag Coverage Δ
daemon 44.89% <0.00%> (-0.03%) ⬇️
jina 90.29% <100.00%> (+1.35%) ⬆️

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

Impacted Files Coverage Δ
jina/jaml/parsers/flow/v1.py 96.22% <100.00%> (+47.20%) ⬆️
jina/peapods/runtimes/jinad/__init__.py 91.42% <0.00%> (-0.96%) ⬇️
jina/helper.py 80.82% <0.00%> (-0.36%) ⬇️
jina/peapods/zmq/__init__.py 89.04% <0.00%> (+0.69%) ⬆️
jina/peapods/runtimes/zmq/zed.py 93.87% <0.00%> (+1.02%) ⬆️
jina/peapods/runtimes/gateway/http/app.py 92.40% <0.00%> (+1.26%) ⬆️
jina/types/message/__init__.py 87.37% <0.00%> (+1.45%) ⬆️
jina/peapods/runtimes/zmq/asyncio.py 96.36% <0.00%> (+1.81%) ⬆️
jina/jaml/helper.py 85.36% <0.00%> (+2.43%) ⬆️
.../runtimes/request_handlers/data_request_handler.py 92.64% <0.00%> (+2.94%) ⬆️
... and 9 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 d8d8070...b42c889. Read the comment docs.

@deepankarm deepankarm linked an issue Aug 24, 2021 that may be closed by this pull request
flow.save_config doesn't consider args in with block #3257
Closed
@github-actions
Copy link

Latency summary

Current PR yields:

  • 😶 index QPS at 1074, delta to last 2 avg.: -1%
  • 😶 query QPS at 43, delta to last 2 avg.: -4%
  • 😶 dam extend QPS at 44136, delta to last 2 avg.: +4%
  • 😶 avg flow time within 1.7539 seconds, delta to last 2 avg.: +6%
  • 😶 import jina within 0.4018 seconds, delta to last 2 avg.: +0%

Breakdown

Version Index QPS Query QPS DAM Extend QPS Avg Flow Time (s) Import Time (s)
current 1074 43 44136 1.7539 0.4018
2.0.20 1101 43 42393 1.5199 0.4107
2.0.19 1076 46 42445 1.7732 0.3874

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

@deepankarm deepankarm requested a review from hanxiao August 24, 2021 12:13
maximilianwerk
maximilianwerk approved these changes Aug 24, 2021
View reviewed changes
Copy link
Member
@maximilianwerk maximilianwerk left a comment

Choose a reason for hiding this comment

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

I like :) Even though I feel the dict(...) is to implicit.

jina/jaml/parsers/flow/v1.py
Comment on lines 86 to +91
if data._kwargs:
r['with'] = data._kwargs

if data._common_kwargs:
r['with'] = dict(r.get('with', {}), **data._common_kwargs)

Copy link
Member

Choose a reason for hiding this comment

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

I think, something like the following is easier to reason about:

with_dict = {}
if ...:
    with_dict.extend(data._kwargs
...
if with_dict:
    r['with'] = with_dict

@deepankarm deepankarm merged commit 8a67a40 into master Aug 24, 2021
@deepankarm deepankarm deleted the jaml-save-config-flow branch August 24, 2021 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maximilianwerk maximilianwerk maximilianwerk approved these changes

@tadejsv tadejsv Awaiting requested review from tadejsv

@hanxiao hanxiao Awaiting requested review from hanxiao

Assignees
No one assigned
Labels
area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing component/jaml size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

flow.save_config doesn't consider args in with block
2 participants
@deepankarm @maximilianwerk
0