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

Add cancellation token to hooks and pipeline bootstrap #345

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

slaroche
Copy link
Contributor

No description provided.

@slaroche slaroche requested review from isra17 and mlefebvre August 15, 2023 15:09
@slaroche slaroche self-assigned this Aug 15, 2023
@isra17
Copy link
Member

isra17 commented Aug 15, 2023

I'm not sure why we need the token to track cancellation instead of just handling the exception inside the hook that stop running the hooks and scope.

@isra17
Copy link
Member

isra17 commented Aug 15, 2023

I'm not sure why we need the token to track cancellation instead of just handling the exception inside the hook that stop running the hooks and scope.

Another approach could be to add a cancel method or event (asyncio.Event) to ExecutableMessage/TopicMessage? This could be checked in the executor queue to not execute the message.

I had a use-case for this this morning where a RabbitMQ channel closing must cancel all its yielded messages.

@slaroche
Copy link
Contributor Author

I'm not sure why we need the token to track cancellation instead of just handling the exception inside the hook that stop running the hooks and scope.

I wanted to do that initial, but not running the following hooks was ok, not running the scope function was trickier. I might be because I was trying to work just in the utils/hook.

I will the idea to handle it in the executor, I will try something.

Thanks for the feedback.

@slaroche
Copy link
Contributor Author

I have a question:
if I catch and handle a CancelPipeline(Exception), is it important to close all the generators from on_call and on_results or I should just try to fail silently?

@slaroche slaroche force-pushed the sam/cancel_pipeline branch 2 times, most recently from f90856e to a474d41 Compare August 16, 2023 14:54
@slaroche slaroche marked this pull request as draft August 17, 2023 18:38
@slaroche slaroche changed the title WIP: add cancellation token to hooks and pipeline bootstrap Add cancellation token to hooks and pipeline bootstrap Aug 17, 2023
@slaroche slaroche marked this pull request as ready for review August 17, 2023 18:39
@slaroche
Copy link
Contributor Author

@isra17 ready for review

@slaroche slaroche force-pushed the sam/cancel_pipeline branch from a474d41 to 18d88aa Compare August 21, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants