Raise TriggerFail from triggers and add a "message" attribute to all states #67
Conversation
|
In one version of this, I had different states take different arguments. For example, |
prefect/engine/executors/base.py
Outdated
| def set_state(self, current_state: State, state: State, data: Any = None) -> State: | ||
| return state(data) | ||
| def set_state( | ||
| self, current_state: State, state: State, data: Any = None, message: str = None |
There was a problem hiding this comment.
message will sometimes be an actual Exception as well.
There was a problem hiding this comment.
Good point. I'm not too familiar with passing errors around as strings (maybe the traceback is lost?)-- do you think it's preferable to change the type signature to message: Union[str, Exception] or to call this function as set_state(message=str(e))?
There was a problem hiding this comment.
So from what I remember, we changed from str(e) because that drops the exception type and only returns the exception's message. Maybe we could use repr(e) though? I'm game to try it with all strings first and try to keep states lean, although I do wonder how a user would debug a failing task with no traceback.
There was a problem hiding this comment.
Setting the exception is common enough, let's just make it Union type annotation for now. I am removing this method anyway in my proposed executor/state refactor
Following up on #59, this PR does two major things:
Converts triggers to raise a new
TRIGGERFAILsignal that, in turn, sets aTriggerFailstate, which closes Update trigger functions to useTriggerFailed#61.Trapping this error highlighted the importance of being able to pass an optional
messagewith each state, in addition to its data attribute, that helps explain how and why it came to be. So state now takes two (optional) arguments:dataandmessage.In addition, it adds a bunch of tests for states