Refactor agent execution to simplify context handling and reduce code duplication #16
Conversation
- Remove unused BaseMessage import from base.py - Remove unnecessary column check in execute (already in configure) - Pass ExecutionContext directly instead of AgentPrompterContext wrapper - Move conversation construction into _get_conversation method - Extract history table processing into dedicated method with row IDs in error messages - Move validation logic into AgentPrompterConversation.validate_final_message - Remove AgentPrompterContext class (no longer needed) - Simplify create_output_table by removing with_error_column parameter - Remove AgentWidgetContext wrapper in _data_service.py - Simplify FrontendConversation.create_output_table to use self._tool_converter - Remove _append_messages wrapper in _agent.py (inline wrapping) Co-authored-by: AtR1an <[email protected]>
The error handler now has a single handle_error method that accepts any exception and makes the differentiation itself, simplifying the calling code. Co-authored-by: AtR1an <[email protected]>
- Move BaseMessage import to module level to avoid repeated import overhead - Remove inline imports from frequently called methods - Extract duplicated append_msg logic into shared _append_sanitized_message helper - Fix inconsistent whitespace in import statements Co-authored-by: AtR1an <[email protected]>
- Fix row index access using iloc[idx].name instead of index[idx] - Move IterationLimitError import to top of _error_handler.py module Co-authored-by: AtR1an <[email protected]>
- Use history_df.index[idx] to correctly get row index - Remove extra trailing blank lines in _agent.py for consistency Co-authored-by: AtR1an <[email protected]>
| return | ||
| self._append_messages(response) | ||
|
|
||
| self._conversation.append_messages(response) |
There was a problem hiding this comment.
shouldn't this wrap the response into a message?
There was a problem hiding this comment.
The append_messages method in AgentPrompterConversation already handles wrapping single messages into a list (line 892-893 in base.py):
if isinstance(messages, BaseMessage):
messages = [messages]So when we call self._conversation.append_messages(response) with a single BaseMessage response from invoke(), it's automatically wrapped. The code is working correctly.
src/agents/base.py
Outdated
| ctx.set_warning(str(e)) | ||
| else: | ||
| conversation.append_error(e) | ||
| conversation.validate_final_message(validate_ai_message, ctx) |
There was a problem hiding this comment.
The conversation should validate each AI message, not just the final one. Therefore validate_ai_message should be a method of the conversation.
There was a problem hiding this comment.
Refactored in commit ed6e64e. The conversation now validates each AI message as it's added via the append_messages method, not just the final message. The validate_ai_message function is passed to the conversation constructor and called for every AI message added.
src/agents/base.py
Outdated
| desanitized_messages_and_errors = [ | ||
| { | ||
| "message": tool_converter.desanitize_tool_names(moe["message"]) | ||
| if moe["message"] is not None | ||
| else None, | ||
| "error": moe["error"], | ||
| } | ||
| for moe in messages_and_errors | ||
| ] | ||
| messages = [ | ||
| from_langchain_message(msg) if msg is not None else None | ||
| for msg in desanitized_messages | ||
| from_langchain_message(moe["message"]) | ||
| if moe["message"] is not None | ||
| else None | ||
| for moe in desanitized_messages_and_errors |
There was a problem hiding this comment.
This is even more complicated and harder to read than the previous implementation that I asked you to simplify. Can't you define a helper function that returns None if the input is None and otherwise desanitizes and converts to a KNIME message?
There was a problem hiding this comment.
Simplified in commit ed6e64e. Added helper function to_knime_message_or_none that returns None if input is None, otherwise desanitizes and converts to KNIME message. This eliminates the nested list comprehensions and makes the code much more readable.
|
@copilot apply requested changes |
… table creation - Move validation logic into append_messages to validate each AI message as it's added - Simplify create_output_table with to_knime_message_or_none helper function - Remove validate_final_message method (now inline in append_messages) - Properly handle FAIL mode with ctx.set_warning for validation errors Co-authored-by: AtR1an <[email protected]>
Co-authored-by: AtR1an <[email protected]>
All requested changes have been applied in commits ed6e64e and f484d43:
All files compile successfully and are ready for review. |
src/agents/base.py
Outdated
| history_table: Optional[knext.Table], | ||
| tool_converter, | ||
| data_registry, | ||
| validate_ai_message, |
There was a problem hiding this comment.
there is no need to pass a method handle. It's always the same method. Just call it directly.
There was a problem hiding this comment.
Fixed in commit f3e8f4a. The AgentPrompterConversation class now imports validate_ai_message directly from the _agent module instead of receiving it as a parameter. This simplifies the method signature since it's always the same function.
Instead of passing validate_ai_message as a parameter through _get_conversation, the AgentPrompterConversation class now imports and calls it directly from the _agent module. This simplifies the API since it's always the same function. Co-authored-by: AtR1an <[email protected]>
AtR1an
merged commit b3273f7
into
bug/AP-25110-implement-error-handling-option
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.