Embed image for Virchow2#6
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as "Caller"
participant Models as "Models / AsyncModels"
participant Converter as "Image -> uint8 bytes"
participant Compressor as "LZ4 Compressor"
participant ModelAPI as "Model API (POST)"
participant Parser as "_parse_embedding_response"
Caller->>Models: embed_image(model, image, output_dtype, timeout)
Models->>Converter: convert PIL/NDArray -> uint8 bytes
Converter->>Compressor: compress bytes (LZ4)
Compressor->>ModelAPI: POST compressed payload
ModelAPI-->>Models: response (embedding bytes)
Models->>Parser: parse response, cast to output_dtype
Parser-->>Caller: NDArray (embedding)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces embed_image methods to both the synchronous Models and asynchronous AsyncModels classes, enabling image embedding functionality. The implementation is consistent with existing methods in the file. My review includes suggestions to improve input validation for the output_dtype parameter to prevent unexpected behavior with invalid inputs.
There was a problem hiding this comment.
Pull request overview
Adds an image-embedding API method to the Models and AsyncModels resources so clients can request embedding vectors (e.g., for Virchow2) using the existing models service transport patterns.
Changes:
- Added
embed_image()toModels(sync) to POST an lz4-compressed image and parse embeddings into a NumPy array. - Added
embed_image()toAsyncModels(async) with the same behavior. - Added typing imports to support the new method’s type signatures (
Any,Literal).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
rationai/resources/models.py (1)
85-86: Enforce the documented 1-D embedding contract.Line 85 and Line 166 document a 1-D embedding vector, but the current return path accepts any JSON shape. Consider validating
ndim == 1before returning.💡 Proposed fix
- return np.array(response.json(), dtype=np_dtype) + embedding = np.asarray(response.json(), dtype=np_dtype) + if embedding.ndim != 1: + raise ValueError(f"Expected 1-D embedding, got shape {embedding.shape}") + return embedding @@ - return np.array(response.json(), dtype=np_dtype) + embedding = np.asarray(response.json(), dtype=np_dtype) + if embedding.ndim != 1: + raise ValueError(f"Expected 1-D embedding, got shape {embedding.shape}") + return embeddingAlso applies to: 92-93, 166-167, 173-173
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rationai/resources/models.py` around lines 85 - 86, The code documents embeddings as a 1-D numpy array (NDArray[np.floating[Any]]) but currently returns any JSON shape; update the embedding-return path(s) that produce/parse the embedding (the functions documented around the NDArray return types) to validate that the numpy array has ndim == 1 before returning, and raise a clear ValueError (e.g., "embedding must be 1-D, got ndim=X, shape=Y") if not; add this check in every location that returns the embedding (the functions/methods documented at the NDArray return lines) and add a small unit test asserting that non-1D input raises the error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rationai/resources/models.py`:
- Around line 73-74: The code currently silently coerces unknown output_dtype
values to np.float32; change this to explicit validation: in any function or
class constructor that accepts the output_dtype parameter (named output_dtype)
validate it against the allowed set {"float16","float32"} and if it is not one
of these values raise a ValueError with a clear message; then map the validated
string to the numpy dtype via a small dict (e.g. {"float16": np.float16,
"float32": np.float32}) instead of using a default fallback, and update all code
paths that currently fallback to np.float32 to use this validated mapping
(search for uses of output_dtype and the implicit np.float32 fallback and
replace with the validator + mapping).
---
Nitpick comments:
In `@rationai/resources/models.py`:
- Around line 85-86: The code documents embeddings as a 1-D numpy array
(NDArray[np.floating[Any]]) but currently returns any JSON shape; update the
embedding-return path(s) that produce/parse the embedding (the functions
documented around the NDArray return types) to validate that the numpy array has
ndim == 1 before returning, and raise a clear ValueError (e.g., "embedding must
be 1-D, got ndim=X, shape=Y") if not; add this check in every location that
returns the embedding (the functions/methods documented at the NDArray return
lines) and add a small unit test asserting that non-1D input raises the error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ccf814b0-7849-4149-b178-7753d77e0e6d
📒 Files selected for processing (1)
rationai/resources/models.py
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
matejpekar
requested review from
Adames4
and removed request for
JakubPekar and
ejdam87
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rationai/resources/models.py (1)
98-107: Extract shared embed request/parse logic to reduce sync/async drift.The serialization/header/parsing flow is duplicated in both methods. A small shared helper would reduce maintenance risk and keep behavior consistent.
Also applies to: 183-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rationai/resources/models.py` around lines 98 - 107, The image serialization, header setup, request posting and response parsing logic duplicated between the sync and async embedding flows should be extracted into a shared helper; create a helper (e.g., _prepare_and_post_embedding or _send_embedding_request) that accepts the same inputs used in both places (self, model, image, output_dtype, timeout) and performs np.asarray(..., dtype=np.uint8), lz4.frame.compress(... .tobytes()), sets headers={"x-output-dtype": np.dtype(output_dtype).name}, posts via the appropriate internal requester, and calls _parse_embedding_response to return the parsed result; update both the sync code path that calls self._post and the async path (the corresponding async post caller around lines ~183-192) to delegate to this shared helper to eliminate duplication and keep behavior consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rationai/resources/models.py`:
- Around line 16-17: The annotated return type NDArray[np.floating[Any]] is
incorrect for functions that accept a generic output_dtype; update the return
annotations on the functions that take the parameter output_dtype (the three
signatures shown that currently end with -> NDArray[np.floating[Any]] ) to a
generic NDArray[Any] so the type matches whatever dtype is requested at runtime,
and ensure the necessary typing imports (Any, NDArray) are present or adjusted
accordingly.
- Around line 98-99: Add explicit input validation before the np.asarray(image,
dtype=np.uint8) call: detect if `image` is a PIL Image (isinstance(image,
Image.Image)) and ensure image.mode is an expected mode (e.g., 'RGB' or 'L') or
else raise a clear ValueError advising the caller to convert (e.g.,
image.convert('RGB')). If `image` is a numpy array, check its dtype
(image.dtype) and raise a ValueError if it is not np.uint8 instead of silently
coercing; alternatively, perform an explicit, documented conversion step (with a
clear log or comment) before creating `image_array`. Apply this validation
around the `image_array = np.asarray(image, dtype=np.uint8)` and
`compressed_data = lz4.frame.compress(image_array.tobytes())` sites so callers
get a helpful error instead of silent data truncation.
---
Nitpick comments:
In `@rationai/resources/models.py`:
- Around line 98-107: The image serialization, header setup, request posting and
response parsing logic duplicated between the sync and async embedding flows
should be extracted into a shared helper; create a helper (e.g.,
_prepare_and_post_embedding or _send_embedding_request) that accepts the same
inputs used in both places (self, model, image, output_dtype, timeout) and
performs np.asarray(..., dtype=np.uint8), lz4.frame.compress(... .tobytes()),
sets headers={"x-output-dtype": np.dtype(output_dtype).name}, posts via the
appropriate internal requester, and calls _parse_embedding_response to return
the parsed result; update both the sync code path that calls self._post and the
async path (the corresponding async post caller around lines ~183-192) to
delegate to this shared helper to eliminate duplication and keep behavior
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db7239bb-ebbb-4c6b-bee7-24c304343631
📒 Files selected for processing (1)
rationai/resources/models.py
|
@Jurgee always resolve the comments that are no more relevant before requesting another review! |
|
I accidentally closed this PR. Sorry about that. PR is open right now |
Summary by CodeRabbit
New Features
Chores / API Changes
Unchanged