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

refactor(proto): remove content_hash from builtin proto #3058

Merged
merged 8 commits into from Jul 31, 2021

Conversation

hanxiao
Copy link
Member

@hanxiao hanxiao commented Jul 31, 2021

No description provided.

@hanxiao hanxiao requested a review from a team as a code owner July 31, 2021 11:44
@github-actions github-actions bot added size/M area/core This issue/PR affects the core codebase area/network This issue/PR affects network functionality area/testing This issue/PR affects testing component/proto component/type labels Jul 31, 2021
@codecov
Copy link

codecov bot commented Jul 31, 2021

Codecov Report

Merging #3058 (8fd5ecd) into master (b205a19) will decrease coverage by 0.06%.
The diff coverage is 90.90%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3058      +/-   ##
==========================================
- Coverage   89.53%   89.46%   -0.07%     
==========================================
  Files         142      142              
  Lines        9515     9503      -12     
==========================================
- Hits         8519     8502      -17     
- Misses        996     1001       +5     
Flag Coverage Δ
daemon 43.94% <36.36%> (-0.01%) ⬇️
jina 89.45% <90.90%> (-0.07%) ⬇️

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

Impacted Files Coverage Δ
jina/proto/jina_pb2.py 100.00% <ø> (ø)
jina/types/arrays/match.py 95.45% <ø> (-0.38%) ⬇️
jina/types/arrays/chunk.py 89.28% <80.00%> (-3.31%) ⬇️
jina/types/document/__init__.py 96.13% <85.71%> (-0.42%) ⬇️
jina/clients/base/websocket.py 86.36% <100.00%> (ø)
jina/clients/request/helper.py 83.92% <100.00%> (-0.56%) ⬇️
jina/proto/serializer.py 100.00% <100.00%> (ø)
jina/types/arrays/document.py 90.18% <100.00%> (ø)
jina/types/document/multimodal.py 95.91% <100.00%> (-0.38%) ⬇️
jina/types/message/__init__.py 87.25% <100.00%> (ø)
... and 5 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 b205a19...8fd5ecd. Read the comment docs.

@github-actions
Copy link

github-actions bot commented Jul 31, 2021

Latency summary

Current PR yields:

  • 😶 index QPS at 1785, delta to last 2 avg.: -1%
  • 🐎🐎🐎🐎 query QPS at 46, delta to last 2 avg.: +104%
  • 🐎🐎🐎🐎 dam extend QPS at 84630, delta to last 2 avg.: +19%
  • 🐎🐎🐎🐎 avg flow time within 1.4736 seconds, delta to last 2 avg.: +5%
  • 🐢🐢 import jina within 0.305 seconds, delta to last 2 avg.: -7%

Breakdown

Version Index QPS Query QPS DAM Extend QPS Avg Flow Time (s) Import Time (s)
current 1785 46 84630 1.4736 0.305
2.0.13 1603 19 63021 1.3261 0.3682
2.0.12 2015 25 78135 1.458 0.293

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

@bwanglzu
Copy link
Member

how about add @cached_property the content hash?

@hanxiao
Copy link
Member Author

hanxiao commented Jul 31, 2021

how about add @cached_property the content hash?

no, that will introduce inconsistency issue, leads to more bug. In general, @cached_property always leads to inconsistency to some extent, but in this case Document fields change much more frequently than other cases. so no.

@hanxiao hanxiao mentioned this pull request Jul 31, 2021
@hanxiao hanxiao merged commit 390ff0a into master Jul 31, 2021
@hanxiao hanxiao deleted the refactor-rm-content-hash branch July 31, 2021 14:24
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/network This issue/PR affects network functionality area/testing This issue/PR affects testing component/client component/proto component/type size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants