8000 refactor: avoid import drivers from types by JoanFM · Pull Request #1759 · jina-ai/serve · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

refactor: avoid import drivers from types #1759

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 8 commits into from
Jan 26, 2021
Merged

refactor: avoid import drivers from types #1759

merged 8 commits into from
Jan 26, 2021

Conversation

JoanFM
Copy link
Contributor
@JoanFM JoanFM commented Jan 22, 2021

No description provided.

@JoanFM JoanFM requested a review from a team as a code owner January 22, 2021 12:54
@JoanFM JoanFM marked this pull request as draft January 22, 2021 12:54
@JoanFM JoanFM linked an issue Jan 22, 2021 that may be closed by this pull request
QueryLang instantiation with Driver instance #1357
Closed
@jina-bot jina-bot added area/core This issue/PR affects the core codebase component/client component/type labels Jan 22, 2021
@codecov
Copy link
codecov bot commented Jan 22, 2021 •
edited
Loading

Codecov Report

Merging #1759 (74f8c9a) into master (e29734b) will increase coverage by 0.37%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1759      +/-   ##
==========================================
+ Coverage   85.32%   85.69%   +0.37%     
==========================================
  Files         135      135              
  Lines        6903     6921      +18     
==========================================
+ Hits         5890     5931      +41     
+ Misses       1013      990      -23
Impacted Files Coverage Δ
jina/clients/request.py 91.42% <100.00%> (-0.24%) ⬇️
jina/drivers/querylang/select.py 89.65% <100.00%> (+10.34%) ⬆️
jina/types/sets/querylang.py 96.61% <100.00%> (-0.06%) ⬇️
jina/docker/hubio.py 69.79% <0.00%> (-0.80%) ⬇️
jina/peapods/runtimes/zmq/zed.py 89.85% <0.00%> (-0.08%) ⬇️
jina/jaml/helper.py 92.30% <0.00%> (ø)
jina/parsers/peapods/pod.py 100.00% <0.00%> (ø)
jina/optimizers/flow_runner.py 84.44% <0.00%> (ø)
jina/parsers/peapods/runtimes/zed.py 100.00% <0.00%> (ø)
jina/parsers/peapods/runtimes/remote.py 100.00% <0.00%> (ø)
... 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 e29734b...74f8c9a. Read the comment docs.

@github-actions
Copy link
github-actions bot commented Jan 22, 2021 •
edited
Loading

Latency summary

Current PR yields:

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

Breakdown

Version Index QPS Query QPS
current 1198 31
0.9.20 1204 32

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

@JoanFM JoanFM force-pushed the refactor-decouple-types-drivers branch 3 times, most recently from e1773ef to bf6e589 Compare January 22, 2021 13:18
@jina-bot jina-bot added the area/testing This issue/PR affects testing label Jan 22, 2021
@JoanFM JoanFM force-pushed the refactor-decouple-types-drivers branch from bf6e589 to e90a770 Compare January 22, 2021 13:31
@JoanFM JoanFM marked this pull request as ready for review January 22, 2021 13:31
@JoanFM JoanFM force-pushed the refactor-decouple-types-drivers branch from e90a770 to 11831b1 Compare January 22, 2021 13:51
@JoanFM JoanFM force-pushed the refactor-decouple-types-drivers branch from 11831b1 to a803036 Compare January 22, 2021 14:15
@jina-bot jina-bot added size/M and removed size/S labels Jan 22, 2021
@JoanFM JoanFM force-pushed the refactor-decouple-types-drivers branch from ca3b14b to bd0c3e2 Compare January 22, 2021 14:20
@JoanFM JoanFM force-pushed the refactor-decouple-types-drivers branch from bd0c3e2 to 710e04b Compare January 22, 2021 14:44
@JoanFM
Copy link
Contributor Author
JoanFM commented Jan 22, 2021

I think we can invert the logic and have a QueryLang created from a Driver instead of passing a Driver to QueryLang.

nan-wang
nan-wang requested changes Jan 23, 2021
View reviewed changes
Copy link
Member
@nan-wang nan-wang left a comment

Choose a reason for hiding this comment

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

I agree upon that the types should not import drivers because conceptually drivers are one level higher than the types. However, I'm not sure using tuples for initializing QueryLang is a good solution. For me, it doesn't feel pythonic.

Joan Fontanals and others added 2 commits January 23, 2021 16:59
@nan-wang
refactor: update jina/types/querylang/__init__.py
96b3b43 
Co-authored-by: Nan Wang <nan.wang@jina.ai>
@nan-wang
refactor: update jina/types/querylang/__init__.py
dde922a 
Co-authored-by: Nan Wang <nan.wang@jina.ai>
@JoanFM
Copy link
Contributor Author
JoanFM commented Jan 23, 2021

I agree upon that the types should not import drivers because conceptually drivers are one level higher than the types. However, I'm not sure using tuples for initializing QueryLang is a good solution. For me, it doesn't feel pythonic.

I agree it does not feel the best way, I think what we could do is to have a namedtuple and have this creation come from the Driver?

Maybe the best solution would be to put QueryLang in a different module (in between types and drivers so that both Drivers and Types can depend on it. (I think there we could also fit all the dunderkeys syntax and so?

@JoanFM
Copy link
Contributor Author
JoanFM commented Jan 25, 2021

I agree upon that the types should not import drivers because conceptually drivers are one level higher than the types. However, I'm not sure using tuples for initializing QueryLang is a good solution. For me, it doesn't feel pythonic.

Hey @nan-wang can you see if you like the new changes?

@JoanFM JoanFM requested a review from nan-wang January 25, 2021 08:24
@JoanFM JoanFM requested a review from maximilianwerk January 25, 2021 12:00
@hanxiao hanxiao merged commit 317e267 into master Jan 26, 2021
@hanxiao hanxiao deleted the refactor-decouple-types-drivers branch January 26, 2021 09:22
@nan-wang nan-wang mentioned this pull request Jan 29, 2021
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanxiao hanxiao hanxiao approved these changes

@CatStark CatStark Awaiting requested review from CatStark

@florian-hoenicke florian-hoenicke Awaiting requested review from florian-hoenicke

@nan-wang nan-wang Awaiting requested review from nan-wang

@maximilianwerk maximilianwerk Awaiting requested review from maximilianwerk

Assignees
No one assigned
Labels
area/core This issue/PR affects the core codebase area/testing This issue/PR affects testing component/client component/driver component/type size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

QueryLang instantiation with Driver instance
4 participants
@JoanFM @hanxiao @nan-wang @jina-bot
0