Implement ServiceBase using a new PersistentTaskGroup
#30
Conversation
Even when just returning `None` should be compatible with `bool | None`, it is better to declare the right type explicitly, as we had some issues in the past with `mypy` if this is not the case. Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
We are using a syntax that it looks like one can (re)construct the object by using the `repr()` but this is not true in this case, so btter to follow the Python convention and use `<>` in this case. Signed-off-by: Leandro Lucarella <[email protected]>
We tend to use `:` for short representations, so it is better to stick to that and it is shorter and make nesting less verbose. Signed-off-by: Leandro Lucarella <[email protected]>
Since more code is coming, and this module is already a bit large, we split it into a util and a service sub-module. Signed-off-by: Leandro Lucarella <[email protected]>
github-actions
bot
added
part:tests
|
This brings a Next, I will create a |
|
I hope that with this |
llucax
added
the
cmd:skip-release-notes
|
Skipping release notes as I will write them for the first release from scratch. |
llucax
force-pushed
the
service-main-task
branch
from
8d83a23 to
6772da2
Compare
Right, I think it would help to reduce complexity in the code and make it less error-prone by keeping track of the created tasks throughout their lifespan and taking care of errors handling. |
Yeah, I actually went through the same reasoning, and thought too about being able to attach some metadata to the task, but it feels like this is not the place to put that metadata, you can always create the dict yourself for that. In that case for me, for example, I finally didn't even need to attach the metadata by just thinking about the problem differently. We can see and if in the future we see more uses cases for attaching metadata, we could add it, but for now I would leave it out. |
Agree. It is clear to me now after checking your example. I admit that's not how I initially thought about it 👼 |
`asyncio.TaskGroup` is a very convenient construct when using parallelization for doing calculations for example, where the results for all the tasks need to be merged together to produce a final result. In this case if one of the tasks fails, it makes sense to cancel the others and abort as soon as possible, as any further calculations would be thrown away. This class is intended to help managing a group of tasks that should persist even if other tasks in the group fail, usually by either only discarding the failed task or by restarting it somehow. Signed-off-by: Leandro Lucarella <[email protected]>
This is revealing too much internal information about how a service can be implemented, it makes more sense to expose it only to classes implementing a service, but not to service users. Signed-off-by: Leandro Lucarella <[email protected]>
The service base class now delegates all sub-task management to a `PersistentTaskGroup`, and has a special task called `main()` that drives the service. Writing single-task services should be much easier now, as only a `main()` method needs to be implemented. For more complex, multi-task, services, the internal task group can be used to monitor sub-tasks, for example by using `task_group.as_completed()`. Signed-off-by: Leandro Lucarella <[email protected]>
llucax
force-pushed
the
service-main-task
branch
from
6772da2 to
bd468e7
Compare
| *, | ||
| name: str | None = None, | ||
| context: contextvars.Context | None = None, | ||
| log_exception: bool = True, |
There was a problem hiding this comment.
I'm not so sure about this, if it is easy to check for completed tasks to handle errors, maybe we don't need the logging feature (or we'll get most code littered with log_exceptions=False). But if anyone forgets, then we are screwed again with task dying without notice.
I would love to find a way to log only if the task was not ACKed somehow, but I couldn't so far.
I guess leaving it for now is the safest bet, if people need to opt-out explicitly at least there are much bigger chances that they will handle the exceptions. And for a casual user that doesn't know how to do it right, at least we have a log message.
|
Will merge to keep moving, but if anyone has any more comments or feedback, please comment and I can change in future PRs. |
3 participants
asyncio.TaskGroupis a very convenient construct when using parallelization for doing calculations for example, where the results for all the tasks need to be merged together to produce a final result. In this case if one of the tasks fails, it makes sense to cancel the others and abort as soon as possible, as any further calculations would be thrown away.This PR introduces a new
PersistentTaskGroupclass, intended to help managing a group of tasks that should persist even if other tasks in the group fail, usually by either only discarding the failed task or by restarting it somehow.The
ServiceBaseclass is updated to use aPersistentTaskGroupunderneath, and it is simplified for single-task services by making the service driven by a singlemaintask, which can use the new group to monitor sub-tasks and act accordingly.This is part of #27 and #9.