Repository navigation
chore(wren-ai-service): minor update - #1119
Conversation
|
Warning Rate limit exceeded@cyyeh has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 34 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces several modifications across the Wren AI service configuration and provider implementations. The primary changes involve removing explicit Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
0a804a8 to
061d110
Compare
![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 (10)
deployment/kustomizations/base/cm.yaml(0 hunks)docker/config.example.yaml(0 hunks)wren-ai-service/src/providers/__init__.py(1 hunks)wren-ai-service/src/providers/embedder/azure_openai.py(1 hunks)wren-ai-service/src/providers/embedder/ollama.py(2 hunks)wren-ai-service/src/providers/embedder/openai.py(1 hunks)wren-ai-service/src/providers/llm/ollama.py(1 hunks)wren-ai-service/src/providers/loader.py(0 hunks)wren-ai-service/tools/config/config.example.yaml(0 hunks)wren-ai-service/tools/config/config.full.yaml(0 hunks)
💤 Files with no reviewable changes (5)
- docker/config.example.yaml
- wren-ai-service/src/providers/loader.py
- deployment/kustomizations/base/cm.yaml
- wren-ai-service/tools/config/config.full.yaml
- wren-ai-service/tools/config/config.example.yaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (4)
wren-ai-service/src/providers/__init__.py (1)
132-132: Verify handling of optional dimension downstream.The change from direct access to
.get()makes dimension optional. Please ensure that downstream code properly handles cases where dimension is None.Let's check for direct dimension access in the codebase:
✅ Verification successful
Dimension handling properly implemented downstream ✅
The downstream code in both OpenAI and Azure OpenAI embedders already handles optional dimensions properly through:
- Optional type hints (
Optional[int])- Explicit null checks (
if self.dimensions is not None)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for direct access to dimension that might break rg -l "(\[[\"\']dimension[\"\']|\.dimension)\b" --type py # Search for validation or assertions on dimension rg -l "assert.*dimension|validate.*dimension" --type pyLength of output: 230
Script:
#!/bin/bash # Search for dimension access patterns with context rg -A 3 -B 3 '\.dimension\b|dimension["\']|\["dimension"\]|\['\''dimension'\''\]' --type py # Search for dimension validation patterns rg -A 2 'assert.*dimension|validate.*dimension|if.*dimension|dimension.*None' --type pyLength of output: 3946
wren-ai-service/src/providers/llm/ollama.py (1)
12-12: Verify model availability without explicit pulling.The removal of
pull_ollama_modelfunctionality changes how models are initialized. Please ensure that models are still accessible when needed.Run the following script to check if the model is available without explicit pulling:
✅ Verification successful
Model availability is handled correctly without explicit pulling
The implementation relies on Ollama's built-in model management, which automatically downloads models when they're first requested. The removal of explicit pulling simplifies the code while maintaining the same functionality through Ollama's native behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for model availability references in the codebase # Test: Search for model availability checks or alternative initialization methods # Look for model availability checks rg -A 5 "model.*avail|check.*model" # Look for alternative model initialization methods ast-grep --pattern 'class OllamaLLMProvider { $$$ def __init__($$$) { $$$ } $$$ }'Length of output: 6411
Script:
#!/bin/bash # Search for OllamaLLMProvider implementation with Python syntax ast-grep --pattern 'class OllamaLLMProvider: $$$ def __init__($$$): $$$' # Look for the complete file content cat wren-ai-service/src/providers/llm/ollama.py # Search for model validation or initialization patterns rg -A 5 "def (get_model|init.*model|validate.*model)" wren-ai-service/src/providers/llm/Length of output: 6204
wren-ai-service/src/providers/embedder/ollama.py (1)
162-166: LGTM! Optional dimension parameter aligns with PR objectives.The change to make the dimension parameter optional provides more flexibility in model configuration while maintaining compatibility with the embedding interface.
wren-ai-service/src/providers/embedder/openai.py (1)
184-188: LGTM! Consistent implementation of optional dimension parameter.The change maintains consistency with other embedder providers and aligns with the PR objectives.
| dimension: Optional[int] = ( | ||
| int(os.getenv("EMBEDDING_MODEL_DIMENSION")) | ||
| if os.getenv("EMBEDDING_MODEL_DIMENSION") | ||
| else 0 | ||
| ) | ||
| or EMBEDDING_MODEL_DIMENSION, | ||
| else None, | ||
| ), |
There was a problem hiding this comment.
it seems not to be used. we can remove it or check how to push down to the embedder object.
| dimension: Optional[int] = ( | ||
| int(os.getenv("EMBEDDING_MODEL_DIMENSION")) | ||
| if os.getenv("EMBEDDING_MODEL_DIMENSION") | ||
| else 0 | ||
| ) | ||
| or EMBEDDING_MODEL_DIMENSION, | ||
| else None | ||
| ), |
| dimension: Optional[int] = ( | ||
| int(os.getenv("EMBEDDING_MODEL_DIMENSION")) | ||
| if os.getenv("EMBEDDING_MODEL_DIMENSION") | ||
| else 0 | ||
| ) | ||
| or EMBEDDING_MODEL_DIMENSION, | ||
| else None | ||
| ), |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
wren-ai-service/src/providers/document_store/qdrant.py (1)
Line range hint
315-321: Validate embedding dimension to prevent runtime errors.The current implementation sets
embedding_model_dimto 0 whenEMBEDDING_MODEL_DIMENSIONis not set. However, Qdrant requires a valid dimension for vector storage. This could lead to runtime errors when initializing the document store.Consider adding validation:
def __init__( self, location: str = os.getenv("QDRANT_HOST", "qdrant"), api_key: Optional[str] = os.getenv("QDRANT_API_KEY", None), timeout: Optional[int] = ( int(os.getenv("QDRANT_TIMEOUT")) if os.getenv("QDRANT_TIMEOUT") else 120 ), embedding_model_dim: int = ( int(os.getenv("EMBEDDING_MODEL_DIMENSION")) if os.getenv("EMBEDDING_MODEL_DIMENSION") else 0 ), recreate_index: bool = ( bool(os.getenv("SHOULD_FORCE_DEPLOY")) if os.getenv("SHOULD_FORCE_DEPLOY") else False ), **_, ): + if embedding_model_dim <= 0: + raise ValueError("embedding_model_dim must be a positive integer") self._location = location self._api_key = Secret.from_token(api_key) if api_key else None self._timeout = timeout self._embedding_model_dim = embedding_model_dim self._reset_document_store(recreate_index)
🧹 Nitpick comments (1)
wren-ai-service/src/providers/document_store/qdrant.py (1)
Line range hint
344-345: Add documentation for optional embedding dimension.The log message could be more informative about the optional nature of the embedding dimension and its requirements.
Consider updating the log message:
- logger.info( - f"Using Qdrant Document Store with Embedding Model Dimension: {self._embedding_model_dim}" - ) + logger.info( + f"Using Qdrant Document Store with Embedding Model Dimension: {self._embedding_model_dim}. " + "Note: This dimension must match your embedding model's output dimension." + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
wren-ai-service/src/core/provider.py(0 hunks)wren-ai-service/src/globals.py(0 hunks)wren-ai-service/src/providers/__init__.py(0 hunks)wren-ai-service/src/providers/document_store/qdrant.py(1 hunks)wren-ai-service/src/providers/embedder/azure_openai.py(0 hunks)wren-ai-service/src/providers/embedder/ollama.py(1 hunks)wren-ai-service/src/providers/embedder/openai.py(0 hunks)wren-ai-service/src/providers/loader.py(0 hunks)
💤 Files with no reviewable changes (6)
- wren-ai-service/src/core/provider.py
- wren-ai-service/src/providers/init.py
- wren-ai-service/src/providers/loader.py
- wren-ai-service/src/providers/embedder/azure_openai.py
- wren-ai-service/src/providers/embedder/openai.py
- wren-ai-service/src/globals.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
wren-ai-service/src/providers/embedder/ollama.py (3)
15-15: LGTM! Import cleanup aligns with removal of pull_ollama_model.The removal of the
pull_ollama_modelimport is consistent with its removal from the codebase as mentioned in the PR objectives.
Line range hint
143-151: LGTM! Constructor signature simplified by removing dimension parameter.The removal of the
dimensionparameter aligns with the PR objective of making embedding model dimensions optional. The remaining parameters maintain their functionality with proper environment variable handling.
Line range hint
152-156: Verify the impact of removing pull_ollama_model.The initialization looks good with the dimension-related logic removed. However, let's verify that the removal of
pull_ollama_modeldoesn't affect the model availability at runtime.Let's check if there are any other mechanisms ensuring model availability:
✅ Verification successful
Removal of pull_ollama_model is safe
The removal is safe as Ollama manages its own models internally through its service. The model availability is handled by the Ollama service itself, making the explicit pull method redundant.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for alternative model pulling mechanisms # Look for model pulling or initialization logic in other files # Search for model pulling patterns rg -l "pull.*model|download.*model|load.*model" --type py # Search for Ollama model initialization ast-grep --pattern 'class $name implements $_ { $$$ model = "nomic-embed-text" $$$ }'Length of output: 196
Script:
#!/bin/bash # Check the content of app.py and look for Ollama-specific patterns rg -A 5 "pull.*model|download.*model|load.*model" wren-ai-service/eval/data_curation/app.py # Search for Ollama-specific initialization and runtime checks rg -l "ollama" --type py # Look for model validation or availability checks ast-grep --pattern 'def embed($$$) { $$$ model = $_ $$$ }'Length of output: 937
wren-ai-service/src/providers/document_store/qdrant.py (2)
28-28: LGTM: Import cleanup aligns with making embedding dimension optional.The removal of
get_default_embedding_model_dimimport aligns with the PR objective of making the embedding model dimension optional.
Line range hint
346-371: Verify quantization configuration for large dimensions.The code applies binary quantization for dimensions >= 1024. Let's verify if this threshold is still appropriate for the current version of Qdrant.
✅ Verification successful
Binary quantization configuration is correctly implemented
The current implementation correctly applies binary quantization for dimensions >= 1024, which aligns with Qdrant's official recommendations. The configured embedding dimension of 3072 is well above this threshold, and the implementation includes proper performance optimizations including rescoring and oversampling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Qdrant's documentation for recommended quantization thresholds # and any related discussions or issues. # Search for quantization-related discussions in the repository rg -A 5 "binary.*quantization" # Search for dimension-related configurations rg -A 5 "dimension.*1024|1024.*dimension"Length of output: 671
Script:
#!/bin/bash # Search for embedding_model_dim configuration and usage rg -A 5 "embedding_model_dim" # Search for any quantization settings or configurations rg -B 2 -A 2 "quantization|quantize" # Look for dimension-related configurations in the codebase ast-grep --pattern 'self._embedding_model_dim = $_'Length of output: 14875
cyyeh
added
module/ai-service
Summary by CodeRabbit
Configuration Changes
gpt-4o-mini-2024-07-18Logging
Embedder Providers