Allow targeting categories with subtypes #168

Marenz · 2025-05-19T17:25:34Z

Update to latest dispatch-api
Implement extra classes for each target type

Fixes #139

Marenz · 2025-05-19T17:28:30Z

src/frequenz/client/dispatch/__main__.py

@@ -86,7 +86,8 @@ def print_dispatch(dispatch: Dispatch) -> None:
    # Format the target
    if dispatch.target:
        if len(dispatch.target) == 1:
-            target_str: str = str(dispatch.target[0])
+            (el,) = dispatch.target
+            target_str: str = str(el)


This change is because target is a frozenset now

OK, but why can't use use [0] for something frozen? You should be able, or is it because it is a set now?

Also what the hell does el stands for? It look it is pretty cryptic. Also if you need to clarify in the PR, maybe it is best to add a comment in the code itself for the future ourselves looking at this code.

You cannot index-access a set, it has no order :)

el stands for element.. sure I'll make it more clear 😄
I was surprised as well, but this seems to be the easiest way to access the only element of a set with size 1

llucax

LGTM in general, although not completely convinced about the current approach with CategoryAndType, looks a bit too low-level for the "high-level interface".

llucax · 2025-05-20T07:55:25Z

src/frequenz/client/dispatch/__main__.py

@@ -86,7 +86,8 @@ def print_dispatch(dispatch: Dispatch) -> None:
    # Format the target
    if dispatch.target:
        if len(dispatch.target) == 1:
-            target_str: str = str(dispatch.target[0])
+            (el,) = dispatch.target
+            target_str: str = str(el)


OK, but why can't use use [0] for something frozen? You should be able, or is it because it is a set now?

Also what the hell does el stands for? It look it is pretty cryptic. Also if you need to clarify in the PR, maybe it is best to add a comment in the code itself for the future ourselves looking at this code.

llucax · 2025-05-20T07:58:41Z

src/frequenz/client/dispatch/types.py

+class EvChargerType(IntEnum):
+    """Enum representing the type of EV charger."""
+
+    EV_CHARGER_TYPE_UNSPECIFIED = PBEvChargerType.EV_CHARGER_TYPE_UNSPECIFIED
+    """Unspecified type of EV charger."""
+
+    EV_CHARGER_TYPE_AC = PBEvChargerType.EV_CHARGER_TYPE_AC
+    """AC EV charger."""
+
+    EV_CHARGER_TYPE_DC = PBEvChargerType.EV_CHARGER_TYPE_DC
+    """DC EV charger."""
+
+    EV_CHARGER_TYPE_HYBRID = PBEvChargerType.EV_CHARGER_TYPE_HYBRID
+    """Hybrid EV charger."""


I have these in the microgrid API too, next step is to move them to client-common so we can share it. I will do a PR with this soon after I finish the microgrid API 0.17 PR, probably this week. But just FYI, I think we should move forward adding it where needed locally as it might take some time to agree on something we are all happy about to put in common + release. In any case, I would remove the EV_CHARGER_TYPE_ prefix here.

llucax · 2025-05-20T08:25:40Z

src/frequenz/client/dispatch/test/generator.py

@@ -66,6 +76,30 @@ def generate_recurrence_rule(self) -> RecurrenceRule:
            ],
        )

+    def generate_target_cat_and_type(self) -> CategoryAndType:


Nitpick: This is already pretty long, I wouldn't save the 5 extra chars from egory.

You're worse than copilot

llucax · 2025-05-20T08:26:57Z

src/frequenz/client/dispatch/types.py

+
+This is a frozen set, so it is immutable.
+The target components are used to specify the components that a dispatch
+should target.


Maybe include a match example here?

llucax · 2025-05-20T08:34:17Z

tests/test_proto.py

@@ -99,7 +130,7 @@ def test_dispatch() -> None:
            start_time=datetime(2024, 10, 10, tzinfo=timezone.utc),
            end_time=datetime(2024, 10, 20, tzinfo=timezone.utc),
            duration=timedelta(days=10),
-            target=[1, 2, 3],
+            target=TargetIds([1, 2, 3]),


Maybe we can add a __init__() here to simply allow: TargetIds(1, 2, 3)?

llucax · 2025-05-20T08:34:36Z

tests/test_proto.py

@@ -120,7 +151,7 @@ def test_dispatch() -> None:
            start_time=datetime(2024, 11, 10, tzinfo=timezone.utc),
            end_time=datetime(2024, 11, 20, tzinfo=timezone.utc),
            duration=timedelta(seconds=20),
-            target=[ComponentCategory.BATTERY],
+            target=TargetCategories([ComponentCategory.BATTERY]),


Same here: TargetCategories(ComponentCategory.BATTERY) and for TargetCategoryAndTypes, maybe allow something like TargetCategoryAndTypes(ComponentCategory.BATTERY, InverterType.SOLAR) (as with the type, we can infer the category, seems to be pretty redundant to require both).

llucax · 2025-05-20T08:56:21Z

src/frequenz/client/dispatch/types.py

+@dataclass(frozen=True)
+class CategoryAndType:
+    """Represents a category and type of dispatch."""
+
+    category: ComponentCategory
+    """The category of the dispatch."""
+
+    type: BatteryType | EvChargerType | InverterType | None
+    """The type of the dispatch."""


This is probably for the API repo, but it seems we now have overlapping target options:

TargetCategories([cat1, cat2]) == TargetCategoryAndTypes([CategoryAndType(cat1), CategoryAndType(cat2)])

(figuratively speaking)

As for the high-level user interface, since as soon as we have a type, the category can be inferred, I would simplify for the end user:

# Could still be CategoryAndType, but it sounds a bit weird type is not always present @dataclass(frozen=True) class Group: target: ComponentCategory | BatteryType | EvChargerType | InverterType @property def category(self) -> ComponentCategory: match self.target: case ComponentCategory(): return self.target case BattertyType(): return ComponentCategory.BATTERY ... @property def type(self) -> ComponentCategory: match self.target: case ComponentCategory(): return None case type: return type ...

And then add a ctor to class TargetCategoryAndTypes to allow for:

TargetCategoryAndTypes(ComponentCategory.BATTERY, InverterType.SOLAR)

to result in:

frozenset([Group(ComponentCategory.BATTERY), Group(InverterType.SOLAR)])

I'm not sure if we already touched on the redundancy between CategorySet and CategoryTypeSet in the protocol, if not maybe we should.

I'm not sure if we already touched on the redundancy between CategorySet and CategoryTypeSet in the protocol, if not maybe we should.

We don't need to talk about those redundancies because they are deprecated and scheduled to be removed, the whole typeless category and it's field in the oneof construct will be gone once all clients are a capable of reading/writing both.

llucax · 2025-05-20T09:01:31Z

src/frequenz/client/dispatch/_cli_types.py

@@ -5,13 +5,23 @@

 import json
 from datetime import datetime, timedelta, timezone
+from sre_constants import IN


I bet this was Tab-oriented-programming + auto-import 😆

Suggested change

from sre_constants import IN

it was more desperation, but this is a draft after all :P

Renames a few fields, but stays compatible otherwise. Signed-off-by: Mathias L. Baumann <[email protected]>

ela-kotulska-frequenz · 2025-05-26T16:00:45Z

src/frequenz/client/dispatch/types.py

+class TargetIds(frozenset[int]):
+    """A set of target component IDs.
+
+    This is a frozen set, so it is immutable.
+    """
+


Can we make it
class TargetIds(frozenset[ComponentId]): from frequenz.client.microgrid.ComponentId. ?

Let's wait for that, we should use it here only when it is in client-common. We can always convert between ComponentId and int. I don't want to rush things, because whatever goes to client-common will affect everyone, so it might take some time.

..and support the latest CategoryAndType target type. This implements extra classes for each target component case: * List of Ids `TargetIds` * List of categories: `TargetCategories` * List of categories + subtypes: `TargetCategoriesAndTypes` This was easier to do together as the addition of the new type increased the places where it was difficult to find out what the actual target type/ids is/are. Signed-off-by: Mathias L. Baumann <[email protected]>

llucax

Pretty cool!

llucax · 2025-05-27T12:22:39Z

RELEASE_NOTES.md

-<!-- Here goes notes on how to upgrade from previous versions, including deprecations and what they should be replaced with -->
+* `TargetComponents` was reworked. It now is a type alias for `TargetIds | TargetCategories`:
+ * `TargetIds` can be used to specify one or more specific target IDs:
+  * `TargetIds(1, 2, 3)` or


Unless we have some other situation like the one @ela-kotulska-frequenz mentioned and we want this only as a transitional step, I wouldn't allow raw ints, the whole point of introducing ComponentId is that you can't accidentally pass some random int (or some ID for other class of object) where a specific ID is required.

llucax · 2025-05-27T12:25:39Z

RELEASE_NOTES.md

+* `TargetComponents` was reworked. It now is a type alias for `TargetIds | TargetCategories`:
+ * `TargetIds` can be used to specify one or more specific target IDs:
+  * `TargetIds(1, 2, 3)` or
+  * `TargetIds(ComponentIds(1), ComponentIds(2), ComponentIds(3))`


Oh, but this seems to be fake news, right? This one is not supported yet?

This is specifically supported (if the type can be cast to int)!

llucax · 2025-05-27T12:35:32Z

src/frequenz/client/dispatch/types.py


-It can be a list of component IDs or a list of categories.
+class EvChargerType(IntEnum):


Don't use IntEnum, this makes the values instances of int, allowing crazy stuff like abs(EvChargerType.UNSPECIFIED) or EvChargerType.AC / EvChargerType.DC 👨‍🎤

llucax · 2025-05-27T12:38:21Z

src/frequenz/client/dispatch/types.py

+    This is a frozen set, so it is immutable.
+    """
+
+    def __new__(cls, *ids: SupportsInt) -> "TargetIds":


😱 you can't define __init__ becuase it is frozen? 🥶

llucax · 2025-05-27T12:38:46Z

src/frequenz/client/dispatch/types.py

+    This is a frozen set, so it is immutable.
+    """
+
+    def __new__(cls, *ids: SupportsInt) -> "TargetIds":


from typing import Self:

Suggested change

def __new__(cls, *ids: SupportsInt) -> "TargetIds":

def __new__(cls, *ids: SupportsInt) -> Self:

Probably it will not be subclasses, but if it is, the type should be the type of the sub-class, not always this one.

llucax · 2025-05-27T13:08:11Z

src/frequenz/client/dispatch/types.py

+    This is a frozen set, so it is immutable.
+    """
+
+    def __new__(cls, *categories_input: TargetCategoryInputType) -> "TargetCategories":


Suggested change

def __new__(cls, *categories_input: TargetCategoryInputType) -> "TargetCategories":

def __new__(cls, *categories_input: TargetCategoryInputType) -> Self:

llucax · 2025-05-27T13:09:01Z

src/frequenz/client/dispatch/types.py

+# Define the union of types that can be passed to TargetCategories constructor
+TargetCategoryInputType = (
+    TargetCategory | ComponentCategory | BatteryType | InverterType | EvChargerType
+)


Usually it is a good idea to document type aliases too so they appear in the docs.

Suggested change

)

)

"""The union of all types that can be used as target categories."""

Marenz commented May 19, 2025

View reviewed changes

llucax reviewed May 20, 2025

View reviewed changes

Update to latest dispatch-api

d981cd8

Renames a few fields, but stays compatible otherwise. Signed-off-by: Mathias L. Baumann <[email protected]>

Marenz force-pushed the api_update branch from 6e3c2c7 to 40043e5 Compare May 26, 2025 15:59

ela-kotulska-frequenz reviewed May 26, 2025

View reviewed changes

Marenz force-pushed the api_update branch from 40043e5 to 4e38b52 Compare May 26, 2025 16:20

ela-kotulska-frequenz mentioned this pull request May 27, 2025

Change type of TargetComponents #170

Closed

Marenz force-pushed the api_update branch from 4e38b52 to 770b8dd Compare May 27, 2025 11:29

github-actions bot added the part:docs Affects the documentation label May 27, 2025

Marenz marked this pull request as ready for review May 27, 2025 11:30

Marenz requested review from a team as code owners May 27, 2025 11:30

Marenz requested review from stefan-brus-frequenz, shsms and llucax and removed request for shsms and stefan-brus-frequenz May 27, 2025 11:30

Marenz force-pushed the api_update branch from 770b8dd to b5f513a Compare May 27, 2025 12:14

Marenz force-pushed the api_update branch from b5f513a to 722b878 Compare May 27, 2025 13:06

llucax reviewed May 27, 2025

View reviewed changes


		It can be a list of component IDs or a list of categories.
		class EvChargerType(IntEnum):

	def __new__(cls, *ids: SupportsInt) -> "TargetIds":
	def __new__(cls, *ids: SupportsInt) -> Self:

	def __new__(cls, *categories_input: TargetCategoryInputType) -> "TargetCategories":
	def __new__(cls, *categories_input: TargetCategoryInputType) -> Self:

	)
	)
	"""The union of all types that can be used as target categories."""

Allow targeting categories with subtypes #168

Are you sure you want to change the base?

Allow targeting categories with subtypes #168

Uh oh!

Conversation

Marenz commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Marenz commented May 19, 2025 •

edited

Loading