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

fix: warn user when calling load config from flow instance #4787

Merged
merged 7 commits into from
May 19, 2022
Merged

Conversation

samsja
Copy link
Contributor

@samsja samsja commented May 16, 2022

fixing #4773

Now when doing

from jina import Flow

Flow().load_config('config.yaml')

you got this warning

UserWarning: Calling load_config from a Flow instance will override all of its initial parameters. You should probably use `Flow.load_config(...)` instead

@samsja samsja changed the title refactor: flow load config is called on class not object fix: warn user when calling load config from flow instance May 16, 2022
@github-actions github-actions bot added size/S area/testing This issue/PR affects testing labels May 16, 2022
@github-actions
Copy link

github-actions bot commented May 16, 2022

Latency summary

Current PR yields:

  • 🐎🐎🐎🐎 index QPS at 1370, delta to last 2 avg.: +24%
  • 🐎🐎🐎🐎 query QPS at 81, delta to last 2 avg.: +39%
  • 😶 avg flow time within 1.6072 seconds, delta to last 2 avg.: -1%
  • 🐢🐢 import jina within 0.4504 seconds, delta to last 2 avg.: -23%

Breakdown

Version Index QPS Query QPS Avg Flow Time (s) Import Time (s)
current 1370 81 1.6072 0.4504
3.4.4 1200 64 1.9339 0.5609
3.4.3 1001 51 1.3132 0.6239

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

@codecov
Copy link

codecov bot commented May 16, 2022

Codecov Report

Merging #4787 (0393f44) into master (072a47a) will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #4787      +/-   ##
==========================================
+ Coverage   88.03%   88.18%   +0.15%     
==========================================
  Files         119      119              
  Lines        8974     9029      +55     
==========================================
+ Hits         7900     7962      +62     
+ Misses       1074     1067       -7     
Flag Coverage Δ
jina 88.18% <100.00%> (+0.21%) ⬆️

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

Impacted Files Coverage Δ
jina/__init__.py 65.88% <100.00%> (ø)
jina/orchestrate/flow/base.py 89.56% <100.00%> (-0.48%) ⬇️
jina/excepts.py 97.05% <0.00%> (-2.95%) ⬇️
jina/serve/runtimes/gateway/http/__init__.py 97.77% <0.00%> (-2.23%) ⬇️
...a/orchestrate/deployments/config/docker_compose.py 99.00% <0.00%> (-1.00%) ⬇️
jina/helper.py 81.75% <0.00%> (+0.02%) ⬆️
jina/serve/runtimes/gateway/request_handling.py 96.07% <0.00%> (+0.03%) ⬆️
jina/hubble/helper.py 88.31% <0.00%> (+0.31%) ⬆️
jina/jaml/__init__.py 94.79% <0.00%> (+0.43%) ⬆️
... and 1 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 01503bc...0393f44. Read the comment docs.

@samsja samsja marked this pull request as draft May 16, 2022 09:55
@github-actions github-actions bot added the area/core This issue/PR affects the core codebase label May 16, 2022
jina/orchestrate/flow/base.py Outdated Show resolved Hide resolved
tests/integration/issues/github_4773/test_load_config.py Outdated Show resolved Hide resolved
tests/integration/issues/github_4773/test_load_config.py Outdated Show resolved Hide resolved
Co-authored-by: Johannes Messner <44071807+JohannesMessner@users.noreply.github.com>
@samsja samsja marked this pull request as ready for review May 16, 2022 12:17
@samsja samsja closed this May 16, 2022
@samsja samsja reopened this May 16, 2022
JoanFM
JoanFM previously approved these changes May 16, 2022
@JoanFM JoanFM merged commit 3fb3b41 into master May 19, 2022
@JoanFM JoanFM deleted the fix-4773 branch May 19, 2022 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

It shoud not be possible to pass argument to the Flow init function when loading from a yaml file
3 participants