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

new: Added jina clip v1 #408

Merged
merged 20 commits into from
Dec 16, 2024
Merged

new: Added jina clip v1 #408

merged 20 commits into from
Dec 16, 2024

Conversation

hh-space-invader
Copy link
Contributor

@hh-space-invader hh-space-invader commented Nov 19, 2024

Adding jinaai/jina-clip-v1
They provided two examples, the first one works and the second one complains about missing jinaai/jina-clip-v1/sentence_xlnet_config.json. The output of the first one seems to have small numbers, like they are normallized but its not mentioned so not sure tbh.

Update:
The text model needs pooling and normalizing
The image model needs the image to be square

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

  • Does your submission pass the existing tests?
  • Have you added tests for your feature?
  • Have you installed pre-commit with pip3 install pre-commit and set up hooks with pre-commit install?

New models submission:

  • Have you added an explanation of why it's important to include this model?
  • Have you added tests for the new model? Were canonical values for tests computed via the original model?
  • Have you added the code snippet for how canonical values were computed?
  • Have you successfully ran tests with your changes locally?

@hh-space-invader hh-space-invader changed the title WIP: Added jina clip text embedding new: Added jina clip text embedding Nov 21, 2024
fastembed/image/transform/functional.py Outdated Show resolved Hide resolved
fastembed/image/transform/operators.py Outdated Show resolved Hide resolved
tests/test_text_onnx_embeddings.py Outdated Show resolved Hide resolved
fastembed/image/transform/operators.py Outdated Show resolved Hide resolved
fastembed/image/transform/operators.py Show resolved Hide resolved
fastembed/image/transform/operators.py Show resolved Hide resolved
height, width = image.height, image.width

# if the size is larger than the new canvas
if width > size or height > size:
Copy link
Member

Choose a reason for hiding this comment

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

should not it be if width >= size and height >= size ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be or. Assuming that size=500, width=600, height=400. With or, it will trigger cuz width>size, with and it will not trigger cuz hight <= size (and in theory we want either dimension if higher to be cropped)

Copy link
Member

Choose a reason for hiding this comment

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

what will happen to the second dimension?

if height and width are 600 and 400, required size is 500, the result shape will be (500, 500)
So, what are those 100 pixels which occur in width? What are their values? Which color was used to fill them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It turns out that internally, the crop function pads the smaller side with zeros by default and the fill_color is not used if one of the sides > size. I modified the implementation.

fastembed/image/transform/functional.py Outdated Show resolved Hide resolved
image = image.crop((left, top, right, bottom))
return image

new_image = Image.new(mode="RGB", size=(size, size), color=fill_color)
Copy link
Member

Choose a reason for hiding this comment

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

What if we pass a grayscale image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In our post processor, the first operation is to change the image to RGB, so it shouldn't happen

fastembed/image/transform/operators.py Outdated Show resolved Hide resolved
fastembed/image/transform/operators.py Outdated Show resolved Hide resolved
Comment on lines +255 to +268
@staticmethod
def _interpolation_resolver(resample: Optional[str] = None) -> Image.Resampling:
interpolation_map = {
"nearest": Image.Resampling.NEAREST,
"lanczos": Image.Resampling.LANCZOS,
"bilinear": Image.Resampling.BILINEAR,
"bicubic": Image.Resampling.BICUBIC,
"box": Image.Resampling.BOX,
"hamming": Image.Resampling.HAMMING,
}

if resample and (method := interpolation_map.get(resample.lower())):
return method

raise ValueError(f"Unknown interpolation method: {resample}")
Copy link
Member

Choose a reason for hiding this comment

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

feels like it should not be a part of Compose class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt the same. Got any suggestions ? fastembed/common/utils ?

Copy link
Member

Choose a reason for hiding this comment

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

idk for sure, we can just move it out of the class
at least, this Compose._interpolation_resolver is super ugly, if we keep this method here, we need to make get_resize a class method, not a static, it is only used inside of Compose class anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed get_resize to cls method as _interpolation_resolver would only be used in here

fastembed/text/pooled_embedding.py Outdated Show resolved Hide resolved
@joein joein changed the title new: Added jina clip text embedding new: Added jina clip v1 Dec 4, 2024
Copy link
Member

@joein joein left a comment

Choose a reason for hiding this comment

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

please look at the comments above

@hh-space-invader hh-space-invader merged commit 3b5e4c8 into main Dec 16, 2024
17 checks passed
@hh-space-invader hh-space-invader deleted the support-jina-clip branch December 16, 2024 10:45
@I8dNLo I8dNLo mentioned this pull request Dec 24, 2024
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