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

Implement BackgroundService and new Actor class #564

Merged
merged 25 commits into from
Aug 25, 2023

Conversation

llucax
Copy link
Contributor

@llucax llucax commented Aug 4, 2023

BackgroundService is a new abstract base class can be used to write other classes that runs one or more tasks in the background. It provides a consistent API to start and stop these services and also takes care of the handling of the background tasks. It can also work as an async context manager, giving the service a deterministic lifetime and guaranteed cleanup.

The new Actor class brings quite a few new improvements over the old @actor decorator. These are the main differences:

  • It doesn't start automatically, start() needs to be called to start an actor.
  • The method to implement the main logic was renamed from run() to _run(), as it is not intended to be run externally.
  • Actors can have an optional name (useful for debugging/logging purposes) and loop (if the actor should run in a loop different from the currently running loop).
  • The actor will only be restarted if an unhandled Exception is raised by _run(). It will not be restarted if the _run() method finishes normally. If an unhandled BaseException is raised instead, it will be re-raised. For normal cancellation the _run() method should handle asyncio.CancelledError if the cancellation shouldn't be propagated (this is the same as with the decorator).
  • The _stop() method is public (stop()) and will cancel() and await for the task to finish, catching the asyncio.CancelledError.
  • The join() method is renamed to wait(), but they can also be awaited directly ( await actor).
  • For deterministic cleanup, actors can now be used as async context managers.

The base actors (ConfigManagingActor, ComponentMetricsResamplingActor, DataSourcingActor, PowerDistributingActor) now inherit from the new Actor class, as well as the MovingWindow.

Fixes #240, fixes #45, fixes #196.

@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:actor Affects an actor ot the actors utilities (decorator, etc.) labels Aug 4, 2023
@llucax llucax force-pushed the background-service branch from 4692189 to 8db7fca Compare August 4, 2023 13:38
@llucax llucax added type:enhancement New feature or enhancement visitble to users part:core Affects the SDK core components (data structures, etc.) part:config Affects the configuration management labels Aug 4, 2023
@llucax

This comment was marked as outdated.

@llucax llucax self-assigned this Aug 4, 2023
@Marenz
Copy link
Contributor

Marenz commented Aug 4, 2023

In your commit message, insted -> instead, raise -> raised (printed bold in the quoted text)

The main differences with the @actor decorator are:
[...]

  • The actor will only be restarted if an unhandled Exception is raise
    by _run(). It will not be restarted if the _run() method finishes
    normally. If an unhandled BaseException is raised insted, it will be

@llucax llucax force-pushed the background-service branch from 8db7fca to b0020c0 Compare August 7, 2023 14:30
@github-actions github-actions bot removed part:core Affects the SDK core components (data structures, etc.) part:config Affects the configuration management labels Aug 7, 2023
@llucax llucax force-pushed the background-service branch from b0020c0 to 0673319 Compare August 7, 2023 14:35
@llucax
Copy link
Contributor Author

llucax commented Aug 7, 2023

Updated with the suggestions by @Marenz

@llucax llucax force-pushed the background-service branch from 0673319 to 9a31a82 Compare August 7, 2023 14:44
@llucax llucax requested a review from Marenz August 7, 2023 14:44
@llucax llucax force-pushed the background-service branch from 9a31a82 to 10bc894 Compare August 8, 2023 10:59
@github-actions github-actions bot added part:data-pipeline Affects the data pipeline part:microgrid Affects the interactions with the microgrid labels Aug 8, 2023
@Marenz
Copy link
Contributor

Marenz commented Aug 9, 2023

Other than those small things, design wise I didn't notice any problems with the code. I would leave a more detailed review once the draft part is over.

@llucax llucax force-pushed the background-service branch 2 times, most recently from 1d28683 to ee5ecb1 Compare August 11, 2023 10:06
@llucax
Copy link
Contributor Author

llucax commented Aug 11, 2023

OK, now this has all actor classes migrated. I'm pretty happy about the results.

Next step are:

  • Remove the actor decorator
  • Write tests
  • Write release notes
  • Update PR description

And in a follow-up PR:

  • Migrate other classes to use BackgroundService (like MovingWindow, BatteryPoolStatus, and whatever uses some background tasks and has run/stop/join methods.

@llucax llucax force-pushed the background-service branch 2 times, most recently from 68458ec to 4d3af38 Compare August 21, 2023 13:40
@github-actions github-actions bot added the part:docs Affects the documentation label Aug 21, 2023
llucax added 20 commits August 25, 2023 09:54
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
When using an union, if we want to use a method present only in one of
the union types, then we need to assert for the type, so it is better
to just make it generic to the type is always the specific type.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
We'll need to start actors in this function in the future, so we need it
to be async to be able to `await actor.start()`.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
This improves actors debuggeability.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
This gives the moving window a common interface and removes some
bookkeeping from the implementation, as well as making tests more
correct (when using it as an async context manager).

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Now that actors are stopped properly, we can reestablish the restart
limit after it was changed.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
A new utility `actor_restart_limit` context manager is added to be able
to temporarily change the actor's restart limit, and it is now used in
the `disable_actor_auto_restart` fixture.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
This is useful to test that actors are being properly restarted.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
In particular we test that the new restart rules work as expected, and
we also check the logs produced to have extra certainty that what we
expect to happen is really happening.

The restart tests are done using the `run()` function because we expect
to move the restart logic there, so the tests are "forward-compatible".

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
When a `BackgroundService.wait()` is used, it should also propage
`CancelledError`s, as is it not always normal that a background service
is cancelled unless `stop()` is called, which uses `cancel()` to stop
the tasks.

When `stop()` is used, we properly filter out all `CancelledError`s too,
as in that case we triggered the cancellation, so it is normal.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
We verify that cancelled actors are reported as such by the `run()`
function and that the actor is not automatically restarted.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Add some tests for the `BackgroundService` class. The tests were written
looking at the coverage to avoid duplicating a lot of tests that are
already performed in the `Actor` tests.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
This is so they don't show up in the generated documentation.

Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
Signed-off-by: Leandro Lucarella <luca-frequenz@llucax.com>
@llucax llucax force-pushed the background-service branch from ace41e1 to cf12100 Compare August 25, 2023 07:55
@llucax llucax requested a review from shsms August 25, 2023 07:55
@llucax
Copy link
Contributor Author

llucax commented Aug 25, 2023

Enabling auto-merge

Copy link
Contributor

@shsms shsms left a comment

Choose a reason for hiding this comment

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

Awesome, everybody get ready to refactor everything now!

@shsms
Copy link
Contributor

shsms commented Aug 25, 2023

Enabling auto-merge

I think you also have to click a button, looks like you just typed the message. :D

@llucax
Copy link
Contributor Author

llucax commented Aug 25, 2023

It seems like GitHub copilot is not there yet...

@llucax llucax added this pull request to the merge queue Aug 25, 2023
Merged via the queue into frequenz-floss:v0.x.x with commit 3e7c2fe Aug 25, 2023
@llucax llucax deleted the background-service branch August 25, 2023 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
part:actor Affects an actor ot the actors utilities (decorator, etc.) part:data-pipeline Affects the data pipeline part:docs Affects the documentation part:microgrid Affects the interactions with the microgrid part:tests Affects the unit, integration and performance (benchmarks) tests type:enhancement New feature or enhancement visitble to users
Projects
None yet
3 participants