Skip to content
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

chore: modify ollama default env url from API_URL to BASE_URL #1972

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vicradon
Copy link

Issue:

  • When Ollama is set as the model, it fails because "api_base" is not defined in the LLM class (llm.py)

Solution:

  • Set the default from API_BASE to BASE_URL because BASE_URL is already in the LLM class

Result:

Ollama models now work without errors

@joaomdmoura
Copy link
Collaborator

Disclaimer: This review was made by a crew of AI Agents.

Code Review Comment for PR #1972

Overview

This pull request addresses a critical naming consistency by modifying the Ollama default environment URL variable from API_BASE to BASE_URL in the src/crewai/cli/constants.py file. This change aligns the codebase with common standards and facilitates improved clarity in configuration management.

Code Quality Findings

  • Consistency: The change enhances uniformity across the codebase, as BASE_URL is a more conventional term. It helps prevent confusion, particularly for new developers or when integrating with other services.
  • Impact: While the modification enhances clarity, it poses a potential breaking change. Existing implementations that rely on API_BASE will require immediate updates, which could lead to some integration issues.

Specific Code Improvements

  1. Migration Notice: It's crucial to indicate the deprecation of the old key to guide current users. Consider adding a comment above the Ollama section in constants.py:

    # TODO: API_BASE is deprecated as of version X.X.X, use BASE_URL instead
  2. Validation Flag: Introducing a URL validation mechanism would improve error handling and configuration integrity. Add a flag for validation along with the base URL:

    "ollama": [
        {
            "default": True,
            "BASE_URL": "http://localhost:11434",
            "validate_url": True,  # Add URL validation flag
        }
    ],
  3. Type Hints: Including type hints for clarity and better tool support enhances overall code quality. Update the definition of PROVIDERS as follows:

    from typing import List, Dict, Any
    
    PROVIDERS: Dict[str, List[Dict[str, Any]]] = {
        "ollama": [
            {
                "default": True,
                "BASE_URL": "http://localhost:11434",
            }
        ],
        # ... other providers
    }

Historical Context

While it is challenging to provide specific historical examples here without access to previous PRs impacting constants.py, it is vital to review related historical PRs that may contain previous discussions on naming conventions, breaking changes, or configuration management practices.

Documentation Implications

  • Any documentation referencing the Ollama configuration must be updated to reflect the change from API_BASE to BASE_URL.
  • Consider providing a migration guide for users transitioning from the old version, outlining the necessary updates they must implement in their configurations.

Testing Recommendations

  • Unit Tests: Ensure that unit tests are written to validate that the new configuration behaves as expected.
  • Backward Compatibility: Adding tests that validate existing functionality with the old API_BASE would help catch any issues that arise from this change.

Security Considerations

  • Although the default URL remains hardcoded to localhost, if this configuration is utilized in production, it's essential to consider implementing HTTPS support to ensure the security of transmission.

Conclusion

This code change positively impacts naming consistency and improves the codebase's quality. However, given its potential breaking nature, it is critical to enhance documentation, provide migration support, and incorporate necessary tests before merging.

This change should be merged after:

  1. Comprehensive updates to documentation are made.
  2. Deprecation warnings for the API_BASE key are implemented.
  3. Relevant tests are thoroughly reviewed and updated as necessary.
  4. Consideration is given to the suggested code improvements above.

This review should facilitate a smooth integration of the changes while ensuring all necessary considerations are addressed.

@callmeBalloch
Copy link

When will this be merged? :)

@lorenzejay
Copy link
Collaborator

lorenzejay commented Jan 29, 2025

sounds good!

@lorenzejay lorenzejay requested a review from bhancockio January 29, 2025 22:13
@vicradon
Copy link
Author

vicradon commented Jan 31, 2025

@bhancockio, could you please review and merge? I'm about to use CrewAI in production with Ollama, and this is needed as we can't manually make this update in production.

Please 🙏🏽

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants