-
Notifications
You must be signed in to change notification settings - Fork 111
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
poetry unlock #430
poetry unlock #430
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #430 +/- ##
==========================================
+ Coverage 79.14% 79.27% +0.12%
==========================================
Files 41 41
Lines 3246 3242 -4
==========================================
+ Hits 2569 2570 +1
+ Misses 677 672 -5 ☔ View full report in Codecov by Sentry. |
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.
PR Summary
This pull request primarily focuses on updating the project's build system and code formatting:
- Switched from 'black' to 'ruff' for linting and formatting in Makefiles
- Updated dependencies in pyproject.toml, including infinity_emb to version 0.0.65
- Improved code readability by removing unnecessary line breaks and simplifying expressions
- Modified the base64 encoding process for embeddings in OpenAIEmbeddingResult class
- Consolidated warning message about ct2 inference deprecation in transformer/embedder/ct2.py
28 file(s) reviewed, 26 comment(s)
Edit PR Review Bot Settings | Greptile
poetry run ruff . | ||
[ "$(PYTHON_FILES)" = "" ] || poetry run black $(PYTHON_FILES) --check | ||
poetry run ruff check . | ||
|
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.
style: An empty line has been introduced here. Consider removing it to maintain consistent spacing in the Makefile.
@@ -32,13 +32,12 @@ lint format: PYTHON_FILES=. | |||
lint_diff format_diff: PYTHON_FILES=$(shell git diff --relative=libs/infinity_emb --name-only --diff-filter=d master | grep -E '\.py$$|\.ipynb$$') |
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.
logic: This line uses 'libs/infinity_emb' in the path. Ensure this is the correct path, as the Makefile is in 'libs/embed_package'.
PYTHON_FILES=. | ||
lint format: PYTHON_FILES=. | ||
lint: PYTHON_FILES=./infinity_emb | ||
lint_diff format_diff: PYTHON_FILES=$(shell git diff --relative=libs/infinity_emb --name-only --diff-filter=d master | grep -E '\.py$$|\.ipynb$$') |
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.
style: PYTHON_FILES variable is defined differently for various tasks, may lead to inconsistent file selection
# return core_schema.no_info_after_validator_function(cls, handler(str)) | ||
return core_schema.no_info_after_validator_function( | ||
cls.validate, core_schema.str_schema() | ||
) | ||
return core_schema.no_info_after_validator_function(cls.validate, core_schema.str_schema()) |
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.
style: The commented-out line and the new line are functionally equivalent. Consider removing the commented line to improve code clarity.
else: | ||
embeddings = [e.tolist() for e in embeddings] |
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.
style: The else condition now always converts embeddings to lists, removing the previous check for numpy arrays. Ensure this doesn't cause issues with other embedding types.
@@ -31,9 +31,7 @@ async def resolve_audio( | |||
# |
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.
style: There's a commented line here (#). Consider removing it if it's not needed, or add a comment explaining its purpose if it's intentionally left for future use.
resolve_audio(audio, allowed_sampling_rate, session) | ||
for audio in audio_urls | ||
] | ||
*[resolve_audio(audio, allowed_sampling_rate, session) for audio in audio_urls] |
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.
style: This line has been compacted. Ensure that it doesn't exceed the project's line length limit, which could affect readability.
assert ( | ||
percentile > 50 and percentile <= 100 | ||
), "percentile should be between 50 and 100" | ||
assert percentile > 50 and percentile <= 100, "percentile should be between 50 and 100" |
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.
style: Assertion simplified. Verify if this affects readability.
if self.is_colipali: | ||
self.model = torch.compile(self.model, dynamic=True) | ||
else: | ||
self.model.vision_model = torch.compile( | ||
self.model.vision_model, dynamic=True | ||
) | ||
self.model.text_model = torch.compile( | ||
self.model.text_model, dynamic=True | ||
) | ||
self.model.vision_model = torch.compile(self.model.vision_model, dynamic=True) | ||
self.model.text_model = torch.compile(self.model.text_model, dynamic=True) |
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.
style: The compilation of vision_model and text_model has been simplified to single-line statements. Ensure this doesn't affect readability or debugging.
embeddings = list( | ||
next(image_embeds if is_img else text_embeds) for is_img in type_is_img | ||
) | ||
embeddings = list(next(image_embeds if is_img else text_embeds) for is_img in type_is_img) |
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.
style: This list comprehension might be less readable than a for loop. Consider refactoring for clarity if it becomes complex.
No description provided.