feat: update tool parameters propagation #167
Open
+162
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
olbychos
changed the title
olbychos
requested review from
acoola,
delamainer,
anastasiasenyk,
vitalii-dynamiq and
MaksymAI
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 64a61a3 in 2 minutes and 45 seconds
More details
- Looked at
254lines of code in5files - Skipped
0files when reviewing. - Skipped posting
10drafted comments based on config settings.
1. dynamiq/nodes/agents/base.py:510
- Draft comment:
Consider clarifying the merging logic in _apply_parameters: when both merged_input[key] and the incoming value are dicts, the call to deep_merge may not override primitive values. Ensure that higher‐priority parameters truly override lower ones if needed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
The comment raises a valid technical concern about the deep merging behavior, but several issues make me lean towards removing it: 1) The deep_merge method isn't shown in the diff or context, so we can't verify if there's actually an issue 2) The comment is speculative ("may not override") rather than pointing to a definite problem 3) The priority order is clearly documented in the code comments (global -> name -> id) 4) The code has debug logging to help trace any merge issues.
I could be wrong about the severity of the potential deep merge issue - if deep_merge has bugs, it could cause subtle parameter override problems that would be hard to debug.
While deep merge bugs could be serious, without seeing the deep_merge implementation we can't verify if there's a real issue. The comment is too speculative and would require more context to be actionable.
Delete the comment because it's speculative and requires more context about deep_merge to be actionable. The code already has clear documentation about parameter priority and debug logging.
2. dynamiq/nodes/agents/base.py:537
- Draft comment:
The merging order (global, then tool name, then tool id) is subtle. Add a comment clarifying these priority rules. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. dynamiq/nodes/node.py:1129
- Draft comment:
deep_merge overwrites non-dict values. Consider if a more robust merge strategy is needed for complex nested structures. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%
None
4. dynamiq/nodes/tools/http_api_call.py:123
- Draft comment:
The check using 'self.model_fields_set' for response_type fallback depends on pydantic internals. Verify compatibility with your pydantic version. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. dynamiq/nodes/agents/base.py:617
- Draft comment:
Typo: The variable name 'formated_prompt_blocks' appears to be misspelled. Consider renaming it to 'formatted_prompt_blocks' for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. dynamiq/nodes/agents/react.py:84
- Draft comment:
Typographical error: In the REACT_BLOCK_INSTRUCTIONS block, 'strickly adhere to it' should be corrected to 'strictly adhere to it'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. dynamiq/nodes/agents/react.py:120
- Draft comment:
Typographical error: In the REACT_BLOCK_INSTRUCTIONS_STRUCTURED_OUTPUT block, 'Structure you responses in JSON format.' should be corrected to 'Structure your responses in JSON format.' - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. dynamiq/nodes/node.py:341
- Draft comment:
Typo in the docstring: 'if it is is provided' should be corrected to 'if it is provided'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
9. dynamiq/nodes/node.py:313
- Draft comment:
Typo in the comment: 'if the function accepts the at least "inputs" and "outputs"' should be changed to 'if the function accepts at least "inputs" and "outputs" parameters' (remove the extra 'the'). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
10. dynamiq/nodes/tools/http_api_call.py:53
- Draft comment:
Typographical error: In the description of the 'connection' attribute, there is a missing space after the period. It currently reads '...http requests.A new connection...' and should be corrected to '...http requests. A new connection...' - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_rugefUrJN2Pxpjsk
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.
…assing' into feat/investigate_agents_params_passing
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e4f1200 in 1 minute and 25 seconds
More details
- Looked at
1679lines of code in14files - Skipped
0files when reviewing. - Skipped posting
16drafted comments based on config settings.
1. tests/integration/evaluations/metrics/test_answer_correctness.py:33
- Draft comment:
The use of multiple side_effect lists for extraction and classification is clear. Consider adding inline comments or helper functions to document the order of calls to improve maintainability. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
2. tests/integration/flows/test_flow.py:60
- Draft comment:
Good use of fixtures and tracing validations. Ensure that file encoding (BytesIO) and bytes decoding are robust—especially when handling non-UTF-8 content. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
3. tests/integration/flows/test_flow.py:252
- Draft comment:
Test assertions regarding tracing and dependency mapping are comprehensive. Verify that expected metadata keys (e.g., 'host', 'workflow id') always exist; if possible, add defensive checks or more informative error messages. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%
None
4. pyproject.toml:6
- Draft comment:
Versioning and dependency information look consistent. Consider adding a changelog entry referencing the tool parameter propagation update for clarity. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%
None
5. dynamiq/nodes/agents/base.py:119
- Draft comment:
Nice addition of the 'tool_params' field with proper agent exposure control. Consider adding inline comments to explain the merging priority (global < tool name < tool ID). - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. dynamiq/nodes/agents/base.py:305
- Draft comment:
Switching assignment from _prompt_variables['context'] to _prompt_blocks['context'] is a notable change. Ensure this is intended as it affects prompt formatting. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. dynamiq/nodes/tools/exa_search.py:25
- Draft comment:
Input schema parameters now include an 'is_accessible_to_agent' flag, which is good for tool parameter control. Verify that the new parameters align with tool usage in agents. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to verify alignment, which is against the rules. It doesn't provide a specific suggestion or point out a clear issue.
8. pyproject.toml:7
- Draft comment:
Version bumped to 0.11.1; ensure all dependency updates have been properly tested. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is related to dependency changes and asks the author to ensure that all dependency updates have been properly tested. This violates the rule against asking the author to ensure testing and commenting on dependency changes.
9. tests/integration/evaluations/metrics/test_answer_correctness.py:140
- Draft comment:
The test expectations for F1 scores seem correctly computed. Ensure the mocked responses align with actual LLM behavior for future robustness. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to ensure that the mocked responses align with actual LLM behavior, which is a form of asking them to double-check or verify something. This violates the rule against asking the author to ensure or verify things.
10. tests/integration/flows/test_flow.py:124
- Draft comment:
Workflow tests now include checks for tracing and input mappings. Review if all expected metadata fields are consistently propagated. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%
This comment is asking the PR author to review and ensure that all expected metadata fields are consistently propagated. This falls under the category of asking the author to double-check things, which is against the rules.
11. dynamiq/nodes/agents/base.py:55
- Draft comment:
Typo in the prompt template: Consider removing the extra 'to' in the phrase "Refer to this as to additional information, not as direct instructions." It should read "Refer to this as additional information, not as direct instructions." - Reason this comment was not posted:
Comment was on unchanged code.
12. dynamiq/nodes/agents/base.py:617
- Draft comment:
Typographical error: The variable 'formated_prompt_blocks' is misspelled. It should be 'formatted_prompt_blocks' to maintain correct spelling. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
13. dynamiq/nodes/agents/react.py:84
- Draft comment:
Typographical error: In the REACT_BLOCK_INSTRUCTIONS string (around line 84), 'strickly adhere to it' should be corrected to 'strictly adhere to it'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
14. dynamiq/nodes/agents/react.py:198
- Draft comment:
Typographical error in the description of final_answer_function_schema: The phrase "when if you can answer the initial request or if there is not request at all" is awkward. Consider revising it to 'Function should be called when you can answer the initial request or when there is no request at all.' - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
15. dynamiq/nodes/tools/exa_search.py:31
- Draft comment:
Typographical error: please change 'converted to a Exa query' to 'converted to an Exa query'. This issue can be found in the description for 'use_autoprompt' (line 31) and is repeated in the tool's field description (line 94). - Reason this comment was not posted:
Comment was on unchanged code.
16. dynamiq/prompts/prompts.py:73
- Draft comment:
Typo: In the docstring for the format_message method, 'formated copy of message' should be corrected to 'formatted copy of message'. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_cfj7Ww9fn5iQO4Ky
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
anastasiasenyk
approved these changes
Feb 28, 2025
| @@ -157,6 +166,7 @@ class Agent(Node): | |||
|
|
|||
| model_config = ConfigDict(arbitrary_types_allowed=True) | |||
| input_schema: ClassVar[type[AgentInputSchema]] = AgentInputSchema | |||
| _tool_params: dict[str, Any] | None = None | |||
There was a problem hiding this comment.
It looks like this parameter has not beed used. Is it needed somewhere?
| @@ -1127,6 +1127,26 @@ def merge_and_short_content(inputs: dict, outputs: dict[str, dict]): | |||
| self.input_mapping[key] = value | |||
| return self | |||
|
|
|||
| def deep_merge(self, source: dict, destination: dict) -> dict: | |||
There was a problem hiding this comment.
We could consider adding list concatenation in the deep_merge function. For example, in TavilyInputSchema, several parameters have the list[str] type.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Add
tool_paramstoAgentInputSchemafor flexible tool parameter application, with priority handling and a new example script.tool_paramsfield toAgentInputSchemainbase.pyfor passing parameters to tools without LLM exposure._apply_parameters()inAgentto apply parameters fromtool_paramswith priority: global, tool name, tool ID._run_tool()inAgentto use merged input with applied parameters.deep_merge()function toNodeinnode.pyfor merging dictionaries.use_agents_hidden_params.pydemonstrates setting up aReActAgentwith API tools and usingtool_paramsfor authorization headers.This description was created by
for e4f1200. It will automatically update as commits are pushed.