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

feat: add plugin tests #1096

merged 17 commits into from
Jan 13, 2025

Conversation

tushar-composio
Copy link
Contributor

@tushar-composio tushar-composio commented Dec 27, 2024

Important

Adds plugin tests to CI workflow and separates them in tox configuration, with import adjustment in embedder.py.

  • CI Workflow:
    • Adds a new job for plugin tests in .github/workflows/common.yml for Python 3.10.
  • Code Changes:
    • Moves import of SentenceTransformer inside Embedding.__init__() in embedder.py to comply with pylint.
  • Testing:
    • Adds test_plugins.py to test various toolsets like composio_openai, composio_crewai, etc.
    • Updates tox.ini to include a new testenv:plugins for running plugin tests separately.

This description was created by Ellipsis for 78fc5b1. It will automatically update as commits are pushed.

Copy link

vercel bot commented Dec 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
composio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 10, 2025 10:39am

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to cf4548c in 32 seconds

More details
  • Looked at 68 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 0 drafted comments based on config settings.

Workflow ID: wflow_CG7uX4Xy3xiluGET


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

"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.

"drafted. Please provide a value of type string.",
"title": "Thread Id",
# TODO: this seems wrong, why both type and anyof
"type": "string",
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.

from composio import Action, App
from composio.client.enums.base import ActionData
from composio.tools.local.base import action
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.

@shreysingla11
Copy link
Collaborator

Code Review Summary

The PR adds valuable test coverage for input serialization in the get_tools() method, particularly focusing on the OpenAI toolset implementation. The tests verify proper handling of:

  • Required and optional parameters
  • Parameter type definitions
  • Docstring parsing
  • Tool naming conventions

Suggestions for Improvement:

  1. Clean up imports by removing unused ones and grouping related imports
  2. Simplify the type definition for optional parameters to avoid redundancy
  3. Consider adding more test cases for:
    • Edge cases in parameter types
    • Error scenarios
    • Other runtime implementations beyond OpenAI

Overall, this is a good addition to the test suite that will help ensure consistent parameter handling across different runtime implementations. The code quality is good with room for minor improvements.

Comment on lines 38 to 51
"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",
"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 for 'thread_id'
The current schema definition for the 'thread_id' field uses both 'type' and 'anyOf', which is redundant and can lead to confusion in schema validation. To improve clarity and avoid potential validation errors, it's recommended to use only 'anyOf' if the intention is to allow both string and null values. This will streamline the schema and ensure consistent behavior.

🔧 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
"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",
"anyOf": [
{"type": "string"},
{"type": "null"},
],
},
{
"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 or null.",
"title": "Thread Id",
"anyOf": [
{"type": "string"},
{"type": "null"},
],
}

@tushar-composio tushar-composio changed the title chore: add tests for input serialization in get_tools() chore: add plugin tests Dec 31, 2024
@tushar-composio tushar-composio changed the title chore: add plugin tests feat: add plugin tests Dec 31, 2024
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on b3d0029 in 24 seconds

More details
  • Looked at 34 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. python/composio/tools/local/codeanalysis/embedder.py:51
  • Draft comment:
    Consider the impact of moving the 'SentenceTransformer' import inside the 'init' method. This can reduce initial load time but may delay instantiation.
  • Reason this comment was not posted:
    Confidence changes required: 33%
    The import statement for 'SentenceTransformer' was moved inside the 'init' method of the 'Embedding' class. This change is beneficial for reducing the initial load time and memory usage if the 'Embedding' class is not instantiated. However, it should be noted that this can lead to a slight delay when the class is first instantiated, as the import will occur at that time.
2. python/tests/test_tools/test_plugins.py:46
  • Draft comment:
    Remove 'type' when using 'anyOf' to avoid redundancy. Applicable to 'thread_id' and similar cases.
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_b80AZGHzjVoSl00g


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Comment on lines 43 to 62
"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",
},

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"}
]
}
}

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on ba782b7 in 14 seconds

More details
  • Looked at 49 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 2 drafted comments based on config settings.
1. .github/workflows/common.yml:105
  • Draft comment:
    Consider moving the condition if: matrix.python-version == '3.10' to the job level for consistency with other jobs.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The addition of the plugin tests in the workflow is a good step, but the condition for running them is not consistent with the rest of the workflow. The condition should be placed on the job level, not the step level, for consistency and clarity.
2. python/tox.ini:124
  • Draft comment:
    The installation of plugins/langchain is redundant here as it is already included in the [testenv:plugins] section. Consider removing it to avoid unnecessary installations.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The tox.ini file has a redundant installation of plugins/langchain in the [testenv:test] section. This is unnecessary because the [testenv:plugins] section already handles plugin installations.

Workflow ID: wflow_PeG3YqkBeAR35hWk


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Comment on lines 43 to 62
"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",
},

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"
}
}

Comment on lines 121 to +128

; 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

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.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on e93a7f6 in 10 seconds

More details
  • Looked at 36 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/tests/test_tools/test_plugins.py:1
  • Draft comment:
    Remove unused imports to clean up the code. composio_crewai, composio_langchain, and composio_llamaindex are not used in this file.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The import statements for unused modules in test_plugins.py should be removed to clean up the code.

Workflow ID: wflow_Qj9d71JEQRYgNIkg


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 7425b2f in 7 seconds

More details
  • Looked at 15 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/tox.ini:134
  • Draft comment:
    Combine plugin installations into a single command to improve performance and reduce redundancy.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The tox.ini file has been updated to include environment variables for the plugins test environment, which is a good practice for managing configurations. However, the deps section can be optimized by combining the plugin installations into a single command to improve performance.

Workflow ID: wflow_c8wdNMY7z9PIs7EG


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Comment on lines +314 to +336
assert tool.args_schema.model_json_schema() == {
"properties": {
"message_body": {
"description": "The content of the draft reply. Please provide a value of type "
"string. This parameter is required.",
"examples": [],
"title": "Message Body",
"type": "string",
},
"thread": {
"default": None,
"description": "The thread to which the reply is to be drafted",
"examples": [],
"title": "Thread",
# TODO: Thread's properties should be present in the schema
"type": "object",
},
},
"required": ["message_body"],
# TODO: title should be MYTOOL_CREATE_DRAFT
"title": "CreateDraftRequest",
"type": "object",
}

Choose a reason for hiding this comment

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

Potential Issue: ### Issues Identified:

  1. Naming Consistency: The title in the JSON schema is inconsistent with the tool name. The TODO comment indicates that the title should be MYTOOL_CREATE_DRAFT, but it is currently CreateDraftRequest. This inconsistency can lead to confusion and should be addressed across the codebase.

  2. Incomplete Schema Definition: The thread property is defined as an object type without specifying its internal properties. This can lead to ambiguity in how the thread data is structured and used, potentially causing runtime errors or misinterpretations.

  3. Redundant Descriptions: The description for the action and text properties in the second tool's schema is excessively repetitive, which can clutter the code and make it harder to maintain.

Suggested Actions:

  • Update Naming: Ensure that the title in the JSON schema matches the tool name consistently across all test functions.
  • Define Schema Properties: Specify the properties of the thread object to ensure clarity and consistency in data handling.
  • Simplify Descriptions: Remove redundant phrases in the property descriptions to improve readability and maintainability.

Impact:

  • Consistency: Addressing these issues will improve the consistency and maintainability of the codebase.
  • Clarity: Clearer schema definitions will enhance understanding and reduce potential errors.
🔧 Suggested Code Diff:
- "title": "CreateDraftRequest",
+ "title": "MYTOOL_CREATE_DRAFT",

- "description": "The action to perform on the computer. Please provide a value of type string. This parameter is required. Please provide a value of type string. This parameter is required. Please provide a value of type string. This parameter is required. Please provide a value of type string. This parameter is required.",
+ "description": "The action to perform on the computer. This parameter is required.",

- "description": "Text to type or key sequence to press. Please provide a value of type string. Please provide a value of type string. Please provide a value of type string. Please provide a value of type string.",
+ "description": "Text to type or key sequence to press."
📝 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
assert tool.args_schema.model_json_schema() == {
"properties": {
"message_body": {
"description": "The content of the draft reply. Please provide a value of type "
"string. This parameter is required.",
"examples": [],
"title": "Message Body",
"type": "string",
},
"thread": {
"default": None,
"description": "The thread to which the reply is to be drafted",
"examples": [],
"title": "Thread",
# TODO: Thread's properties should be present in the schema
"type": "object",
},
},
"required": ["message_body"],
# TODO: title should be MYTOOL_CREATE_DRAFT
"title": "CreateDraftRequest",
"type": "object",
}
assert tool.args_schema.model_json_schema() == {
"properties": {
"message_body": {
"description": "The content of the draft reply. Please provide a value of type string. This parameter is required.",
"examples": [],
"title": "Message Body",
"type": "string",
},
"thread": {
"default": None,
"description": "The thread to which the reply is to be drafted",
"examples": [],
"title": "Thread",
"type": "object",
"properties": { # Define the properties of the thread object
"id": {
"type": "string",
"description": "Unique identifier for the thread."
},
"subject": {
"type": "string",
"description": "Subject of the thread."
}
}
},
},
"required": ["message_body"],
"title": "MYTOOL_CREATE_DRAFT",
"type": "object",
}

Comment on lines 121 to +128

; 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

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}

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 05944dd in 12 seconds

More details
  • Looked at 15 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/tox.ini:174
  • Draft comment:
    Adding tests/**:E501 to per-file-ignores allows long lines in test files, which is a common practice to improve readability of test cases. This change is appropriate.
  • Reason this comment was not posted:
    Confidence changes required: 10%
    The addition of tests/**:E501 to the per-file-ignores in the flake8 configuration allows long lines in test files, which is a common practice to improve readability of test cases. This change is appropriate and aligns with typical practices.

Workflow ID: wflow_vULOZqK2x32bCzR0


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 67ce449 in 12 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/tox.ini:146
  • Draft comment:
    The addition of 'pytest' here is redundant as it is already included in the 'test' environment dependencies. Consider removing it to avoid duplication.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The addition of 'pytest' to the dependencies in the plugins test environment is redundant since 'pytest' is already specified in the 'test' environment. This could lead to unnecessary duplication and potential version conflicts.

Workflow ID: wflow_Rr3p7W4Ek4sxg4j0


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Incremental review on 78fc5b1 in 13 seconds

More details
  • Looked at 12 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. python/tox.ini:146
  • Draft comment:
    Adding 'pip' to the dependencies list is unnecessary and could lead to confusion. Consider removing it.
  • Reason this comment was not posted:
    Comment did not seem useful.

Workflow ID: wflow_VMbANio2GL5DEfUP


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Comment on lines +158 to +179
assert tool.args_schema.model_json_schema() == {
"properties": {
"message_body": {
"description": "The content of the draft reply. Please provide a value of type "
"string. This parameter is required.",
"examples": [],
"title": "Message Body",
"type": "string",
},
"thread": {
"default": None,
"description": "The thread to which the reply is to be drafted",
"examples": [],
"title": "Thread",
# TODO: Thread's properties should be present in the schema
"type": "object",
},
},
"required": ["message_body"],
"title": "CreateDraftRequest",
"type": "object",
}

Choose a reason for hiding this comment

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

Potential Issue: The current schema definition for the thread property is inconsistent with the expectations set by the TODO comment. The thread is defined as a generic object without specific properties, which could lead to issues in code that relies on this schema.

Actionable Steps:

  • Define the specific properties for the thread object within the schema to ensure consistency and clarity.
  • Remove the TODO comment once the properties are defined to maintain code cleanliness.
🔧 Suggested Code Diff:
            "thread": {
                "default": None,
                "description": "The thread to which the reply is to be drafted",
                "examples": [],
                "title": "Thread",
                "type": "object",
+               "properties": {
+                   "id": {
+                       "type": "string",
+                       "description": "The unique identifier of the thread."
+                   },
+                   "subject": {
+                       "type": "string",
+                       "description": "The subject of the thread."
+                   }
+               }
            },
📝 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
assert tool.args_schema.model_json_schema() == {
"properties": {
"message_body": {
"description": "The content of the draft reply. Please provide a value of type "
"string. This parameter is required.",
"examples": [],
"title": "Message Body",
"type": "string",
},
"thread": {
"default": None,
"description": "The thread to which the reply is to be drafted",
"examples": [],
"title": "Thread",
# TODO: Thread's properties should be present in the schema
"type": "object",
},
},
"required": ["message_body"],
"title": "CreateDraftRequest",
"type": "object",
}
assert tool.args_schema.model_json_schema() == {
"properties": {
"message_body": {
"description": "The content of the draft reply. Please provide a value of type string. This parameter is required.",
"examples": [],
"title": "Message Body",
"type": "string",
},
"thread": {
"default": None,
"description": "The thread to which the reply is to be drafted",
"examples": [],
"title": "Thread",
"type": "object",
"properties": {
"id": {
"type": "string",
"description": "The unique identifier of the thread."
},
"subject": {
"type": "string",
"description": "The subject of the thread."
}
}
},
},
"required": ["message_body"],
"title": "CreateDraftRequest",
"type": "object",
}

Comment on lines +254 to +263

tools = toolset.get_tools(actions=[Action.ANTHROPIC_COMPUTER])
assert len(tools) == 1

tool = tools[0]
assert (
tool.description
== "A Tool That Allows Interaction With The Screen, Keyboard, And Mouse Of The Current Computer. Adapted For Mac Os And Linux."
)
assert tool.args_schema.model_json_schema() == {

Choose a reason for hiding this comment

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

Code Quality: The current test case for the ANTHROPIC_COMPUTER tool in the ComposioToolSet is missing assertions for the args_schema.model_json_schema(). This is crucial for ensuring the schema is correctly defined and validated. Additionally, the description for the action and text properties contains redundant phrases that should be cleaned up for clarity.

🔧 Suggested Code Diff:
    tools = toolset.get_tools(actions=[Action.ANTHROPIC_COMPUTER])
    assert len(tools) == 1

    tool = tools[0]
    assert (
        tool.description
        == "A Tool That Allows Interaction With The Screen, Keyboard, And Mouse Of The Current Computer. Adapted For Mac Os And Linux."
    )
+   assert tool.args_schema.model_json_schema() == {
+       "properties": {
+           "action": {
+               "description": "The action to perform on the computer. Please provide a value of type string. This parameter is required.",
+               "examples": [],
+               "title": "Action",
+               "type": "string",
+           },
+           "coordinate": {
+               "default": None,
+               "description": "X,Y coordinates for mouse actions",
+               "examples": [],
+               "items": {},
+               "title": "Coordinate",
+               "type": "array",
+           },
+           "text": {
+               "default": None,
+               "description": "Text to type or key sequence to press. Please provide a value of type string.",
+               "examples": [],
+               "title": "Text",
+               "type": "string",
+           },
+       },
+       "required": ["action"],
+       "title": "ComputerRequest",
+       "type": "object",
+   }
📝 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=[Action.ANTHROPIC_COMPUTER])
assert len(tools) == 1
tool = tools[0]
assert (
tool.description
== "A Tool That Allows Interaction With The Screen, Keyboard, And Mouse Of The Current Computer. Adapted For Mac Os And Linux."
)
assert tool.args_schema.model_json_schema() == {
tools = toolset.get_tools(actions=[Action.ANTHROPIC_COMPUTER])
assert len(tools) == 1
tool = tools[0]
assert (
tool.description
== "A Tool That Allows Interaction With The Screen, Keyboard, And Mouse Of The Current Computer. Adapted For Mac Os And Linux."
)
assert tool.args_schema.model_json_schema() == {
"properties": {
"action": {
"description": "The action to perform on the computer. Please provide a value of type string. This parameter is required.",
"examples": [],
"title": "Action",
"type": "string",
},
"coordinate": {
"default": None,
"description": "X,Y coordinates for mouse actions",
"examples": [],
"items": {},
"title": "Coordinate",
"type": "array",
},
"text": {
"default": None,
"description": "Text to type or key sequence to press. Please provide a value of type string.",
"examples": [],
"title": "Text",
"type": "string",
},
},
"required": ["action"],
"title": "ComputerRequest",
"type": "object",
}

Comment on lines +314 to +336
assert tool.args_schema.model_json_schema() == {
"properties": {
"message_body": {
"description": "The content of the draft reply. Please provide a value of type "
"string. This parameter is required.",
"examples": [],
"title": "Message Body",
"type": "string",
},
"thread": {
"default": None,
"description": "The thread to which the reply is to be drafted",
"examples": [],
"title": "Thread",
# TODO: Thread's properties should be present in the schema
"type": "object",
},
},
"required": ["message_body"],
# TODO: title should be MYTOOL_CREATE_DRAFT
"title": "CreateDraftRequest",
"type": "object",
}

Choose a reason for hiding this comment

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

Potential Issue: The current code has several issues that could affect its functionality:

  • Inconsistent Schema Titles: The title of the schema for the CreateDraftRequest should be consistent with the tool name MYTOOL_CREATE_DRAFT. This inconsistency can lead to confusion or errors in tool identification.

  • Missing Thread Properties: The thread object's properties are not defined in the schema. This omission could result in incomplete or incorrect handling of thread data, affecting the tool's functionality.

  • Redundant Descriptions: The repetition of phrases like "please provide" and "is required" in the description fields suggests a bug in the description generation logic. This redundancy can clutter the documentation and confuse users.

These issues should be addressed to ensure the tools function correctly and are user-friendly.

🔧 Suggested Code Diff:
-        "title": "CreateDraftRequest",
+        "title": "MYTOOL_CREATE_DRAFT",

-            "thread": {
+            "thread": {
+                "properties": {
+                    "id": {"type": "string", "description": "The unique identifier of the thread."},
+                    "subject": {"type": "string", "description": "The subject of the thread."}
+                },

-                "description": "The action to perform on the computer. Please provide a value of "
-                "type string. This parameter is required. Please provide a value "
-                "of type string. This parameter is required. Please provide a "
-                "value of type string. This parameter is required. Please provide "
-                "a value of type string. This parameter is required.",
+                "description": "The action to perform on the computer. This parameter is required.",

-                "description": "Text to type or key sequence to press. Please provide a value of "
-                "type string. Please provide a value of type string. Please "
-                "provide a value of type string. Please provide a value of type string.",
+                "description": "Text to type or key sequence to press.",
📝 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
assert tool.args_schema.model_json_schema() == {
"properties": {
"message_body": {
"description": "The content of the draft reply. Please provide a value of type "
"string. This parameter is required.",
"examples": [],
"title": "Message Body",
"type": "string",
},
"thread": {
"default": None,
"description": "The thread to which the reply is to be drafted",
"examples": [],
"title": "Thread",
# TODO: Thread's properties should be present in the schema
"type": "object",
},
},
"required": ["message_body"],
# TODO: title should be MYTOOL_CREATE_DRAFT
"title": "CreateDraftRequest",
"type": "object",
}
assert tool.args_schema.model_json_schema() == {
"properties": {
"message_body": {
"description": "The content of the draft reply. This parameter is required.",
"examples": [],
"title": "Message Body",
"type": "string",
},
"thread": {
"default": None,
"description": "The thread to which the reply is to be drafted",
"examples": [],
"title": "Thread",
"type": "object",
"properties": {
"id": {"type": "string", "description": "The unique identifier of the thread."},
"subject": {"type": "string", "description": "The subject of the thread."}
},
},
},
"required": ["message_body"],
"title": "MYTOOL_CREATE_DRAFT",
"type": "object",
}

Comment on lines 121 to +128

; 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

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 +16 to +20
@action(toolname="my_tool")
def create_draft(
message_body: str,
thread: Thread | None = None,
) -> dict:

Choose a reason for hiding this comment

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

Code Quality Improvement: The current implementation of the create_draft function is minimal and lacks functionality. While the function signature and docstring are clear, the function body does not perform any operations and simply returns an empty dictionary. This might be a placeholder, but it should be noted that the function does not fulfill its described purpose of creating a draft reply.

Suggestions:

  • Implement the logic to create a draft reply using the provided message_body and thread parameters.
  • Ensure that the function returns meaningful draft details as indicated in the docstring.
  • Consider adding error handling to manage cases where the thread is None or invalid.

This will enhance the function's utility and align it with its intended purpose. 🛠️


Comment on lines +21 to +28
"""
Create a draft reply to a specific Gmail thread

:param thread: 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

Choose a reason for hiding this comment

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

Code Quality Improvement: The current code snippet is well-structured and includes a clear docstring. However, the function implementation is incomplete as it only returns an empty dictionary. To improve code quality and functionality, consider implementing the logic to create a draft reply. This will ensure the function performs its intended purpose and can be tested effectively.

🔧 Suggested Code Diff:
@action(toolname="my_tool")
def create_draft(
    message_body: str,
    thread: Thread | None = None,
) -> dict:
    """
    Create a draft reply to a specific Gmail thread

    :param thread: 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
    """
    # Implement the logic to create a draft reply
    draft = {
        "thread_id": thread.id if thread else None,
        "message_body": message_body,
        "status": "draft_created"
    }
    return draft
📝 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
"""
Create a draft reply to a specific Gmail thread
:param thread: 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
@action(toolname="my_tool")
def create_draft(
message_body: str,
thread: Thread | None = None,
) -> dict:
"""
Create a draft reply to a specific Gmail thread
:param thread: 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
"""
if not message_body:
raise ValueError("Message body cannot be empty")
draft = {
"thread_id": thread.id if thread else None,
"message_body": message_body,
"status": "draft_created"
}
return draft

Comment on lines +145 to +157
def test_crewai_toolset() -> None:
toolset = composio_crewai.ComposioToolSet()

tools = toolset.get_tools(actions=[create_draft])
assert len(tools) == 1

tool = tools[0]
assert tool.name == "MYTOOL_CREATE_DRAFT"
assert (
tool.description
# TODO: Thread's properties should be present in the arguments
== "Tool Name: MYTOOL_CREATE_DRAFT\nTool Arguments: {'message_body': {'description': 'The content of the draft reply. Please provide a value of type string. This parameter is required.', 'type': 'str'}, 'thread': {'description': 'The thread to which the reply is to be drafted', 'type': 'dict'}}\nTool Description: Create a draft reply to a specific Gmail thread"
)

Choose a reason for hiding this comment

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

Potential Issue: The code contains a TODO comment indicating that the 'thread' object's properties should be detailed in both the arguments and schema. This suggests a potential inconsistency in the schema definition, which could lead to issues when the create_draft function is used elsewhere in the codebase.

Actionable Steps:

  • Expand the 'thread' object definition to include its properties in both the arguments and schema.
  • Ensure that the schema accurately reflects the expected structure of the 'thread' object to prevent runtime errors.

This change will improve the robustness and clarity of the tool's schema, ensuring that all necessary properties are defined and accessible.

🔧 Suggested Code Diff:
- 'thread': {'description': 'The thread to which the reply is to be drafted', 'type': 'dict'}
+ 'thread': {
+     'description': 'The thread to which the reply is to be drafted',
+     'type': 'object',
+     'properties': {
+         'id': {'type': 'string', 'description': 'The unique identifier of the thread'},
+         'subject': {'type': 'string', 'description': 'The subject of the thread'},
+         // Add other relevant properties here
+     }
+ }
📝 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
def test_crewai_toolset() -> None:
toolset = composio_crewai.ComposioToolSet()
tools = toolset.get_tools(actions=[create_draft])
assert len(tools) == 1
tool = tools[0]
assert tool.name == "MYTOOL_CREATE_DRAFT"
assert (
tool.description
# TODO: Thread's properties should be present in the arguments
== "Tool Name: MYTOOL_CREATE_DRAFT\nTool Arguments: {'message_body': {'description': 'The content of the draft reply. Please provide a value of type string. This parameter is required.', 'type': 'str'}, 'thread': {'description': 'The thread to which the reply is to be drafted', 'type': 'dict'}}\nTool Description: Create a draft reply to a specific Gmail thread"
)
def test_crewai_toolset() -> None:
toolset = composio_crewai.ComposioToolSet()
tools = toolset.get_tools(actions=[create_draft])
assert len(tools) == 1
tool = tools[0]
assert tool.name == "MYTOOL_CREATE_DRAFT"
assert (
tool.description
== "Tool Name: MYTOOL_CREATE_DRAFT\nTool Arguments: {'message_body': {'description': 'The content of the draft reply. Please provide a value of type string. This parameter is required.', 'type': 'str'}, 'thread': {'description': 'The thread to which the reply is to be drafted', 'type': 'object', 'properties': {'id': {'type': 'string', 'description': 'The unique identifier of the thread'}, 'subject': {'type': 'string', 'description': 'The subject of the thread'}}}}\nTool Description: Create a draft reply to a specific Gmail thread"
)

Comment on lines 121 to +128

; 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

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.

Comment on lines +105 to +108
- name: Plugin tests
if: matrix.python-version == '3.10'
run: |
tox -e plugins

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

Comment on lines +105 to +107
- name: Plugin tests
if: matrix.python-version == '3.10'
run: |

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

@angrybayblade angrybayblade merged commit 4a00e54 into master Jan 13, 2025
22 of 23 checks passed
@angrybayblade angrybayblade deleted the ENG-3283 branch January 13, 2025 06:07
abhishekpatil4 pushed a commit that referenced this pull request Jan 20, 2025
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.

3 participants