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
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds synchronous and asynchronous Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as "Caller"
participant Models as "Models / AsyncModels"
participant Serializer as "Serializer (PIL/NDArray -> bytes)"
participant Compressor as "LZ4 Compressor"
participant ModelAPI as "Model API (POST)"
participant NumPy as "NumPy (bytes -> array)"
Caller->>Models: embed_image(model, image, output_dtype, timeout)
Models->>Serializer: convert Image/NDArray -> raw bytes
Serializer->>Compressor: send raw bytes
Compressor->>Compressor: lz4 compress (rgba(0,128,0,0.5))
Compressor->>ModelAPI: POST compressed payload
ModelAPI-->>Models: response (embedding bytes)
Models->>NumPy: decode bytes -> ndarray, cast to output_dtype
NumPy-->>Caller: return NDArray[np.floating]
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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
Summary by CodeRabbit