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

Fix #2390: Add databricks-sdk dependency to tools extra #2391

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

Conversation

devin-ai-integration[bot]
Copy link
Contributor

This PR fixes issue #2390 by adding the databricks-sdk dependency to the tools extra in pyproject.toml.

Problem

When users install CrewAI with the tools extra using pip install "crewai[tools]", they encounter an error when trying to import the databricks_query_tool due to a missing dependency (databricks-sdk).

Solution

Added the databricks-sdk dependency directly to the tools extra in CrewAI's pyproject.toml. This ensures that when users install with pip install "crewai[tools]", they get all the necessary dependencies, including databricks-sdk.

Testing

Added a test that verifies the databricks-sdk can be imported without errors when the tools extra is installed.

Link to Devin run: https://app.devin.ai/sessions/4fdd8ede2653463e871d2e23c7c8a0d4
Requested by: user

Co-Authored-By: Joe Moura <joao@crewai.com>
Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add "(aside)" to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Co-Authored-By: Joe Moura <joao@crewai.com>
@joaomdmoura
Copy link
Collaborator

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

Code Review Report for PR #2391

Overview

This pull request addresses issue #2390 by adding the databricks-sdk dependency to the tools extra in pyproject.toml, and includes a corresponding test file, test_databricks_import.py, to verify the import functionality of the new dependency.

File-by-File Analysis

1. pyproject.toml

Changes

-tools = ["crewai-tools>=0.37.0"]
+tools = ["crewai-tools>=0.37.0", "databricks-sdk>=0.46.0"]

Observations

  • ✅ The new dependency for databricks-sdk is well-structured, with versions appropriately pinned.
  • ✅ The change adheres to the established format for dependencies.

Suggestions

  1. It would be helpful to add a comment describing the necessity of the databricks-sdk in the tools extra:
[project.optional-dependencies]
tools = [
    "crewai-tools>=0.37.0",
    "databricks-sdk>=0.46.0"  # Required for Databricks query tool functionality
]

2. tests/tools/test_databricks_import.py

Observations

  • ✅ The test file contains a helpful docstring explaining its purpose.
  • ✅ Utilizes pytest effectively to ensure proper testing.
  • ✅ The basic import verification is correctly implemented.

Issues and Improvements

  1. Test Isolation: The current structure may benefit from improved isolation and setup:
import pytest

@pytest.mark.dependency
def test_databricks_sdk_import():
    """Ensure that databricks-sdk can be imported without errors."""
    try:
        import databricks.sdk
        assert databricks.sdk.__version__ >= "0.46.0", "Databricks SDK version is too old"
    except ImportError as e:
        pytest.fail(f"Failed to import databricks.sdk: {e}")
  1. Additional Test Coverage: More comprehensive tests would strengthen overall quality:
@pytest.mark.dependency(depends=["test_databricks_sdk_import"])
def test_databricks_core_functionality():
    """Test core functionality of the Databricks SDK."""
    from databricks.sdk import WorkspaceClient
    
    assert hasattr(WorkspaceClient, 'sql'), "SQL functionality not available"

General Recommendations

  1. Documentation Updates

    • Update the project documentation to mention the new Databricks integration.
    • Include a note in the changelog regarding this dependency addition.
  2. CI/CD Considerations

    • Ensure that CI pipelines incorporate tests with the tools extra installed.
    • Consider a test matrix to verify compatibility with different versions of databricks-sdk.
  3. Version Management

    • Consider specifying an upper limit to the databricks-sdk version to prevent future compatibility issues:
"databricks-sdk>=0.46.0,<1.0.0"

Security Considerations

  • ✅ The specified version (0.46.0) currently has no known vulnerabilities.
  • ⚠️ Regularly review dependency versions for security updates.

Conclusion

The changes made are well-structured and support new functionality effectively. By implementing the recommended improvements, particularly around enhancing test coverage and updating documentation, this PR can significantly contribute to the project's dependability and maintainability.

The PR is approvable, contingent upon addressing the following:

  1. Ensuring version verification in the import test.
  2. Adding more comprehensive coverage for core functionality.
  3. Considering upper bounds for dependency versions to maintain compatibility.

devin-ai-integration bot and others added 2 commits March 17, 2025 18:24
… tests for databricks-sdk

Co-Authored-By: Joe Moura <joao@crewai.com>
Co-Authored-By: Joe Moura <joao@crewai.com>
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.

1 participant