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

add vision client template #526

Merged
merged 1 commit into from
Feb 11, 2025
Merged

add vision client template #526

merged 1 commit into from
Feb 11, 2025

Conversation

wirthual
Copy link
Collaborator

@wirthual wirthual commented Feb 3, 2025

Related Issue

Checklist

  • I have read the CONTRIBUTING guidelines.
  • I have added tests to cover my changes.
  • I have updated the documentation (docs folder) accordingly.

Additional Notes

Add any other context about the PR here.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Added a new vision client template that enables both image and text embedding functionality through a unified API interface.

  • Added InfinityVisionAPI class in /libs/client_infinity/template/vision_client.py with concurrent request handling via ThreadPoolExecutor
  • Implemented robust error handling with request retries and HTTP session management using HTTPAdapter
  • Added base64 encoding/decoding support for image processing with PIL integration
  • Added semaphore-based request throttling with a limit of 64 concurrent requests
  • Added automatic hidden dimension detection during client initialization for proper embedding shape handling

💡 (5/5) You can turn off certain types of comments like style here!

1 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile

Comment on lines +36 to +37
self.tp = ThreadPoolExecutor()
self.tp.__enter__()
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: ThreadPoolExecutor is entered but never exited. Need a del or close method to properly clean up resources.

Suggested change
self.tp = ThreadPoolExecutor()
self.tp.__enter__()
self.tp = ThreadPoolExecutor()
self.tp.__enter__()
# Add destructor to ensure cleanup
def __del__(self):
self.tp.__exit__(None, None, None)

Comment on lines +5 to +10
import numpy as np # Import numpy
from typing import Union
HAS_IMPORTS = True
try:
from PIL import Image
import numpy as np
Copy link
Contributor

Choose a reason for hiding this comment

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

style: numpy is imported twice unnecessarily

Suggested change
import numpy as np # Import numpy
from typing import Union
HAS_IMPORTS = True
try:
from PIL import Image
import numpy as np
import numpy as np # Import numpy
from typing import Union
HAS_IMPORTS = True
try:
from PIL import Image

Comment on lines +102 to +104
def embed(self, model: str, sentences: list[str]) -> Future[list]:
self.health()
with self.sem:
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: health check failure should prevent the embedding request rather than just raising separately

req.raise_for_status()
return req.status_code == 200

def _request(self, model: str, images_or_text: list[Union["Image.Image", str]]) -> dict:
Copy link
Contributor

Choose a reason for hiding this comment

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

syntax: return type hint should be tuple[list, int] instead of dict

Suggested change
def _request(self, model: str, images_or_text: list[Union["Image.Image", str]]) -> dict:
def _request(self, model: str, images_or_text: list[Union["Image.Image", str]]) -> tuple[list, int]:

Comment on lines +112 to +116
def test_colpali():
colpali = InfinityVisionAPI()
future = colpali.embed("michaelfeil/colqwen2-v0.1", ["test"])
embeddings, total_tokens = future.result()
print(embeddings, total_tokens)
Copy link
Contributor

Choose a reason for hiding this comment

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

style: test function should use assertions instead of print statements

@codecov-commenter
Copy link

codecov-commenter commented Feb 3, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.86%. Comparing base (b17b588) to head (8003c5b).

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #526      +/-   ##
==========================================
+ Coverage   79.74%   79.86%   +0.11%     
==========================================
  Files          43       43              
  Lines        3486     3486              
==========================================
+ Hits         2780     2784       +4     
+ Misses        706      702       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wirthual wirthual requested a review from michaelfeil February 6, 2025 14:29
@wirthual wirthual merged commit 215f795 into main Feb 11, 2025
36 checks passed
@wirthual wirthual deleted the add-vision-client-template branch February 11, 2025 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants