Skip to content

Conversation

@llucax
Copy link
Contributor

@llucax llucax commented Sep 20, 2023

  • Fix the outdated frequenz-channels cross-reference link
  • Show inherited members in the API reference
  • Use a quote admonition instead of the normal quote
  • Use more readable variable names for examples
  • Add a note about why we use an async context manager
  • Fix numbered list indentation
  • Add Lifecycle, Communication and Implementing an Actor sections
  • Improve the Example section to host more than one example
  • Add an example showing how to receive from multiple channels
  • Add section on spawning extra tasks
  • Clarify that using channels is not enforced
  • Remove detailed documentation from the class (and add links to the actor module documentation)
  • Add a section explaining how to initialize an actor

@llucax llucax requested a review from a team as a code owner September 20, 2023 14:09
@llucax llucax requested a review from Marenz September 20, 2023 14:09
@llucax llucax self-assigned this Sep 20, 2023
@github-actions github-actions bot added part:docs Affects the documentation part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) part:actor Affects an actor ot the actors utilities (decorator, etc.) labels Sep 20, 2023
@llucax llucax added this to the v1.0.0-rc2 milestone Sep 20, 2023
@llucax
Copy link
Contributor Author

llucax commented Sep 20, 2023

I have a couple of TODOs:

  • Add an advanced section explaining how to create more tasks using the background service underneath.
  • Write release notes (?) (Maybe we should only add one item saying "added a lot of docs)

@llucax llucax force-pushed the more-actors branch 3 times, most recently from 0525561 to da7192b Compare September 21, 2023 12:26
@llucax
Copy link
Contributor Author

llucax commented Sep 21, 2023

This is based on #688.

@llucax llucax dismissed matthias-wende-frequenz’s stale review September 22, 2023 07:20

The merge-base changed after approval.

@llucax llucax force-pushed the more-actors branch 2 times, most recently from f372cea to 1ad1f67 Compare September 22, 2023 09:51
@llucax
Copy link
Contributor Author

llucax commented Sep 22, 2023

Rebased on the current v0.x.x and added a few improvements:

  • Add a section explaining how to initialize an actor
  • Add link to the actor module documentation
  • Remove detailed documentation from the class
  • Remove unused (and duplicated) code annotation
  • Improve wording
  • Improve links consistency

They are all new commits so it should be easy to resume the reviewing 😇

@llucax
Copy link
Contributor Author

llucax commented Sep 22, 2023

Enabled auto-merge.

@llucax llucax force-pushed the more-actors branch 3 times, most recently from ab6d1ea to 0c2d276 Compare September 25, 2023 07:38
@llucax
Copy link
Contributor Author

llucax commented Sep 25, 2023

Mmm, there is an unrelated test failure now, I guess pylint got smarter and it is complaining about:

nox > pylint src benchmarks docs examples noxfile.py tests
************* Module src.frequenz.sdk.microgrid._data_pipeline
src/frequenz/sdk/microgrid/_data_pipeline.py:270:12: E1101: Instance of '_T' has no 'start' member (no-member)
src/frequenz/sdk/microgrid/_data_pipeline.py:296:12: E1101: Instance of '_T' has no 'start' member (no-member)
src/frequenz/sdk/microgrid/_data_pipeline.py:302:18: E1101: Instance of '_T' has no 'stop' member (no-member)
src/frequenz/sdk/microgrid/_data_pipeline.py:304:18: E1101: Instance of '_T' has no 'stop' member (no-member)
************* Module tests.microgrid.test_datapipeline
tests/microgrid/test_datapipeline.py:48:11: E1101: Instance of '_T' has no 'is_running' member (no-member)
tests/microgrid/test_datapipeline.py:52:11: E1101: Instance of '_T' has no 'is_running' member (no-member)

@shsms
Copy link
Contributor

shsms commented Sep 25, 2023

But this is already checked by mypy, pylint has an incomplete implementation that doesn't look at type hints. We see the same failure where it is checking the generated grpc code in the api client implementation as well. We should just disable the check for the whole of the sdk.

But adding some constraints in the code doesn't sound like a bad idea either:

diff --git a/src/frequenz/sdk/microgrid/_data_pipeline.py b/src/frequenz/sdk/microgrid/_data_pipeline.py
index 09f5808c..a5483e2a 100644
--- a/src/frequenz/sdk/microgrid/_data_pipeline.py
+++ b/src/frequenz/sdk/microgrid/_data_pipeline.py
@@ -30,6 +30,7 @@ _logger = logging.getLogger(__name__)
 # pylint: disable=import-outside-toplevel
 if typing.TYPE_CHECKING:
     from ..actor import (
+        Actor,
         ComponentMetricRequest,
         ComponentMetricsResamplingActor,
         DataSourcingActor,
@@ -52,7 +53,8 @@ A larger buffer size means that the DataSourcing and Resampling actors don't dro
 requests and will be able to keep up with higher request rates in larger installations.
 """
 
-_T = typing.TypeVar("_T")
+_T = typing.TypeVar("_T", bound="Actor")
 """Type variable for generic actor types."""
 
 

It is much easier to find all the available members of a class if the
inherited members are shown, otherwise one have to inpect every base
manually.

Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
These new sections explain in more detail how actors work, and what's
needed to both use them and implement them properly.

Signed-off-by: Leandro Lucarella <[email protected]>
Also add the expected output for the example.

Signed-off-by: Leandro Lucarella <[email protected]>
This illustrates the use of select to be able to manage multiple
channels in one actor. It also showcases a bit more the `run()` function
and more advance uses of channels.

Signed-off-by: Leandro Lucarella <[email protected]>
This section explains how to use the underlaying `BackgroundService` to
spawn new tasks and get them automatically cancelled when stopping the
actor.

Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
The SDK doesn't enforce the use of channels, but it's the recommended
way to communicate between actors and the one used by SDK actors.

Signed-off-by: Leandro Lucarella <[email protected]>
Now when talking about the `Actor` class, a link to the class is used
more consistently. When mentioning the `_run()` method, a link to the
"The `_run()` Method" section is provided too. When mentioning frequenz
channels, we link using the module instead of using URL. This improves
consistency but also makes sure we link to the version the SDK is
currently using.

Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
The detailed documentation for how to use and implement actors now lives
in the module. The class documentation is removed because it is not
comprehensive enough and to avoid duplication.

A link to the module documentation will be added in a separate commit.

Signed-off-by: Leandro Lucarella <[email protected]>
To be able to use the `Actor` class and `run()` function there is a lot
to have in mind that is hard to explain (without a lot of duplication)
in each docstring. Because of this we just add an information box
suggesting users to read the module-level documentation.

Signed-off-by: Leandro Lucarella <[email protected]>
The initializer is not trivial, as it is usually the one taking input
and output channels, and the `Actor.__init__()` needs to be called too,
and the optional `name` should be explained too.

The new section includes an example and a continuation of this example
is added to the "The `_run()` Method" section, to make it more clear
what the bare minimum needed to implement a complete actor.

We also add the `name` argument to the complete examples.

Signed-off-by: Leandro Lucarella <[email protected]>
The `nox` job makes `publish-docs` depend on every `nox` job matrix
entry, while the `nox-all` is a join point for them. Depending on both
has the same effect, but the dependency graph looks nicer if we depend
on `nox-all` instead.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax llucax added this pull request to the merge queue Sep 26, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 26, 2023
@llucax
Copy link
Contributor Author

llucax commented Sep 26, 2023

Merging without the queue because of the flakiness.

@llucax llucax merged commit e48809f into frequenz-floss:v0.x.x Sep 26, 2023
@llucax llucax deleted the more-actors branch September 26, 2023 11:17
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:docs Affects the documentation part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)

Projects

Development

Successfully merging this pull request may close these issues.

3 participants