Power Manager _Report, Report and BatteryPoolReport have the wrong hierarchy

[llucax]

What happened?

The current hierarchy is:

classDiagram
    Report <|-- _Report
    Report <|-- BatteryPoolReport

Loading

The problem is BatteryPoolReport is a _Report, and _Report is the concrete class sent through channels, so there are instances when a _Report needs to be converted to a BatteryPoolReport, for which we just need to go behind the back of the type system.

What did you expect instead?

If I have a Receiver[_Report], I expect to be able to safely return a BatteryPoolReport from it without any casting.

Affected version(s)

No response

Affected part(s)

Battery power distribution (part:power-distribution)

Extra information

So, the naive solution is:

classDiagram
    BatteryPoolReport <|-- _Report

Loading

But of course this doesn't work in the whole context of Report. In the current code, there are no other Reports that are not BatteryPoolReport, so I'm not sure how the end use of this hierarchy should look like. If it is just documentation, I'm not sure it is worth hiding _Report, I think it would be simpler to just keep it generic and then have something like:

Note

When applied to different components, the attributes of this class have different meanings.

• BatteryPool: component IDs are battery IDs. Yada yada yada
• Etc.

Also I wonder if we can solve this by using completely different types for different Report types, that have no inheritance relationship, are all concrete classes and instead make Report = BatteryPoolReport | PvPoolReport | .... Then we can use the match syntax to know what we received from the channel.

I guess we also need to mix all these different reports in the same channel because we are going to use the same power manager for all, because ideally we should just use one concrete type per channel and then we don't need the hierarchy or the type alias at all.

[llucax]

FYI @shsms

[shsms]

If I have a Receiver[_Report], I expect to be able to safely return a BatteryPoolReport from it without any casting.

This has nothing to do with the use of the Protocol type. This is because the Receiver was too rigid on what type of generic arguments it will accept.

This can be resolved with: frequenz-floss/frequenz-channels-python#262

[shsms]

Actually it needs this too, once the channel PR is merged:

diff --git a/src/frequenz/sdk/_internal/_channels.py b/src/frequenz/sdk/_internal/_channels.py
index d739470d..d3b5a68a 100644
--- a/src/frequenz/sdk/_internal/_channels.py
+++ b/src/frequenz/sdk/_internal/_channels.py
@@ -9,14 +9,14 @@ import typing
 
 from frequenz.channels import Receiver
 
-T = typing.TypeVar("T")
+_T_co = typing.TypeVar("_T_co", covariant=True)
 
 
-class ReceiverFetcher(typing.Generic[T], typing.Protocol):
+class ReceiverFetcher(typing.Protocol[_T_co]):
     """An interface that just exposes a `new_receiver` method."""
 
     @abc.abstractmethod
-    def new_receiver(self, *, maxsize: int = 50) -> Receiver[T]:
+    def new_receiver(self, *, maxsize: int = 50) -> Receiver[_T_co]:
         """Get a receiver from the channel.
 
         Args:
@@ -27,21 +27,21 @@ class ReceiverFetcher(typing.Generic[T], typing.Protocol):
         """
 
 
-class LatestValueCache(typing.Generic[T]):
+class LatestValueCache(typing.Generic[_T_co]):
     """A cache that stores the latest value in a receiver."""
 
-    def __init__(self, receiver: Receiver[T]) -> None:
+    def __init__(self, receiver: Receiver[_T_co]) -> None:
         """Create a new cache.
 
         Args:
             receiver: The receiver to cache.
         """
         self._receiver = receiver
-        self._latest_value: T | None = None
+        self._latest_value: _T_co | None = None
         self._task = asyncio.create_task(self._run())
 
     @property
-    def latest_value(self) -> T | None:
+    def latest_value(self) -> _T_co | None:
         """Get the latest value in the cache."""
         return self._latest_value

[llucax]

I still don't see how this fixes this particular issue. Here Actual is Report and Narrower is BatteryPoolReport. The channel needs to carry Report and the receiver can only receive Broader types, not Narrower, so it can receive a BatteryPoolReport, and the channel could transport some other sub-class of Report that's not a BatteryPoolReport.

This is why I think in this case our best option is to use type unions instead. We don't really need a hierarchy at all, we want battery pools to return receivers for BatteryPoolReport and other pools to receive other types of reports, right?

Maybe I am misunderstanding the requirements because I'm not super familiar with how the power manager works...

[shsms]

BatteryPoolReport is broader than _Report, which is the actual type of the messages, and that's all that matters.

There was no Report originally, and I still don't see a use for it, but using just protocols made you uncomfortable for some reason, so you suggested adding it.

[llucax]

I don't get that at all. If there is no Report then it makes no sense to talk about broader or narrower at all, as there is no relationship between _Report and BatteryPoolReport.

As shown in the issue description, the hierarchy is currently:

class Report(typing.Protocol):
    ...

class _Report(Report):
    ...

class BatteryPoolReport(Report):
    ...

channel = Broadcast[_Report]()

I still don't see how can I legally get a BatteryPoolReport receiver from a Broadcast[_Report] channel, even using covariance.

[shsms]

Because that's what protocols allow, and that is the reason they exist.

BatteryPoolReport is a broader class of _Report, and _Report becomes compatible with the protocol class BatteryPoolReport, just by implementing the methods required by BatteryPoolReport, without using it as a base class.

You can think of them as an implicit ABC from the eyes of the type checker, that we are able to declare after the fact.

[llucax]

the protocol class BatteryPoolReport

Ok, this is the new part, right now the protocol class is Report (ok, maybe BatteryPoolReport is too because it inherits from it, if so I never thought about that side effect about inheriting from a subclass of Protocol).

Ok, now i see what you mean. I still think this is not the typical use case for Protocol, but I agree it solves the issue. So you say this:

class BatteryPoolReport(Protocol):
    ...
class _Report:
    ...

Again, I think this is not intuitive (I use myself as a proof of it 😂) and still wonder what do you have against:

class BatteryPoolReport:
    ...
Report = BatteryPoolReport | ...
    ...
channel = Broadcast[Report]()

This is because the producing end of these reports don't know anything about which type of pool is producing reports for?

[llucax]

BTW, if we are using Protocol I am surprised we need covariance. I thought covariance only applied to inheritance, but maybe is about sub-typing more generally and _Report then is a sub-type of _BatteryPoolReport.

[shsms]

This is because the producing end of these reports don't know anything about which type of pool is producing reports for?

yup, it is because we don't want to add another async layer and additional allocation to rewrap the _Report objects. And this is by no means the most complicated part of the power manager. But it does hide the complication and present the user with a nice interface.

Also I think this is a perfect use case for Protocol, it might not be typical but that's also most things in the SDK.

[shsms]

ok, maybe BatteryPoolReport is too because it inherits from it, if so I never thought about that side effect about inheriting from a subclass of Protocol

of course, that's how inheritance works in Python, but in hindsight, I shouldn't have agreed to mix inheritance with Protocol, because it does make the whole thing murky and opaque.

[llucax]

Yeah, the argument for that was to make it more explicit that a class is implementing a protocol, and the nice side effect is that mypy will complain if the protocol changes but the downstream class "implementing" it is not. I miss the unintended side effect of making the downstream class a protocol too.

[shsms]

and the nice side effect is that mypy will complain if the protocol changes but the downstream class "implementing" it is not

this happens also when there is no common base class.

[llucax]

Not exactly, at least IIRC the error just says that class X is not compatible with protocol Y, it doesn't say in which way, what's missing or incompatible.

[llucax]

$ cat p.py 
from typing import Protocol

class P(Protocol):
    def f(self) -> None:
        ...

class C:
    def g(self) -> None:
        pass

def h(p: P) -> None:
    pass

h(C())
$ mypy p.py 
p.py:14: error: Argument 1 to "h" has incompatible type "C"; expected "P"  [arg-type]
Found 1 error in 1 file (checked 1 source file)

[llucax]

So now the EVChargerPoolReport is a plain protocol¹, not inheriting from Report, right? So we are moving forward to that scheme but still are in a mixed scenario. If this is the case I would remove Report sooner than later to finish cleaning this up.

Footnotes

1. https://github.com/frequenz-floss/frequenz-sdk-python/pull/900/commits/7a00ca340e58cec4355acb0249cd55471ec658ed#diff-a1c09a4b0a1ea86bb308c9d8c16facf2f1dc515aa3c591cd820aff06004a3c7bR13 ↩

[llucax]

PVPoolReport is also using a plain protocol.