Add component types #11

Marenz · 2024-02-06T10:33:04Z

~~Depends on #10~~

Delete cookiecutter template leftovers
Add components types from SDK

llucax

This probably need a better structure, closer to the API definitions structure, but like with #8 I'm OK to merge it if the idea is just to get things moving an improve in followup PRs.

The dependency to microgrid-api is kind of big an problematic though, so I probably would not merge that.

pyproject.toml

llucax · 2024-02-06T11:23:51Z

src/frequenz/client/common/_component.py

I would make the module public instead, all the entities in the different directories in the protobuf are pretty much independent from each other.

I think also for this particular repo it would be very useful to stay as close as possible to the api-common repository, so I would rename this file as: src/frequenz/client/common/microgrid/components/__init__.py and put there everything related to proto/frequenz/api/common/v1/microgrid/components/components.proto there, and create one public file per any other proto/frequenz/api/common/v1/microgrid/components/*.py inside it, for example:

proto/frequenz/api/common/v1/microgrid/components/battery.proto src/frequenz/client/common/microgrid/components/battery.py`.

I would probably even go as far as including the version in the python path too, as I don't think it would be crazy to think that a client might need to support with multiple versions at the same time, so src/frequenz/client/common/v1/microgrid/components/__init__.py and src/frequenz/client/common/v1/microgrid/components/battery.py for example.

I would not re-export symbols in src/frequenz/client/common/__init__.py, as this repo has many unrelated stuff I think it is better if uses just use the full path when importing.

I'd abstract away the version here because it's easy to support multiple on this level, compared to the protobuf level

Yeah, true. At some point we'll need to make breaking changes at this level too, but that's a broader discussion if we want to do something like using vX packages to allow users use multiple incompatible versions in the same code. I'm really starting to believe that could be a good idea in general... But again, separate discussion.

src/frequenz/client/common/_component.py

llucax · 2024-02-06T11:30:36Z

src/frequenz/client/common/_component.py

+    """A base class from which individual component types are derived."""
+
+
+# pylint: disable=no-member


It is better to use this scheme:

# pylint: disable=no-name-in-module import frequenz.api.common.components_pb2 import ComponentCategory # pylint: enable=no-name-in-module

Otherwise mistake trying to use non-existing names in a module will not be caught by mypy and will appear only in runtime.

For example: https://github.com/frequenz-floss/frequenz-sdk-python/blob/91821a3e2719024c0b2a114d6a11aa692d3429d6/src/frequenz/sdk/actor/power_distributing/_component_status/_battery_status_tracker.py#L12-L18

FYI @frequenz-floss/api-team

llucax · 2024-02-06T12:27:49Z

src/frequenz/client/common/_component.py

+        return hash(self.component_id)
+
+
+class ComponentMetricId(Enum):


Are these defined anywhere in the api-common repo? Or where do they come from?

Straight from the SDK...

What I am trying to say: I didn't check where anything came from, I just copied the file and removed Fuse-dependent stuff

OK, again, if we want to first commit the code copied from the SDK as is, then great, we can merge as is, but I would make it very clear in the commit message and PR description that this is the intention.

Nah, fuck that, I'll do it properly from the beginning

No, I think components/components.proto should be translated to components/__init__.py in here, so it should be frequenz.client.common.microgrid.components import ComponentCategory.

And how would the other proto files then be translated?

components/ battery.proto components.proto ev_charger.proto fuse.proto grid.proto inverter.proto transformer.proto

components/battery.py, etc. I think we just treat xxx/xxx.proto specially and have it as xxx/__init__.py instead.

As far as I see this pattern is used in other dirs too, for example microgrid/microgrid.proto, and usually the xxx/xxx.proto file uses other xxx/yyy.proto files and not the other way around.

@tiyash-basu-frequenz FYI, just to confirm that his pattern is used consciously and not just a coincidence. It would be important to have a consistent scheme to make the python modules make sense and make sure we don't run into cyclic dependencies.

In the case of microgrid/sensor/sensor.proto, where there is only that file in the microgrid/sensor/ directory, I would just add that file as a plain microgrid/sensor.py. If more files are added in the future we can rename sensor.py to sensor/init.py` and it will be transparent to the end user.

This is a pattern we consciously use (for protobuf), and this is a result of how we want to define packages in our protobuf definitions. microgrid/sensor/sensor.proto means that the entities in sensor.proto are a member of the package ...microgrid.sensor. The same applies to all other protobuf files.

For references, check these links: frequenz-floss/frequenz-api-common#127 (comment), frequenz-floss/frequenz-api-common#131

Marenz · 2024-02-06T16:23:55Z

I removed now everything that I don't really need for dispatch and put it in the right place in the structure.

llucax · 2024-02-08T08:36:26Z

src/frequenz/client/common/microgrid/__init__.py

+# License: MIT
+# Copyright © 2023 Frequenz Energy-as-a-Service GmbH
+
+"""Common code and utilities for Frequenz API clients."""


Maybe copy the module-level docs from https://github.com/frequenz-floss/frequenz-api-common/blob/v0.x.x/proto/frequenz/api/common/v1/microgrid/microgrid.proto?

It doesn't seem to be very useful to copy the same generic string on every module.

The proto module-level docs seem even more generic...

// Frequenz microgrid definition. // // Copyright 2023 Frequenz Energy-as-a-Service GmbH // // Licensed under the MIT License (the "License"); // you may not use this file except in compliance with the License.

to be fair, I think none of these init files seem very useful to me ;)

to be fair, I think none of these init files seem very useful to me ;)

I agree on this one

The proto module-level docs seem even more generic...

But the one in the proto at least has microgrid in it :D

Anyway, if in this case in the proto file there is nothing useful, all good, my main point is the documentation in here has to be at least as "good" as the one in the proto files.

llucax · 2024-02-08T08:37:59Z

src/frequenz/client/common/microgrid/components/components.py

-> src/frequenz/client/common/microgrid/components/__init__.py

llucax · 2024-02-08T08:39:54Z

src/frequenz/client/common/microgrid/components/components.py

+    """Possible types of microgrid component."""
+
+    NONE = PBComponentCategory.COMPONENT_CATEGORY_UNSPECIFIED
+    """Unspecified component category."""


Same about the documentation of all these items, we should use the same as in the protobuf stuff, or at least that at the minimum/base, maybe we can extend it with python-specific stuff too (or remove stuff that it is too protobuf-specific).

llucax · 2024-02-08T08:44:43Z

src/frequenz/client/common/microgrid/components/components.py

+    """CHP component."""
+
+
+def _component_category_from_protobuf(


Since we are starting from scratch here, I would add a @classmethod from_proto() to the enum as we do with classes.

I'm not sure if I mentioned this, but I was thinking that if the idea is not to expose the protobuf stuff, when it might be better to have the to/from proto conversion separate (even in a separate module, so protobuf stuff is not imported at all when only using the wrappers), but then for enums that would be super inconvenient because we wouldn't be able to use the same values as in the protobuf stuff, which could be very useful when debugging (or we'll need to duplicate everything manually), so I guess for a first version it might make more sense to keep it simple (and coupled).

Marenz · 2024-02-08T09:54:41Z

Added more docu, made to/from proto methods members.

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Marenz · 2024-02-12T11:22:28Z

Moved main types to init as requested

Add component state codes and error codes similar to #11

Marenz requested a review from a team as a code owner February 6, 2024 10:33

Marenz requested review from daniel-zullo-frequenz and removed request for a team February 6, 2024 10:33

github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) labels Feb 6, 2024

Marenz requested review from llucax and removed request for daniel-zullo-frequenz February 6, 2024 10:33

Marenz force-pushed the add_component_types branch from 7b68e85 to 9b9100a Compare February 6, 2024 10:44

github-actions bot removed the part:docs Affects the documentation label Feb 6, 2024

Marenz added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Feb 6, 2024

llucax changed the title ~~add component types~~ Add component types Feb 6, 2024

llucax reviewed Feb 6, 2024

View reviewed changes

Marenz force-pushed the add_component_types branch from 9b9100a to 98a34d1 Compare February 6, 2024 16:20

github-actions bot added the part:microgrid Affects the microgrid protobuf definitions label Feb 6, 2024

Marenz requested a review from llucax February 6, 2024 16:24

Marenz force-pushed the add_component_types branch 2 times, most recently from 5115eb5 to 0a3abd3 Compare February 6, 2024 16:55

llucax reviewed Feb 8, 2024

View reviewed changes

Marenz force-pushed the add_component_types branch from 0a3abd3 to ac9cac4 Compare February 8, 2024 09:54

Delete cookiecutter template leftovers

45dcd61

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Marenz force-pushed the add_component_types branch from ac9cac4 to 9d4b9bf Compare February 8, 2024 10:55

llucax mentioned this pull request Feb 9, 2024

Add pagination types #15

Merged

Add components types from SDK

d62dfa4

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Marenz force-pushed the add_component_types branch from 9d4b9bf to d62dfa4 Compare February 12, 2024 11:22

Marenz requested a review from llucax February 12, 2024 11:22

llucax approved these changes Feb 13, 2024

View reviewed changes

Marenz added this pull request to the merge queue Feb 13, 2024

Merged via the queue into frequenz-floss:v0.x.x with commit 5e6e7b7 Feb 13, 2024
14 checks passed

Marenz deleted the add_component_types branch February 13, 2024 09:23

flora-hofmann-frequenz mentioned this pull request Mar 5, 2024

Add common metric to test with reporting client mvp #21

Merged

cyiallou mentioned this pull request Nov 14, 2024

Add component states and error codes #44

Merged

github-merge-queue bot pushed a commit that referenced this pull request Nov 20, 2024

Add component states and error codes (#44)

13dadc8

Add component state codes and error codes similar to #11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add component types #11

Add component types #11

Marenz commented Feb 6, 2024 •

edited

Loading

llucax left a comment

llucax Feb 6, 2024

Marenz Feb 6, 2024

llucax Feb 8, 2024

llucax Feb 6, 2024

llucax Feb 6, 2024

Marenz Feb 6, 2024

Marenz Feb 6, 2024 •

edited

Loading

llucax Feb 6, 2024

Marenz Feb 6, 2024

llucax Feb 8, 2024

Marenz Feb 8, 2024

llucax Feb 8, 2024

llucax Feb 8, 2024

tiyash-basu-frequenz Feb 12, 2024 •

edited

Loading

Marenz commented Feb 6, 2024

llucax Feb 8, 2024

Marenz Feb 8, 2024

Marenz Feb 8, 2024

llucax Feb 8, 2024

llucax Feb 8, 2024

llucax Feb 8, 2024

llucax Feb 8, 2024

Marenz commented Feb 8, 2024

Marenz commented Feb 12, 2024

		"""A base class from which individual component types are derived."""


		# pylint: disable=no-member

		return hash(self.component_id)


		class ComponentMetricId(Enum):

Add component types #11

Add component types #11

Conversation

Marenz commented Feb 6, 2024 • edited Loading

llucax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marenz Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiyash-basu-frequenz Feb 12, 2024 • edited Loading

Choose a reason for hiding this comment

Marenz commented Feb 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marenz commented Feb 8, 2024

Marenz commented Feb 12, 2024

Marenz commented Feb 6, 2024 •

edited

Loading

Marenz Feb 6, 2024 •

edited

Loading

tiyash-basu-frequenz Feb 12, 2024 •

edited

Loading