Support power requests from shifting actors in the PowerManager #957
Conversation
github-actions
bot
added
part:docs
There was a problem hiding this comment.
I gave this a quick look, I still have to look at the commit doing the actual implementation in more detail, but I have a couple of comments:
-
It is still no 100% clear what incremental means here. It might be nice to add some better description to the PR and maybe even the docs, like one example with a few actors some using incremental and some not, and seeing what difference does it make in terms of what is the resulting power.
-
It looks like you have at least subscriptions and pools grouped in incremental and not incremental. It might be worth adding some class (not sure what it is exactly? Power requests? Algorithms?) that have 2 members, pool and subscriptions, and I don't know, maybe there is some more common code to manipulate both that can go there too. Maybe it doesn't make sense, not sure, as I said I only gave a quick look but it is something that popped out when looking at the commits.
|
Draft until I finish the renaming to |
|
No |
|
in_ in the argument names and class fields. |
shsms
changed the title
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
The older name was a bit ambiguous, because there are also bounds calculated by the power manager for use by actors. This change clears this ambiguity. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
This would return the most recently calculated target power for a given set of components. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
And propagate it all the way to the power manager. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
This is done by having an additional `Matryoshka` instance for resolving requests from shifting actors. And the target power thus obtained is added to the target power from the regular actors, which in effect, shifts the target power of the regular actors by the target power of the shifting actors. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
There was a problem hiding this comment.
I really like how this turned up! And the new documentation is great, it makes things so much clearer!
I made several comments but they are all part of the same thing, a proposal to have a specific class for managing pool groups, as I mentioned in my first review.
I really think it could simplify the code quite a bit and remove some duplication. If you agree, I think this refactoring should be done in a separate PR since I don't have any other comments other than that, I think it would be a good idea to merge this PR as is to keep things moving. So approving.
| @@ -211,6 +212,8 @@ def ev_charger_pool( | |||
| EVChargerPool. | |||
| name: An optional name used to identify this instance of the pool or a | |||
| corresponding actor in the logs. | |||
| in_shifting_group: Whether the power requests get sent to the shifting group | |||
| in the PowerManager or not. | |||
There was a problem hiding this comment.
It would be nice to include a link to the shifting group docs in here (and in every place that receives a in_shifting_group). I will be quite verbose, but we can greatly simplify it by creating a [macro like we did for linking to the glossary][https://github.com/frequenz-floss/frequenz-sdk-python/blob/8e5d65e383442c3fa85060e985e225d5e6412a9e/docs/_scripts/macros.py#L79-L97].
| self._subscriptions: dict[frozenset[int], dict[int, Sender[_Report]]] = {} | ||
| self._non_shifting_subscriptions: dict[ | ||
| frozenset[int], dict[int, Sender[_Report]] | ||
| ] = {} | ||
| self._shifting_subscriptions: dict[ | ||
| frozenset[int], dict[int, Sender[_Report]] | ||
| ] = {} | ||
| self._distribution_results: dict[frozenset[int], power_distributing.Result] = {} | ||
|
|
||
| self._algorithm: BaseAlgorithm = Matryoshka( | ||
| self._non_shifting_group: BaseAlgorithm = Matryoshka( | ||
| max_proposal_age=timedelta(seconds=60.0) | ||
| ) | ||
| self._shifting_group: BaseAlgorithm = Matryoshka( |
There was a problem hiding this comment.
Not going to insist a lot on this because I don't want to keep focusing on bikeshedding, but I though it out there just in case you agree it could bring some extra clarity. Back to my previous suggestion of grouping the groups, what about having something like:
class PoolGroup:
algorithm: BaseAlgorithm
subscriptions: dict[...]
# Again, I'm not sure if there are common operations with
# these algorithm and subscriptions that could be moved here too.
class PowerManagingActor(Actor):
def __init__(self, ...):
self._default_group = PoolGroup(...)
self._shifting_group = PoolGroup(...)?
There was a problem hiding this comment.
If we end up adding more than two groups, I think it makes sense to refactor at that time. For now, I'm not sure it is worth the effort, the code is not really that complicated the way it is.
There was a problem hiding this comment.
I created an issue just so it doesn't get lost, but I agree is OK as it is for now:
src/frequenz/sdk/actor/_power_managing/_power_managing_actor.py
Outdated
Show resolved
Hide resolved
src/frequenz/sdk/actor/_power_managing/_power_managing_actor.py
Outdated
Show resolved
Hide resolved
src/frequenz/sdk/actor/_power_managing/_power_managing_actor.py
Outdated
Show resolved
Hide resolved
|
Oh, right, there is also the cross referencing of |
|
It looks like DCO is still not working |
Co-authored-by: Leandro Lucarella <luca@llucax.com> Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Also update the function's docstring. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
There are cases where the target power needs to be shifted by a certain amount, for example, to make adjustments to the operating point. This PR enables this by designating some actors to be part of the
shifting_group.Closes #905