bug/AP-25110-implement-error-handling-option #5
Conversation
Reverts previous changes by storing error messages separately from the agent memory.
src/agents/base.py
Outdated
| if self.error_handling == ErrorHandlingMode.FAIL.name: | ||
| ctx.set_warning(str(e)) | ||
| else: | ||
| errors.append(str(e)) |
There was a problem hiding this comment.
With this, validation errors for mode "FAIL" are displayed as warnings and validation errors for mode "ERROR COLUMN" are included as errors in the error column and not shown as warnings. This makes sense to me but we could also show these as warning in both modes.
| default_value=False, | ||
| since_version="5.10.0", | ||
| is_advanced=True, | ||
| ) |
There was a problem hiding this comment.
We could also add a rule to only show this if re-execution trigger is set, because otherwise the node does not create output data. But the output table schema is independent from the other parameter so I think it makes more sense without a rule.
| ) | ||
|
|
||
| def _fill_memory_with_messages( | ||
| self, agent, view_data, data_registry, tool_converter |
There was a problem hiding this comment.
I think we could use
from __future__ import annotations
from typing import TYPE_CHECKING
to have typehints without performance issues due to the imports. I didn't do it yet because I was not sure if there are any drawbacks.
| "configurable": {"thread_id": "1"}, | ||
| } | ||
| previous_messages = [] | ||
| error_messages = {} |
There was a problem hiding this comment.
I fill these by passing them into the functions below. I could see how this is bad style because it is not obvious outside of the functions, but I am not sure about it.
src/agents/base.py
Outdated
| history_df = history_table[self.conversation_column].to_pandas() | ||
|
|
||
| for msg in history_df[self.conversation_column]: | ||
| if msg: |
There was a problem hiding this comment.
This changes the behavior of the Agent Prompter slightly because now missing values are simply ignored (in both error handling modes). But I don't think this creates any problems. And it makes sense to me because a conversation table with error column might contain missing values in the conversation column but should still be a valid input for an Agent Prompter that uses mode "FAIL".
AtR1an
left a comment
AtR1an
left a comment
There was a problem hiding this comment.
I didn't review all the code but from the parts I saw it's clear to me that we reached the point where we need to refactor in order to manage the complexity and improve readability. We probably should've done that already but now I feel that it's crucial to keep this maintainable and extendable.
| default_value="Conversation", | ||
| ).rule(knext.DialogContextCondition(has_conversation_table), knext.Effect.HIDE) | ||
|
|
||
| error_handling = knext.EnumParameter( |
There was a problem hiding this comment.
Would it be possible to extract the error handling related parameters in to a parameter group?
| else: | ||
| return knext.Schema.from_columns( | ||
| [ | ||
| knext.Column(_message_type(), self.conversation_column), |
There was a problem hiding this comment.
the line for the conversation column exists multiple times. That makes the logic look way harder than it actually is. There is always a conversation column and if we want to output the error column, there is also an error column.
| f"An error occurred while executing the agent: {e}" | ||
| ) | ||
| error_message = f"An error occurred while executing the agent: {e}" | ||
| if self.error_handling == ErrorHandlingMode.FAIL.name: |
There was a problem hiding this comment.
All of the if self.error_handling switches could be abstracted into a separate class that encapsulates the error handling logic and makes the node's logic simpler. It could also be easily unit tested.
| output_column, | ||
| messages, | ||
| history_table, | ||
| history_df, |
There was a problem hiding this comment.
Aren't history_table and history_df referring to the same thing?
| ] | ||
| } | ||
| conversation_table = self._create_conversation_table( | ||
| output_column_name, |
There was a problem hiding this comment.
The ´output_column_name´ can be constructed inside self._create_conversation_table.
| history_table, | ||
| history_df, | ||
| errors, | ||
| num_messages_before, |
There was a problem hiding this comment.
Isn't that the number of rows of history_df / history_table?
Also reverts previous changes made to conversation histories by storing error messages separately from the agent memory.