Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exception handling rework #329

Draft
wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

Exception handling rework #329

wants to merge 1 commit into from

Conversation

pseusys
Copy link
Collaborator

@pseusys pseusys commented Feb 16, 2024

Description

Exceptional conditions support added. Exceptional conditions are similar to regular conditions, however they are triggered on exceptions instead.

Checklist

  • I have performed a self-review of the changes

To Consider

  • Add tests (if functionality is changed)
  • Update API reference / tutorials / guides
  • Update CONTRIBUTING.md (if devel workflow is changed)
  • Update .ignore files, scripts (such as lint), distribution manifest (if files are added/deleted)
  • Search for references to changed entities in the codebase

@pseusys pseusys added the enhancement New feature or request label Feb 16, 2024
@pseusys pseusys requested review from kudep and RLKRo February 16, 2024 20:11
@pseusys pseusys self-assigned this Feb 16, 2024
Copy link
Member

@RLKRo RLKRo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to have actor.process_exception?

Is there a way to process exception as part of regular actor process? (it is allowed to change actor service however you want -- e.g. split it into sub-services)

@pseusys
Copy link
Collaborator Author

pseusys commented Feb 19, 2024

I would say that a separate step is required, because Actor processing happens between pre- and postservices and if we process exception during regular Actor processing the excetions in postservuces will remain unhandled.

@RLKRo
Copy link
Member

RLKRo commented Feb 19, 2024

Ok. I think it might be better to handle exceptions separately.

But I'm pretty sure this still has a previously discussed problem of leaving ctx in an incomplete state if an exception happens inside of an actor service (ctx.responses might have a response that was never sent).
This can be solved by resetting ctx to its original state with minor exceptions (such as saving exception info).

Other than that -- I think it looks decent. I don't like the copy-paste in process_exception though but don't have a solution for that off the top of my head.

@pseusys pseusys changed the title initial Exceptional conditions support Feb 21, 2024
@RLKRo
Copy link
Member

RLKRo commented Feb 27, 2024

To keep in mind while reading the proposal:

  1. Actor handlers are likely to be removed in the future updates

  2. PRE_TRANSITION_PROCESSING might be replaced by pre-services in the future updates

  3. Actor steps are as follows:

    1. CONTEXT_INIT
    2. GET_PREVIOUS_NODE
    3. REWRITE_PREVIOUS_NODE
    4. RUN_PRE_TRANSITIONS_PROCESSING
    5. GET_TRUE_LABELS
    6. GET_NEXT_NODE
    7. REWRITE_NEXT_NODE
    8. RUN_PRE_RESPONSE_PROCESSING
    9. CREATE_RESPONSE
    10. FINISH_TURN

    Here we can split actor stages in 3 groups: 1-4 (pre-transition); 5-7 (transition); 8-10 (response)

  4. Implementation of this proposal entails editing Actor.__call__ code

Proposal on how to handle exceptions:

  1. Pre/post-service exceptions / before/after-handlers: save & log exception info to ctx. Context state should be reset to that at the beginning of the service / handler (barring exception info).
  2. Exceptions in actor stage group 1-4: save & log exception info. The exception info can be used in conditions. Context state should be reset to that of the last successful actor step (barring exception info). Failed actor handlers do not affect actor step success status.
  3. Exceptions in actor stage group 5-7: Equivalent to actor failing to find correct transition. In this case we should set fallback_node as the next node. Save&log exception info. Context should be reset to its state at the end of the previous group + fallback_node label & exception info.
  4. Exceptions in actor stage group 8-10: Exceptions during response generation. Save & log exception info. Context should be reset to its state at the end of previous group + response substitute & exception info. Response substitute should be the following (in order of priority):
    1. Pipeline.fallback_message if it is passed as a pipeline argument (or a CONFIG keyword).
    2. RESPONSE of the fallback node if it is a Message object and not a function.
    3. An empty message Message().

Exception info should include exception class; exception message; exception traceback; pipeline service / actor stage that raised the exception.


Do tell if there are any suggestions or if I'm overthinking exception handling.

Copy link
Member

@RLKRo RLKRo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RLKRo RLKRo changed the title Exceptional conditions support Exception handling rework May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants