Skip to content
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 49 additions & 2 deletions src/frequenz/client/common/microgrid/__init__.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,51 @@
# License: MIT
# Copyright © 2023 Frequenz Energy-as-a-Service GmbH
# Copyright © 2024 Frequenz Energy-as-a-Service GmbH

"""Frequenz microgrid definition."""
"""Module to define the microgrid used with the common client."""

from __future__ import annotations # required for constructor type hinting

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This shouldn't be necessary anymore if you use Self, right?


from dataclasses import dataclass
from typing import Self

# pylint: disable=no-name-in-module
from frequenz.api.common.v1.microgrid.microgrid_pb2 import (
MicrogridComponentIDs as PBMicrogridComponentIDs,
)
# pylint: enable=no-name-in-module

@dataclass(frozen=True, kw_only=True)
class MicrogridComponentIDs:
"""Linking component IDs with their respective microgrid ID."""

microgrid_id: int
"""The ID of the microgrid."""

component_ids: Sequence[int]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sequence was not imported

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"""List of component IDs belonging to this microgrid."""

@classmethod
def from_proto(cls, microgrid_component_ids: PBMicrogridComponentIDs) -> Self:
"""Convert a protobuf MicrogridComponentIDs to MicrogridComponentIDs object.

Args:
microgrid_component_ids: MicrogridComponentIDs to convert.

Returns:
MicrogridComponentIDs object corresponding to the protobuf message.
"""
return cls(
microgrid_id=microgrid_component_ids.microgrid_id,
component_ids=List(microgrid_component_ids.component_ids),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This raises an interesting question, copying the list (or whatever component_ids is in the proto class) is definitely the safe option, but it means doing some copying, when in 99% of the cases you'll probably just take the protobuf message, convert it to the wrapper and discard it, so it is very unlikely it will be mutated after creating the wrapper (in which case it will also affect the wrapper list, as we stored a reference to the protobuf message).

Maybe since it is unlikely and might have a performance impact, we could document that a reference is taken and the users should take care of the copying if they plan to keep mutating the proto message instance.

@frequenz-floss/python-sdk-team any opinions?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Haven't really figured out what this type is for, but it still looks like a one time thing that we might have to process at startup, and not along with each streaming message, etc. So I don't see any benefit from not choosing the conservative approach.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeah, this is a more general question, because we will probably have it in other situations, as for this particular class, I guess we need to figure out if we want to pursue this PR as you mentioned in #8 (comment), I think it was a good point that we might be missing the end goal here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree it is fine to copy the IDs if the message is only processed once at startup (or only a few times). For general scenarios it makes sense to avoid the copy if it is possible

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And to @shsms point, if this is known to never leave the client, then we always know the protobuf is discarded and we don't need to copy. All points to not really having to copy stuff when converting from/to protobuf in this repo, again under the premise that types here will never be exposed to the user as is.

)

def to_proto(self) -> PBMicrogridComponentIDs:
"""Convert a MicrogridComponentIDs object to protobuf MicrogridComponentIDs.

Returns:
Protobuf message corresponding to the MicrogridComponentIDs object.
"""
return PBMicrogridComponentIDs(
microgrid_id=self.microgrid_id,
component_ids=List(self.component_ids),
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here, if the common case would just be: client.send(instance.to_proto()), then copying doesn't make a lot of sense.