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

feat: add plugin tests #1096

Merged
merged 17 commits into from
Jan 13, 2025
Merged
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/common.yml
Original file line number Diff line number Diff line change
@@ -102,6 +102,10 @@ jobs:
export E2B_API_KEY=${{ secrets.E2B_API_KEY_STAGING }}

tox -e test -- -m 'not e2e and not swe'
- name: Plugin tests
if: matrix.python-version == '3.10'
run: |
Comment on lines +105 to +107

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary conditional for uploading test results. It should always upload regardless of the python version.

📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
- name: Plugin tests
if: matrix.python-version == '3.10'
run: |
name: Upload test results to Codecov
uses: codecov/test-results-action@v1

tox -e plugins
Comment on lines +105 to +108

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Conditional for plugin tests is incorrect. It should execute when matrix.python-version == '3.9'.

📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
- name: Plugin tests
if: matrix.python-version == '3.10'
run: |
tox -e plugins
- name: Plugin tests
if: matrix.python-version == '3.9'
run: |
tox -e plugins

- if: matrix.python-version == '3.10'
name: Upload test results to Codecov
uses: codecov/test-results-action@v1
4 changes: 3 additions & 1 deletion python/composio/tools/local/codeanalysis/embedder.py
Original file line number Diff line number Diff line change
@@ -2,7 +2,6 @@
from typing import Any, Dict, List

from deeplake.core.vectorstore.deeplake_vectorstore import DeepLakeVectorStore
from sentence_transformers import SentenceTransformer

from composio.tools.local.codeanalysis.constants import (
CODE_MAP_CACHE,
angrybayblade marked this conversation as resolved.
Show resolved Hide resolved
angrybayblade marked this conversation as resolved.
Show resolved Hide resolved
angrybayblade marked this conversation as resolved.
Show resolved Hide resolved
angrybayblade marked this conversation as resolved.
Show resolved Hide resolved
angrybayblade marked this conversation as resolved.
Show resolved Hide resolved
angrybayblade marked this conversation as resolved.
Show resolved Hide resolved
@@ -49,6 +48,9 @@ def get_vector_store(repo_name: str, overwrite: bool = True) -> DeepLakeVectorSt

class Embedding:
def __init__(self):
# pylint: disable=import-outside-toplevel
from sentence_transformers import SentenceTransformer

angrybayblade marked this conversation as resolved.
Show resolved Hide resolved
self.model = SentenceTransformer(EMBEDDER)

def compute(self, texts: List[str]) -> List[List[float]]:
angrybayblade marked this conversation as resolved.
Show resolved Hide resolved
angrybayblade marked this conversation as resolved.
Show resolved Hide resolved
angrybayblade marked this conversation as resolved.
Show resolved Hide resolved
angrybayblade marked this conversation as resolved.
Show resolved Hide resolved
angrybayblade marked this conversation as resolved.
Show resolved Hide resolved
angrybayblade marked this conversation as resolved.
Show resolved Hide resolved
angrybayblade marked this conversation as resolved.
Show resolved Hide resolved
58 changes: 58 additions & 0 deletions python/tests/test_tools/test_plugins.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import composio_openai
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider grouping related imports together and removing unused ones:

from composio import Action, App  # Action is unused
from composio.client.enums.base import ActionData  # ActionData is unused
from composio.tools.local.base import action

# Group plugin imports
import composio_openai
import composio_crewai  # Unused
import composio_langchain  # Unused
import composio_llamaindex  # Unused

This will make the imports cleaner and more maintainable.



def test_openai_toolset():
toolset = composio_openai.ComposioToolSet()

@composio_openai.action(toolname="my_tool")
def create_draft(
message_body: str,
thread_id: str | None = None,
) -> dict:
"""
Create a draft reply to a specific Gmail thread

:param thread_id: The ID of the thread to which the reply is to be drafted
:param message_body: The content of the draft reply
:return draft: The created draft details
"""
_ = message_body, thread_id
return {}

tools = toolset.get_tools(actions=[create_draft])
assert tools == [
{
"function": {
"description": "Create a draft reply to a specific Gmail thread",
"name": "MYTOOL_CREATE_DRAFT",
"parameters": {
"properties": {
"message_body": {
"description": "The content of the draft reply. Please provide a "
"value of type string. This parameter is required.",
"title": "Message Body",
"type": "string",
},
Comment on lines +35 to +48

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug Fix: The thread_id parameter in the create_draft function has an inconsistent type definition. It is defined with both a type of string and an anyOf clause that allows for string or null. This redundancy can lead to confusion and potential errors in type validation.

🔧 Suggested Code Diff:
- "type": "string",
  "anyOf": [
      {"type": "string"},
      {"type": "null"},
  ],
+ "type": ["string", "null"],
📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
tools = toolset.get_tools(actions=[create_draft])
assert tools == [
{
"function": {
"description": "Create a draft reply to a specific Gmail thread",
"name": "MYTOOL_CREATE_DRAFT",
"parameters": {
"properties": {
"message_body": {
"description": "The content of the draft reply. Please provide a "
"value of type string. This parameter is required.",
"title": "Message Body",
"type": "string",
},
tools = toolset.get_tools(actions=[create_draft])
assert tools == [
{
"function": {
"description": "Create a draft reply to a specific Gmail thread",
"name": "MYTOOL_CREATE_DRAFT",
"parameters": {
"properties": {
"message_body": {
"description": "The content of the draft reply. Please provide a "
"value of type string. This parameter is required.",
"title": "Message Body",
"type": "string",
},
"thread_id": {
"description": "The ID of the Gmail thread. This parameter can be a string or null.",
"title": "Thread ID",
"type": ["string", "null"],
}
}
}
}
}
]

"thread_id": {
"description": "The ID of the thread to which the reply is to be "
"drafted. Please provide a value of type string.",
"title": "Thread Id",
# TODO: this seems wrong, why both type and anyof
"type": "string",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of both type and anyOf for thread_id is inconsistent. Consider removing the type key or ensure it aligns with the anyOf specification.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The redundant type definition here could be simplified. Since thread_id is an optional string parameter, we could either:

  1. Remove the type: string and keep only the anyOf definition, or
  2. Use type: ["string", "null"] instead

The current implementation with both type and anyOf is redundant and could cause confusion in schema validation.

"default": None,
"anyOf": [
{"type": "string"},
{"type": "null"},
],
},
},
"required": [
"message_body",
],
"title": "CreateDraftRequest",
"type": "object",
},
},
"type": "function",
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Bug Fix:

Redundant Type Definition in 'thread_id' Field
The current schema for the 'thread_id' field uses both 'type' and 'anyOf', which is redundant and can lead to confusion. The 'anyOf' construct already specifies that the field can be either a string or null, making the 'type' declaration unnecessary. Removing the 'type' field will simplify the schema and prevent potential validation issues. This change will ensure clarity and maintainability of the code. 🛠️

🔧 Suggested Code Diff:
- "type": "string",
  "anyOf": [
      {"type": "string"},
      {"type": "null"},
  ],
📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
"drafted. Please provide a value of type string.",
"title": "Thread Id",
# TODO: this seems wrong, why both type and anyof
"type": "string",
"default": None,
"anyOf": [
{"type": "string"},
{"type": "null"},
],
},
},
"required": [
"message_body",
],
"title": "CreateDraftRequest",
"type": "object",
},
},
"type": "function",
},
{
"thread_id": {
"description": "The ID of the thread to which the reply is to be drafted. Please provide a value of type string.",
"title": "Thread Id",
"default": None,
"anyOf": [
{"type": "string"},
{"type": "null"}
]
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Bug Fix:

Redundant Schema Definition in 'CreateDraftRequest'
The current schema definition for 'CreateDraftRequest' includes both a 'type' field and an 'anyOf' field, which is redundant and can lead to confusion in validation logic. If the intention is to allow both strings and null values, the 'type' field should be removed, and 'anyOf' should be used exclusively. Alternatively, if only strings are allowed, the 'anyOf' field should be removed. This will ensure clarity and correctness in the schema definition.

🔧 Suggested Code Diff:
- "type": "string",
+ // Remove the 'type' field if allowing both strings and null values
📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
"drafted. Please provide a value of type string.",
"title": "Thread Id",
# TODO: this seems wrong, why both type and anyof
"type": "string",
"default": None,
"anyOf": [
{"type": "string"},
{"type": "null"},
],
},
},
"required": [
"message_body",
],
"title": "CreateDraftRequest",
"type": "object",
},
},
"type": "function",
},
{
"CreateDraftRequest": {
"type": "object",
"properties": {
"message_body": {
"anyOf": [
{"type": "string"},
{"type": "null"}
],
"default": None
}
},
"required": [
"message_body"
],
"title": "CreateDraftRequest"
}
}

tushar-composio marked this conversation as resolved.
Show resolved Hide resolved

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential Issue: The thread_id parameter in the create_draft function has a redundant type definition. It specifies both a type of string and an anyOf that allows for string or null. This redundancy can lead to confusion and inconsistent type validation. It's advisable to remove the type field and rely solely on anyOf for clarity and consistency.

🔧 Suggested Code Diff:
- "type": "string",
  "anyOf": [
      {"type": "string"},
      {"type": "null"},
  ],
📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
"thread_id": {
"description": "The ID of the thread to which the reply is to be "
"drafted. Please provide a value of type string.",
"title": "Thread Id",
# TODO: this seems wrong, why both type and anyof
"type": "string",
"default": None,
"anyOf": [
{"type": "string"},
{"type": "null"},
],
},
},
"required": [
"message_body",
],
"title": "CreateDraftRequest",
"type": "object",
},
},
"type": "function",
},
{
"thread_id": {
"description": "The ID of the thread to which the reply is to be drafted. Please provide a value of type string.",
"title": "Thread Id",
"default": None,
"anyOf": [
{"type": "string"},
{"type": "null"},
],
},
},
"required": [
"message_body",
],
"title": "CreateDraftRequest",
"type": "object",
"type": "function",
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential Issue: The thread_id parameter in the create_draft function is defined with both a type of string and an anyOf condition allowing for string or null values. This redundancy can lead to inconsistent type checking and validation.

Suggested Action:

  • Remove the type specification and rely solely on the anyOf condition to clearly define the parameter's acceptable types.

This change will ensure consistent validation and improve code clarity. 🛠️

🔧 Suggested Code Diff:
- "type": "string",
  "anyOf": [
      {"type": "string"},
      {"type": "null"},
  ],
📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
"thread_id": {
"description": "The ID of the thread to which the reply is to be "
"drafted. Please provide a value of type string.",
"title": "Thread Id",
# TODO: this seems wrong, why both type and anyof
"type": "string",
"default": None,
"anyOf": [
{"type": "string"},
{"type": "null"},
],
},
},
"required": [
"message_body",
],
"title": "CreateDraftRequest",
"type": "object",
},
},
"type": "function",
},
{
"thread_id": {
"description": "The ID of the thread to which the reply is to be drafted. Please provide a value of type string.",
"title": "Thread Id",
"default": None,
"anyOf": [
{"type": "string"},
{"type": "null"}
]
}
}

]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor: The thread_id parameter in the create_draft function has an inconsistent type definition. It is defined with both type: string and anyOf containing type: string and type: null. This redundancy can lead to confusion and potential errors in type validation.

  • Actionable Suggestion: Remove the type: string line and rely on the anyOf construct to handle both string and null types, which is more explicit and clear.
🔧 Suggested Code Diff:
- "type": "string",
  "anyOf": [
      {"type": "string"},
      {"type": "null"},
  ],
📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
tools = toolset.get_tools(actions=[create_draft])
assert tools == [
{
"function": {
"description": "Create a draft reply to a specific Gmail thread",
"name": "MYTOOL_CREATE_DRAFT",
"parameters": {
"properties": {
"message_body": {
"description": "The content of the draft reply. Please provide a "
"value of type string. This parameter is required.",
"title": "Message Body",
"type": "string",
},
"thread_id": {
"description": "The ID of the thread to which the reply is to be "
"drafted. Please provide a value of type string.",
"title": "Thread Id",
# TODO: this seems wrong, why both type and anyof
"type": "string",
"default": None,
"anyOf": [
{"type": "string"},
{"type": "null"},
],
},
},
"required": [
"message_body",
],
"title": "CreateDraftRequest",
"type": "object",
},
},
"type": "function",
},
]
tools = toolset.get_tools(actions=[create_draft])
assert tools == [
{
"function": {
"description": "Create a draft reply to a specific Gmail thread",
"name": "MYTOOL_CREATE_DRAFT",
"parameters": {
"properties": {
"message_body": {
"description": "The content of the draft reply. Please provide a "
"value of type string. This parameter is required.",
"title": "Message Body",
"type": "string",
},
"thread_id": {
"description": "The ID of the thread to which the reply is to be "
"drafted. Please provide a value of type string.",
"title": "Thread Id",
"default": None,
"anyOf": [
{"type": "string"},
{"type": "null"},
],
},
},
"required": [
"message_body",
],
"title": "CreateDraftRequest",
"type": "object",
},
},
"type": "function",
},
]

30 changes: 21 additions & 9 deletions python/tox.ini
Original file line number Diff line number Diff line change
@@ -121,17 +121,29 @@ commands =

; TODO: Extract plugin tests separately
; Installing separately because of the dependency conflicts
uv pip install plugins/langchain --no-deps
uv pip install plugins/langchain

pytest -vvv -rfE --doctest-modules composio/ tests/ swe/tests --junitxml=junit.xml --cov=composio --cov=examples --cov=swe --cov-report=html --cov-report=xml --cov-report=term --cov-report=term-missing --cov-config=.coveragerc {posargs}
pytest -vvv -rfE --doctest-modules composio/ tests/ swe/tests --ignore tests/test_tools/test_plugins.py --junitxml=junit.xml --cov=composio --cov=examples --cov=swe --cov-report=html --cov-report=xml --cov-report=term --cov-report=term-missing --cov-config=.coveragerc {posargs}

; uv pip install plugins/autogen
; uv pip install plugins/claude
; uv pip install plugins/crew_ai
; uv pip install plugins/griptape
; uv pip install plugins/julep
; uv pip install plugins/lyzr
; uv pip install plugins/openai

Comment on lines 121 to +128

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential Issue:

Ensure Comprehensive Test Coverage for Excluded Test File
The recent change excludes tests/test_tools/test_plugins.py from the main test suite, which could lead to gaps in test coverage. It's crucial to ensure that this test file is included in a new dedicated job for plugin tests, especially under Python 3.10. This will help maintain comprehensive test coverage and prevent potential bugs from going unnoticed.

Actionable Steps:

  • Verify that tests/test_tools/test_plugins.py is included in a separate test job.
  • Ensure that the new job runs under Python 3.10 to maintain compatibility and coverage.
  • Regularly review test coverage reports to confirm no tests are omitted inadvertently.

Comment on lines 121 to +128

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential Issue: The removal of the --no-deps flag when installing the langchain plugin could lead to dependency conflicts. This flag prevents the installation of dependencies that might conflict with other packages in the project. Without it, there is a risk of installing incompatible versions of dependencies, which could affect the stability and functionality of the system.

🔧 Suggested Code Diff:
-    uv pip install plugins/langchain
+    uv pip install plugins/langchain --no-deps
📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
; TODO: Extract plugin tests separately
; Installing separately because of the dependency conflicts
uv pip install plugins/langchain --no-deps
uv pip install plugins/langchain
pytest -vvv -rfE --doctest-modules composio/ tests/ swe/tests --junitxml=junit.xml --cov=composio --cov=examples --cov=swe --cov-report=html --cov-report=xml --cov-report=term --cov-report=term-missing --cov-config=.coveragerc {posargs}
pytest -vvv -rfE --doctest-modules composio/ tests/ swe/tests --ignore tests/test_tools/test_plugins.py --junitxml=junit.xml --cov=composio --cov=examples --cov=swe --cov-report=html --cov-report=xml --cov-report=term --cov-report=term-missing --cov-config=.coveragerc {posargs}
; uv pip install plugins/autogen
; uv pip install plugins/claude
; uv pip install plugins/crew_ai
; uv pip install plugins/griptape
; uv pip install plugins/julep
; uv pip install plugins/lyzr
; uv pip install plugins/openai
uv pip install plugins/langchain --no-deps
pytest -vvv -rfE --doctest-modules composio/ tests/ swe/tests --ignore tests/test_tools/test_plugins.py --junitxml=junit.xml --cov=composio --cov=examples --cov=swe --cov-report=html --cov-report=xml --cov-report=term --cov-report=term-missing --cov-config=.coveragerc {posargs}

Comment on lines 121 to +128

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential Issue: The removal of the --no-deps flag when installing the langchain plugin could introduce dependency conflicts. This flag is typically used to prevent the installation of dependencies that might conflict with other packages in the environment. Reintroducing this flag or ensuring compatibility of dependencies is crucial to maintain a stable environment.

🔧 Suggested Code Diff:
-    uv pip install plugins/langchain
+    uv pip install plugins/langchain --no-deps
📝 Committable Code Suggestion

‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
; TODO: Extract plugin tests separately
; Installing separately because of the dependency conflicts
uv pip install plugins/langchain --no-deps
uv pip install plugins/langchain
pytest -vvv -rfE --doctest-modules composio/ tests/ swe/tests --junitxml=junit.xml --cov=composio --cov=examples --cov=swe --cov-report=html --cov-report=xml --cov-report=term --cov-report=term-missing --cov-config=.coveragerc {posargs}
pytest -vvv -rfE --doctest-modules composio/ tests/ swe/tests --ignore tests/test_tools/test_plugins.py --junitxml=junit.xml --cov=composio --cov=examples --cov=swe --cov-report=html --cov-report=xml --cov-report=term --cov-report=term-missing --cov-config=.coveragerc {posargs}
; uv pip install plugins/autogen
; uv pip install plugins/claude
; uv pip install plugins/crew_ai
; uv pip install plugins/griptape
; uv pip install plugins/julep
; uv pip install plugins/lyzr
; uv pip install plugins/openai
uv pip install plugins/langchain --no-deps
pytest -vvv -rfE --doctest-modules composio/ tests/ swe/tests --ignore tests/test_tools/test_plugins.py --junitxml=junit.xml --cov=composio --cov=examples --cov=swe --cov-report=html --cov-report=xml --cov-report=term --cov-report=term-missing --cov-config=.coveragerc {posargs}

Comment on lines 121 to +128

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential Issue: The recent changes in the tox.ini file involve the removal of several plugin installations and the exclusion of a specific test file. This could lead to potential issues:

  • Test Coverage Impact: By excluding tests/test_tools/test_plugins.py, you might miss out on testing critical functionalities related to plugins. Ensure that the exclusion is intentional and justified.
  • Plugin Dependency Management: The removal of --no-deps from the uv pip install plugins/langchain command could lead to dependency conflicts if langchain has dependencies that clash with other installed packages. Consider reinstating --no-deps if dependency conflicts were previously an issue.
  • Plugin Installation: The removal of other plugin installations might affect the functionality if those plugins are required for certain tests or features. Verify that these plugins are no longer needed or are being managed elsewhere.

Consider reviewing these changes to ensure they align with the overall project goals and do not inadvertently reduce test coverage or introduce dependency issues.


Comment on lines 121 to +128

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential Issue: The removal of the --no-deps flag from the uv pip install plugins/langchain command could lead to dependency conflicts if langchain has dependencies that conflict with other installed packages. This might cause issues during runtime or testing.

Additionally, the removal of several plugin installations and the modification of the pytest command to ignore specific tests could lead to reduced test coverage. This might result in undetected issues in the plugins that are no longer being tested.

Recommendations:

  • Dependency Management: Consider re-adding the --no-deps flag or ensuring that dependencies are managed elsewhere to prevent conflicts.
  • Test Coverage: Evaluate the impact of ignoring certain tests and ensure that critical functionality is still covered. If necessary, reintroduce tests for essential plugins to maintain comprehensive test coverage.

[testenv:plugins]
setenv =
CI={env:CI}
COMPOSIO_API_KEY={env:COMPOSIO_API_KEY}
COMPOSIO_BASE_URL={env:COMPOSIO_BASE_URL}
deps =
.
plugins/autogen
plugins/claude
plugins/crew_ai
plugins/griptape
plugins/julep
plugins/langchain
plugins/langgraph
plugins/llamaindex
plugins/openai
commands =
pytest -vv tests/test_tools/test_plugins.py {posargs}

; Linter config

Loading