-
Notifications
You must be signed in to change notification settings - Fork 6
Add microgrid types #14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
bd68404
b342242
2b923ba
9349402
b158094
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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 | ||
|
|
||
| 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] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually the type here might depend on the resolution of https://github.com/frequenz-floss/frequenz-client-common-python/pull/14/files#r1495647444 |
||
| """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), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This raises an interesting question, copying the list (or whatever 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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), | ||
| ) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, if the common case would just be: |
||
There was a problem hiding this comment.
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?