-
Notifications
You must be signed in to change notification settings - Fork 954
Fix Ollama embedding response key handling #1809
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| """ | ||
| Test script to verify OllamaEmbeddingEngine fix with real Ollama server. | ||
| Tests that the fix correctly handles Ollama's API response format. | ||
| """ | ||
| import asyncio | ||
| import sys | ||
| from cognee.infrastructure.databases.vector.embeddings.OllamaEmbeddingEngine import ( | ||
| OllamaEmbeddingEngine, | ||
| ) | ||
|
|
||
|
|
||
| async def test_ollama_embedding(): | ||
| """Test OllamaEmbeddingEngine with real Ollama server.""" | ||
|
|
||
| print("=" * 80) | ||
| print("Testing OllamaEmbeddingEngine Fix") | ||
| print("=" * 80) | ||
|
|
||
| # Configure for your Ollama server | ||
| ollama_endpoint = "http://10.0.10.9:11434/api/embeddings" | ||
| ollama_model = "nomic-embed-text" | ||
|
|
||
| print("\nConfiguration:") | ||
| print(f" Endpoint: {ollama_endpoint}") | ||
| print(f" Model: {ollama_model}") | ||
| print(" Expected dimensions: 768") | ||
|
|
||
| # Initialize the embedding engine | ||
| print("\n1. Initializing OllamaEmbeddingEngine...") | ||
| try: | ||
| engine = OllamaEmbeddingEngine( | ||
| model=ollama_model, | ||
| dimensions=768, | ||
| endpoint=ollama_endpoint, | ||
| huggingface_tokenizer="bert-base-uncased", | ||
| ) | ||
| print(" ✅ Engine initialized successfully") | ||
| except Exception as e: | ||
| print(f" ❌ Failed to initialize engine: {e}") | ||
| sys.exit(1) | ||
|
Comment on lines
+38
to
+40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major Replace sys.exit() with proper test assertions. Using If keeping this as an integration test script, at least replace sys.exit() with exceptions: except Exception as e:
print(f" ❌ Failed to initialize engine: {e}")
- sys.exit(1)
+ raise RuntimeError(f"Engine initialization failed: {e}") from eBetter yet, use the proper unit test approach suggested in the previous comment. Also applies to: 59-64, 81-83 🧰 Tools🪛 Ruff (0.14.5)39-39: Do not catch blind exception: (BLE001) 🤖 Prompt for AI Agents |
||
|
|
||
| # Test single text embedding | ||
| print("\n2. Testing single text embedding...") | ||
| test_texts = ["The sky is blue and the grass is green."] | ||
|
|
||
| try: | ||
| embeddings = await engine.embed_text(test_texts) | ||
| print(" ✅ Embedding generated successfully") | ||
| print(f" 📊 Embedding shape: {len(embeddings)} texts, {len(embeddings[0])} dimensions") | ||
| print(f" 📊 First 5 values: {embeddings[0][:5]}") | ||
|
|
||
| # Verify dimensions | ||
| if len(embeddings[0]) == 768: | ||
| print(" ✅ Dimensions match expected (768)") | ||
| else: | ||
| print(f" ⚠️ Dimensions mismatch: got {len(embeddings[0])}, expected 768") | ||
|
|
||
| except KeyError as e: | ||
| print(f" ❌ KeyError (this is the bug we're fixing): {e}") | ||
| sys.exit(1) | ||
| except Exception as e: | ||
| print(f" ❌ Failed to generate embedding: {type(e).__name__}: {e}") | ||
| sys.exit(1) | ||
|
|
||
| # Test multiple texts | ||
| print("\n3. Testing multiple text embeddings...") | ||
| test_texts_multiple = [ | ||
| "Hello world", | ||
| "Machine learning is fascinating", | ||
| "Ollama embeddings work great" | ||
| ] | ||
|
|
||
| try: | ||
| embeddings = await engine.embed_text(test_texts_multiple) | ||
| print(" ✅ Multiple embeddings generated successfully") | ||
| print(f" 📊 Generated {len(embeddings)} embeddings") | ||
| for i, emb in enumerate(embeddings): | ||
| print(f" 📊 Text {i+1}: {len(emb)} dimensions, first 3 values: {emb[:3]}") | ||
|
|
||
| except Exception as e: | ||
| print(f" ❌ Failed to generate embeddings: {type(e).__name__}: {e}") | ||
| sys.exit(1) | ||
|
|
||
| # Success! | ||
| print("\n" + "=" * 80) | ||
| print("✅ ALL TESTS PASSED!") | ||
| print("=" * 80) | ||
| print("\nThe OllamaEmbeddingEngine fix is working correctly!") | ||
| print("- Handles 'embedding' (singular) response from Ollama API") | ||
| print("- Generates embeddings successfully") | ||
| print("- Correct dimensions (768 for nomic-embed-text)") | ||
| print("\n✅ Ready to submit PR!") | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| asyncio.run(test_ollama_embedding()) | ||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Critical: Hardcoded private IP address will break CI/CD and other environments.
The endpoint
"http://10.0.10.9:11434/api/embeddings"is a private network address specific to your local setup. This test will fail in:Apply this diff to make the endpoint configurable:
Add this import at the top:
import asyncio import sys +import os from cognee.infrastructure.databases.vector.embeddings.OllamaEmbeddingEngine import (This allows customization via environment variables while defaulting to localhost.
🤖 Prompt for AI Agents