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

Draft demo #778

Closed
wants to merge 11 commits into from
Closed

Draft demo #778

wants to merge 11 commits into from

Conversation

jemmyshin
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Jul 22, 2022

Codecov Report

Merging #778 (4f9c8f1) into main (32b11cd) will increase coverage by 0.90%.
The diff coverage is 62.79%.

❗ Current head 4f9c8f1 differs from pull request most recent head ac7acb7. Consider uploading reports for the commit ac7acb7 to get more accurate results

@@            Coverage Diff             @@
##             main     #778      +/-   ##
==========================================
+ Coverage   81.91%   82.81%   +0.90%     
==========================================
  Files          17       21       +4     
  Lines        1255     1135     -120     
==========================================
- Hits         1028      940      -88     
+ Misses        227      195      -32     
Flag Coverage Δ
cas 82.81% <62.79%> (+0.90%) ⬆️

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

Impacted Files Coverage Δ
client/clip_client/client.py 82.14% <62.79%> (-4.53%) ⬇️
server/clip_server/model/trt_utils.py 56.04% <0.00%> (-27.48%) ⬇️
server/clip_server/model/clip_trt.py 71.05% <0.00%> (-21.06%) ⬇️
server/clip_server/model/clip_onnx.py 88.23% <0.00%> (-8.74%) ⬇️
server/clip_server/executors/helper.py 97.05% <0.00%> (-2.95%) ⬇️
server/clip_server/executors/clip_tensorrt.py 100.00% <0.00%> (ø)
server/clip_server/model/model.py
server/clip_server/model/openclip_model.py 89.28% <0.00%> (ø)
server/clip_server/model/mclip_model.py 83.33% <0.00%> (ø)
server/clip_server/model/clip_model.py 84.21% <0.00%> (ø)
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

client/clip_client/client.py Outdated Show resolved Hide resolved
client/clip_client/client.py Outdated Show resolved Hide resolved
client/clip_client/client.py Outdated Show resolved Hide resolved
client/clip_client/client.py Outdated Show resolved Hide resolved
Comment on lines 124 to 129
self._client.post(
**payload,
on=f'/encode/{model_name}'.rstrip('/'),
inputs=self._iter_doc(content),
on_done=partial(self._gather_result, results=results),
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self._client.post(
**payload,
on=f'/encode/{model_name}'.rstrip('/'),
inputs=self._iter_doc(content),
on_done=partial(self._gather_result, results=results),
)
self._client.post(
on=f'/encode/{model_name}'.rstrip('/'),
inputs=self._iter_doc(content),
on_done=partial(self._gather_result, results=results),
**payload,
)

client/clip_client/client.py Show resolved Hide resolved
@numb3r3
Copy link
Member

numb3r3 commented Jul 22, 2022

@jemmyshin Pleas rebase your PR to fix the conflict.

Copy link
Member

@numb3r3 numb3r3 left a comment

Choose a reason for hiding this comment

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

We should take care of the user experience. Let's polish the client API to offer an easy-to-use client regarding to user's perspective.


parameters = kwargs.pop('parameters', {})
model_name = parameters.get('model', '')
payload = self._get_post_parameters(content, kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

rename it as params?


def index(self, content: Iterable['Document'], **kwargs):
"""Index the embeddings created by server CLIP model.
Given the document with embeddings, this function create an indexer which index
Copy link
Member

Choose a reason for hiding this comment

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

The docstr is not corrct. index and query functions both accept raw text/image docs without embeddings.

Copy link
Member

Choose a reason for hiding this comment

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

What's more, we also implment async version, aindex, aquery

Copy link
Member

Choose a reason for hiding this comment

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

The index should have the same function signature as encode

with self._pbar:
self._client.post(on='/index', inputs=self._iter_doc(content), **payload)

def search(self, content: List[str], **kwargs) -> DocumentArray:
Copy link
Member

Choose a reason for hiding this comment

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

search should also accept either str and Document

total=len(content) if hasattr(content, '__len__') else None,
)
results = DocumentArray()

parameters = kwargs.pop('parameters', {})
Copy link
Member

Choose a reason for hiding this comment

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

alright, pop operator would raise an exception when the given key does not exist.

client/clip_client/client.py Outdated Show resolved Hide resolved
client/clip_client/client.py Outdated Show resolved Hide resolved
client/clip_client/client.py Outdated Show resolved Hide resolved
client/clip_client/client.py Outdated Show resolved Hide resolved
client/clip_client/client.py Outdated Show resolved Hide resolved
client/clip_client/client.py Outdated Show resolved Hide resolved
client/clip_client/client.py Outdated Show resolved Hide resolved
client/clip_client/client.py Outdated Show resolved Hide resolved
client/clip_client/client.py Outdated Show resolved Hide resolved
client/clip_client/client.py Show resolved Hide resolved
@jemmyshin jemmyshin requested a review from numb3r3 July 26, 2022 03:22
@@ -114,10 +117,17 @@ def encode(self, content, **kwargs):
total=len(content) if hasattr(content, '__len__') else None,
)
results = DocumentArray()

model_name = parameters.get('model', '')
Copy link
Member

Choose a reason for hiding this comment

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

raise error when parameters=None

@numb3r3
Copy link
Member

numb3r3 commented Jul 26, 2022

@jemmyshin please rebase your PR to fix conflicts

@jemmyshin jemmyshin marked this pull request as ready for review July 26, 2022 04:16
@@ -284,9 +292,16 @@ async def aencode(self, content, **kwargs):
)

results = DocumentArray()

model_name = parameters.get('model', '')
Copy link
Member

Choose a reason for hiding this comment

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

parameters can be None, and will raise error here. Please fix it

jtype: AnnLiteIndexer
with:
dim: 512
data_path: /Users/jemfu/Desktop/laion400m/workspace
Copy link
Member

Choose a reason for hiding this comment

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

don't use absolute real path


with self._pbar:
self._client.post(
on='/index',
Copy link
Member

@ZiniuYu ZiniuYu Jul 28, 2022

Choose a reason for hiding this comment

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

will we also support selection from different indexers?

@numb3r3
Copy link
Member

numb3r3 commented Sep 9, 2022

@jemmyshin This PR has been merged to #816. I will close this PR first. let's work on the new PR.

@numb3r3 numb3r3 closed this Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants