Fix IndexStats returning attribute names instead of values #1095
Conversation
|
""" WalkthroughThe Changes
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
meilisearch/models/index.py(2 hunks)tests/models/test_index.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
meilisearch/models/index.py (1)
meilisearch/models/embedders.py (6)
OpenAiEmbedder(23-56)HuggingFaceEmbedder(59-92)OllamaEmbedder(95-128)RestEmbedder(131-170)UserProvidedEmbedder(173-191)Embedders(204-213)
🪛 Ruff (0.11.9)
meilisearch/models/index.py
123-123: Undefined name Union
(F821)
🔇 Additional comments (2)
meilisearch/models/index.py (1)
22-25: LGTM! Core bug fix implemented correctly.The fix properly returns the attribute value from
self.__dict[attr]instead of returning the attribute name string. This addresses the main issue described in the PR objectives.🧰 Tools
🪛 Ruff (0.11.9)
23-23: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
tests/models/test_index.py (1)
8-10: Test correctly updated to match fixed behavior.The test assertion now properly verifies that
__getattr__("field1")returns the actual value"test 1"instead of the attribute name"field1". This aligns with the correctedIndexStats.__getattr__implementation.
meilisearch/models/index.py
Outdated
| class OpenAiEmbedder(CamelBase): | ||
| source: str = "openAi" | ||
| url: Optional[str] = None | ||
| model: Optional[str] = None # Defaults to text-embedding-3-small | ||
| dimensions: Optional[int] = None # Uses the model default | ||
| api_key: Optional[str] = None # Can be provided through a CLI option or environment variable | ||
| document_template: Optional[str] = None | ||
| document_template_max_bytes: Optional[int] = None # Default to 400 | ||
| distribution: Optional[EmbedderDistribution] = None | ||
| binary_quantized: Optional[bool] = None | ||
|
|
||
|
|
||
| class HuggingFaceEmbedder(CamelBase): | ||
| source: str = "huggingFace" | ||
| model: Optional[str] = None # Defaults to BAAI/bge-base-en-v1.5 | ||
| revision: Optional[str] = None | ||
| document_template: Optional[str] = None | ||
| document_template_max_bytes: Optional[int] = None # Default to 400 | ||
| distribution: Optional[EmbedderDistribution] = None | ||
| binary_quantized: Optional[bool] = None | ||
|
|
||
|
|
||
| class OllamaEmbedder(CamelBase): | ||
| source: str = "ollama" | ||
| url: Optional[str] = None | ||
| api_key: Optional[str] = None | ||
| model: str | ||
| document_template: Optional[str] = None | ||
| document_template_max_bytes: Optional[int] = None # Default to 400 | ||
| distribution: Optional[EmbedderDistribution] = None | ||
| binary_quantized: Optional[bool] = None | ||
|
|
||
|
|
||
| class RestEmbedder(CamelBase): | ||
| source: str = "rest" | ||
| url: str | ||
| api_key: Optional[str] # required for protected APIs | ||
| document_template: Optional[str] = None | ||
| document_template_max_bytes: Optional[int] = None # Default to 400 | ||
| request: Dict[str, Any] | ||
| response: Dict[str, Any] | ||
| distribution: Optional[EmbedderDistribution] = None | ||
| headers: Optional[Dict[str, Any]] = None | ||
| binary_quantized: Optional[bool] = None | ||
|
|
||
|
|
||
| class UserProvidedEmbedder(CamelBase): | ||
| source: str = "userProvided" | ||
| dimensions: int | ||
| distribution: Optional[EmbedderDistribution] = None | ||
| binary_quantized: Optional[bool] = None | ||
|
|
||
|
|
||
| class Embedders(CamelBase): | ||
| embedders: Dict[ | ||
| str, | ||
| Union[OpenAiEmbedder, HuggingFaceEmbedder, OllamaEmbedder, RestEmbedder, UserProvidedEmbedder], | ||
| ] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential code duplication with existing embedder classes.
The embedder classes defined here (OpenAiEmbedder, HuggingFaceEmbedder, OllamaEmbedder, RestEmbedder, UserProvidedEmbedder, Embedders) appear to duplicate functionality that already exists in meilisearch/models/embedders.py based on the relevant code snippets provided. This could lead to maintenance issues and confusion about which classes to use.
Please verify if these classes are intended to replace the existing ones or if there's a different purpose for having them in both locations:
🏁 Script executed:
#!/bin/bash
# Description: Check for duplicate embedder class definitions across the codebase
# Expected: Find if these classes exist in multiple files
echo "Searching for embedder class definitions..."
rg -A 2 "class.*Embedder.*CamelBase"
echo -e "\nSearching for Embedders container class..."
rg -A 2 "class Embedders"Length of output: 2856
Consolidate duplicate embedder definitions
The OpenAiEmbedder, HuggingFaceEmbedder, OllamaEmbedder, RestEmbedder, UserProvidedEmbedder and Embedders classes live in both:
meilisearch/models/index.py(lines 67–124)meilisearch/models/embedders.py
Maintaining two copies will lead to drift and confusion. Please remove or de-duplicate one set of definitions—ideally centralizing all embedder models in meilisearch/models/embedders.py (or vice versa)—and update any imports accordingly.
🧰 Tools
🪛 Ruff (0.11.9)
123-123: Undefined name Union
(F821)
🤖 Prompt for AI Agents
In meilisearch/models/index.py around lines 67 to 124, the embedder classes
OpenAiEmbedder, HuggingFaceEmbedder, OllamaEmbedder, RestEmbedder,
UserProvidedEmbedder, and Embedders are duplicated from
meilisearch/models/embedders.py. To fix this, remove these class definitions
from index.py and update all imports in the codebase to reference the
centralized definitions in embedders.py, ensuring no duplicate embedder classes
remain.
Related issue
Fixes #1094
What does this PR do?
__getattr__method of theIndexStatsclass return the attribute value not its namePR checklist
Please check if your PR fulfills the following requirements:
Summary by CodeRabbit
New Features
Bug Fixes
Tests