-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
feat: in-place result in clip_client; preserve output order by uid #815
Conversation
Codecov Report
@@ Coverage Diff @@
## main #815 +/- ##
==========================================
+ Coverage 84.30% 84.67% +0.37%
==========================================
Files 21 21
Lines 1548 1573 +25
==========================================
+ Hits 1305 1332 +27
+ Misses 243 241 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
4d41716
to
69ef7b4
Compare
69ef7b4
to
f5eadd5
Compare
client/clip_client/client.py
Outdated
|
||
def _gather_result(self, response, results: 'DocumentArray'): | ||
def _gather_result( | ||
self, response, results: 'DocumentArray', attribute: Optional[str] = '' |
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.
please add docstring
for attribute
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.
default value is None, not ''
client/clip_client/client.py
Outdated
from rich import filesize | ||
from docarray import Document | ||
|
||
if hasattr(self, '_pbar'): | ||
self._pbar.start_task(self._s_task) | ||
|
||
for c in content: | ||
for i, c in enumerate(content): |
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.
where is this i
used?
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.
it is used to mark the order of input, and it is now deprecated. Will remove it
self._prepare_streaming( | ||
not kwargs.get('show_progress'), | ||
total=len(docs), | ||
total=len(docs) if hasattr(docs, '__len__') else None, |
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.
please add test for generator in rank
and index
tests/test_client.py
Outdated
|
||
|
||
@pytest.mark.asyncio | ||
async def test_str_input(make_torch_flow): |
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.
rename test_str_input
-> test_wrong_type_input
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.
LGTM
This PR introduces the following changes to CAS:
clip_client
used to return a copy of the input with embeddings and ranking Infos in it. It now updates these into the original input.Notice: the result is not complete if the ids of input are not unique
Basic tests for memory usage:
Send 10 batches of images with each batch_size = 8
before (no in-place) in MB:
after (in-place) in MB:
Basic tests for time usage:
Time for encoding 1000 images (local machine with CPU):
before (no in-place) in second:
after (in-place) in second:
Another test to encode 10000 images (local with CPU):
Test to encode 10000 images 10 times (GPU server):
before (no in-place) in second:
after (in-place) in second:
we don't observe a substantial difference after this change