Improve UX for action processor #709
Conversation
|
This comment was generated by github-actions[bot]! JS SDK Coverage Report📊 Coverage report for JS SDK can be found at the following URL: 📁 Test report folder can be found at the following URL: |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to d1b40fc in 24 seconds
More details
- Looked at
492lines of code in11files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/tests/test_tools/test_toolset.py:158
- Draft comment:
The test for theprocessorsfield inComposioToolSetconstructor should include a check for the deprecation warning. Usepytest.warnsto ensure the warning is raised. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests a change that is not clearly required by the current code. There is no evidence in the code that a deprecation warning is expected or should be tested for. Without strong evidence that this is necessary, the comment should be removed.
I might be missing some context about theComposioToolSetclass and whether there is a known deprecation warning that should be tested for. However, based on the provided code, there is no indication of this.
The absence of any deprecation-related code or comments in the provided diff suggests that the comment is speculative. Without clear evidence, it is safer to remove the comment.
Remove the comment as it suggests a speculative change without strong evidence from the code.
Workflow ID: wflow_jHYJFUUhDwR047kN
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 36e2317 in 13 seconds
More details
- Looked at
41lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/tests/test_tools/test_toolset.py:11
- Draft comment:
Organize imports according to PEP 8: standard library imports, related third-party imports, and local application/library-specific imports. - Reason this comment was not posted:
Confidence changes required:50%
The import order should follow PEP 8 guidelines for better readability and organization.
Workflow ID: wflow_gcy0Sdepb2WAsE3z
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on bf13af4 in 17 seconds
More details
- Looked at
20lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/tests/test_tools/test_toolset.py:21
- Draft comment:
The docstring mentionsfind_actions_by_tags, but this test is forget_action_schemas. Please update the docstring to reflect the correct method being tested. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_xxl9Spt2xKkq7Eyn
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 2fc40f3 in 14 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_UMZVp5y2XX8B8yQN
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 431ce02 in 14 seconds
More details
- Looked at
21lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/tox.ini:127
- Draft comment:
Uncommenting thepip install plugins/langchainline means this plugin will be installed during the test setup. Ensure this is necessary for the tests being run, as it could introduce dependency conflicts or unnecessary installations. - Reason this comment was not posted:
Confidence changes required:50%
The change in the tox.ini file is minor and involves uncommenting a line to install a plugin. This is a straightforward change, but it should be noted that uncommenting this line means the plugin will be installed during the test environment setup, which could have implications if there are dependency conflicts or if the plugin is not needed for all tests.
Workflow ID: wflow_G8CkGfQVxIbiDWMs
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 8d2ce33 in 21 seconds
More details
- Looked at
19lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. python/tests/test_tools/test_toolset.py:215
- Draft comment:
The testtest_processors_on_get_toolsis not testing theprocessorsfield inget_tools()method as described. Instead, it is testing theexecute_actionmethod. This is a logical error in the test case. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_ShZRi0l4HRFr96jT
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
python/tox.ini
Outdated
| @@ -124,14 +124,15 @@ commands = | |||
|
|
|||
| ; TODO: Extract plugin tests separately | |||
| ; Installing separately because of the dependency conflicts | |||
| pip install plugins/langchain | |||
There was a problem hiding this comment.
| pip install plugins/langchain | |
| pip install plugins/langchain --no-deps |
And add the langchain plugin dependencies here - https://github.com/ComposioHQ/composio/blob/ENG-1726/python/tox.ini#L89
There was a problem hiding this comment.
Reason is, tox installs the current top-level package (composio-core) in this case before running the tests and running pip install plugins/langchain might overwrite the composio-core installation which might lead to tests breaking.
python/composio/tools/toolset.py
Outdated
| # Users may not respect our type annotations and return something that isn't a dict. | ||
| # If that happens we should show a friendly error message. | ||
| if not isinstance(data, t.Dict): | ||
| raise TypeError( | ||
| f"Expected {type_}-processor to return 'dict', got {type(data).__name__!r}" | ||
| ) |
There was a problem hiding this comment.
I would suggest log a warning instead raising error since this can interrupt the agent execution and agent might get stuck in a loop, also as long as the return object is JSON serialisable there won't be any issue so just a warning should be fine. We can improve the documentation around this.
python/composio/tools/toolset.py
Outdated
| for processor_type in self._processors.keys(): | ||
| if processor_type in processors: | ||
| processor_type = t.cast( | ||
| te.Literal["pre", "post", "schema"], processor_type |
There was a problem hiding this comment.
Can extract te.Literal["pre", "post", "schema"] as ProcessorType
There was a problem hiding this comment.
❌ Changes requested. Incremental review on fe574c6 in 38 seconds
More details
- Looked at
141lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. python/composio/tools/toolset.py:611
- Draft comment:
The deprecation warning message for 'processors' could be clearer. Consider rephrasing to: "The 'processors' parameter in the ToolSet constructor is deprecated. Please provide it to the 'get_tools()' or 'execute_action()' methods instead." - Reason this comment was not posted:
Confidence changes required:50%
The deprecation warning for the 'processors' parameter in the ComposioToolSet constructor is correctly implemented, but the warning message could be improved for clarity.
2. python/composio/tools/toolset.py:661
- Draft comment:
The check forprocessor_type in self._processorsis unnecessary sinceexisting_processorsis initialized if not present. You can simplify the code by directly updatingexisting_processors. - Reason this comment was not posted:
Confidence changes required:50%
The _merge_processors method can be simplified by removing the unnecessary check for processor_type in self._processors, as it is already being initialized if not present.
Workflow ID: wflow_LfSkGQnw8vExuZVv
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.
| @@ -67,22 +67,28 @@ | |||
| P = te.ParamSpec("P") | |||
|
|
|||
| _KeyType = t.Union[AppType, ActionType] | |||
| _ProcessorType = t.Callable[[t.Dict], t.Dict] | |||
| _CallableType = t.Callable[[t.Dict], t.Dict] | |||
There was a problem hiding this comment.
Consider using a more specific type for _CallableType. Instead of t.Callable[[t.Dict], t.Dict], you could use t.Callable[[t.Dict[str, t.Any]], t.Dict[str, t.Any]] to be more precise about the input and output types.
| else {"post": {}, "pre": {}, "schema": {}} | ||
| ) | ||
| if processors is not None: | ||
| warnings.warn( |
There was a problem hiding this comment.
The warning message here could be more specific. Instead of just mentioning 'processors', it could say "Setting 'processors' directly on the ToolSet instance is deprecated..."
| @@ -628,6 +650,22 @@ def _process_schema_properties(self, action: Action, properties: t.Dict) -> t.Di | |||
| type_="schema", | |||
| ) | |||
|
|
|||
| def _merge_processors(self, processors: ProcessorsType) -> None: | |||
There was a problem hiding this comment.
Consider adding a type hint for the processors parameter in the _merge_processors method signature.
| # Improperly defined processors | ||
| preprocessor_called = postprocessor_called = False | ||
|
|
||
| def weird_postprocessor(reponse: dict) -> None: |
There was a problem hiding this comment.
The variable name reponse in the weird_postprocessor function is misspelled. It should be response.
|
I've completed the review of this pull request. Overall, the changes look good and improve the functionality of the toolset. Here's a summary of my findings:
In terms of code quality, I'd rate this PR as 8/10. The changes are well-structured and improve the existing codebase. The addition of new tests is particularly commendable. There are just a few minor points that could be addressed to make it even better. Great job on this PR! |
| @@ -628,6 +650,22 @@ def _process_schema_properties(self, action: Action, properties: t.Dict) -> t.Di | |||
| type_="schema", | |||
| ) | |||
|
|
|||
| def _merge_processors(self, processors: ProcessorsType) -> None: | |||
There was a problem hiding this comment.
Consider adding a type hint for the processors parameter in the _merge_processors method to improve code readability and maintainability.
| # Improperly defined processors | ||
| preprocessor_called = postprocessor_called = False | ||
|
|
||
| def weird_postprocessor(reponse: dict) -> None: |
There was a problem hiding this comment.
The weird_postprocessor function in the test case doesn't follow the expected signature for a post-processor. It should return a dict, but it's currently returning None. Consider updating the function to return the modified response.
|
I've completed the review of the pull request. Overall, the changes look good and improve the functionality of the toolset by adding support for processors in various methods. Here's a summary of my observations:
There are a couple of minor suggestions for improvement:
Overall, the code quality is good, and the changes seem to enhance the flexibility and usability of the toolset. Good job! |
|
|
||
| MetadataType = t.Dict[_KeyType, t.Dict] | ||
| ParamType = t.TypeVar("ParamType") | ||
|
|
||
| # Enable deprecation warnings | ||
| warnings.simplefilter("always", DeprecationWarning) |
There was a problem hiding this comment.
Consider adding a type hint for the warnings module to improve code readability and maintainability.
| warnings.simplefilter("always", DeprecationWarning) | ||
|
|
||
|
|
||
| ProcessorType = te.Literal["pre", "post", "schema"] |
There was a problem hiding this comment.
The ProcessorType type alias is defined but not used in this file. Consider removing it if it's not needed, or use it to improve type checking in the relevant parts of the code.
| @@ -628,6 +650,22 @@ def _process_schema_properties(self, action: Action, properties: t.Dict) -> t.Di | |||
| type_="schema", | |||
| ) | |||
|
|
|||
| def _merge_processors(self, processors: ProcessorsType) -> None: | |||
There was a problem hiding this comment.
The _merge_processors method modifies the self._processors dictionary in-place. Consider returning a new dictionary instead of modifying the existing one to maintain immutability and prevent potential side effects.
| # Improperly defined processors | ||
| preprocessor_called = postprocessor_called = False | ||
|
|
||
| def weird_postprocessor(reponse: dict) -> None: |
There was a problem hiding this comment.
The weird_postprocessor function is defined but not used in the test. Consider removing it or using it in a test case to ensure it behaves as expected.
|
I've completed the review of the PR. Here's a summary of the changes and my observations:
Some minor suggestions for improvement:
Overall, this PR seems to improve the flexibility and usability of the toolset. The changes are well-implemented and thoroughly tested. With a few minor adjustments, this PR would be ready for merging. |
| @@ -628,6 +650,22 @@ def _process_schema_properties(self, action: Action, properties: t.Dict) -> t.Di | |||
| type_="schema", | |||
| ) | |||
|
|
|||
| def _merge_processors(self, processors: ProcessorsType) -> None: | |||
There was a problem hiding this comment.
In the _merge_processors method, consider adding a type check for the processors parameter to ensure it's of type ProcessorsType. This will help catch potential errors early and improve type safety.
| self._processors[processor_type] = existing_processors | ||
|
|
||
| existing_processors.update(new_processors) | ||
|
|
||
| @_record_action_if_available | ||
| def execute_action( | ||
| self, |
There was a problem hiding this comment.
Consider adding type hints for the processors parameter in the execute_action method. This will improve type checking and make the method signature more consistent with other parts of the codebase.
| @@ -628,6 +650,22 @@ def _process_schema_properties(self, action: Action, properties: t.Dict) -> t.Di | |||
| type_="schema", | |||
| ) | |||
|
|
|||
| def _merge_processors(self, processors: ProcessorsType) -> None: | |||
There was a problem hiding this comment.
Consider adding a docstring for the new _merge_processors method to explain its purpose and usage. This will help other developers understand the method's functionality and how to use it correctly.
| @@ -593,6 +608,13 @@ def _process( | |||
| f" through: {processor.__name__}" | |||
| ) | |||
| data = processor(data) | |||
| # Users may not respect our type annotations and return something that isn't a dict. | |||
There was a problem hiding this comment.
Consider adding more comprehensive error handling for cases where processors might fail or return unexpected types. While you've added a warning for unexpected return types, it might be beneficial to have a more robust error handling strategy, possibly including custom exceptions or fallback behavior.
|
Overall, this pull request introduces valuable improvements to the processor functionality in the ComposioToolSet. The addition of the Here's a summary of the changes and some suggestions for further improvement:
Suggestions for further improvement:
Overall, this is a solid improvement to the codebase that enhances its flexibility and maintainability. With a few minor additions to documentation and error handling, it will be even more robust and user-friendly. |
Adds a
processorsfield on theexecute_actionandget_toolsmethods, and deprecates the use ofprocessorsfield in theComposioToolSetconstructor.Important
Adds
processorsparameter toexecute_actionandget_tools, deprecating its use inComposioToolSetconstructor, with tests for new functionality.processorsparameter toexecute_actionandget_toolsmethods inComposioToolSetand derived classes.processorsparameter inComposioToolSetconstructor.execute_actionintoolset.pyto merge processors if provided.get_toolsintoolset.pyand derived classes to handle processors.processorsinComposioToolSetconstructor.test_toolset.pyfor newprocessorshandling inexecute_actionandget_toolsmethods.This description was created by
for fe574c6. It will automatically update as commits are pushed.