-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Fix Ollama embedding configurator to respect API_BASE environment variable #3100
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
base: main
Are you sure you want to change the base?
Conversation
…iable - Add support for API_BASE environment variable in _configure_ollama method - Implement priority order: url config > api_base config > API_BASE env var > default - Handle URL construction with proper /api/embeddings endpoint appending - Maintain backward compatibility with existing url parameter - Add comprehensive tests covering all configuration scenarios - Fixes issue #3099: Ollama integration problems with API_BASE configuration The fix ensures consistent behavior with other embedding providers and allows users to configure Ollama embeddings using environment variables for better deployment flexibility. Co-Authored-By: João <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Disclaimer: This review was made by a crew of AI Agents. Code Review Comment for Ollama Embedding Configurator ChangesOverviewThe recent changes made to the Code Quality Findings1. embedding_configurator.pyPositive Aspects
Recommendations
2. test_ollama_embedding_configurator.pyPositive Aspects
Recommendations
Example for Testing Invalid URLs@pytest.mark.error_handling
class TestOllamaErrorHandling:
@pytest.mark.parametrize("invalid_url", [
"not-a-url",
"ftp://invalid-scheme",
"http://",
])
def test_invalid_url_raises_error(self, invalid_url):
config = {
"provider": "ollama",
"config": {
"model": "llama2",
"url": invalid_url
}
}
with pytest.raises(ValueError, match="Invalid Ollama API URL"):
self.configurator.configure_embedder(config) Impact AnalysisThese contributions to both the configurator and its testing suite represent a significant step forward in making the Ollama embedding service more robust and user-friendly. Nevertheless, the implementation would be substantially fortified with enhanced error handling and comprehensive test coverage. The pull request is generally constructive and moves towards improved reliability. It is recommended to address the outlined issues and enhancements before final merging. The strengthened error-handling components would notably improve the codebase's readiness for production deployment. Feel free to merge after addressing the suggested improvements! |
… and edge case tests - Add _validate_url() helper function with proper URL validation using urllib.parse - Add _construct_embeddings_url() helper function to refactor URL construction logic - Add comprehensive error handling with ValueError for invalid URLs - Fix test mocking to use correct chromadb import path - Add edge case tests for invalid URLs with pytest markers - Organize tests with @pytest.mark.url_configuration and @pytest.mark.error_handling - Remove unused imports (pytest, MagicMock) to fix lint issues This addresses all suggestions from joaomdmoura's AI review while maintaining backward compatibility. Co-Authored-By: João <[email protected]>
…dings - Update _validate_url() to restrict schemes to 'http' and 'https' only - Fixes CI test failures for ftp://invalid-scheme test cases - Maintains security by preventing non-web protocols Co-Authored-By: João <[email protected]>
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.
add docs about that also weel
def _construct_embeddings_url(base_url): | ||
"""Construct the full embeddings URL from a base URL.""" | ||
if not base_url: | ||
return "http://localhost:11434/api/embeddings" | ||
|
||
base_url = base_url.rstrip('/') | ||
return f"{base_url}/api/embeddings" if not base_url.endswith('/api/embeddings') else base_url |
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.
you are assuming that all embeddings providers's url has the same format. It might not be totally truth.
I think we don't need this method, please drop it and then use the provided byt config or env var
…d URLs as-is - Remove _construct_embeddings_url() method as requested by lucasgomide - Use URLs exactly as provided by config or environment variable - Add comprehensive documentation about API_BASE environment variable support - Maintain URL validation for security - Simplify implementation to avoid assumptions about URL formats across providers Co-Authored-By: João <[email protected]>
- Update test expectations to provide complete URLs with /api/embeddings endpoint - Remove assumptions about automatic URL construction per lucasgomide's feedback - Tests now reflect the new behavior where URLs are used exactly as provided - Maintains backward compatibility for users who provide complete URLs Co-Authored-By: João <[email protected]>
- Update test inputs to provide complete URLs with /api/embeddings endpoint - Remove tests for automatic URL construction that no longer exists - Tests now reflect the requirement for users to provide complete URLs - Addresses CI failures after removing _construct_embeddings_url helper Co-Authored-By: João <[email protected]>
Fix Ollama Embedding Configuration: Support API_BASE Environment Variable
Summary
This PR fixes issue #3099 by implementing proper support for the
API_BASE
environment variable in Ollama embedding configuration. The implementation provides a flexible, priority-based URL resolution system while addressing GitHub review feedback to use URLs exactly as provided.Key Changes:
API_BASE
environment variable in Ollama embedding configurationurl
config >api_base
config >API_BASE
env var > defaultBreaking Change Note: The URL handling behavior has changed - URLs are now used exactly as provided without automatic
/api/embeddings
endpoint appending.Review & Testing Checklist for Human
Risk Level: 🟡 Medium - Significant behavior changes with potential backward compatibility impact
Recommended Test Plan:
/api/embeddings
endpoint)Diagram
Notes