-
Couldn't load subscription status.
- Fork 5
feat: ElectricalComponent #38
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
feat: ElectricalComponent #38
Conversation
3e84a78 to
ea8cfdb
Compare
3e9ed22 to
291f468
Compare
f768466 to
d98bb97
Compare
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.
Besides the comments, it LGTM in general although I didn't check in depth if there are more diversions compared to my base implementation of the dataclass wrappers.
src/frequenz/client/assets/electrical_component/_electrical_component.py
Outdated
Show resolved
Hide resolved
src/frequenz/client/assets/electrical_component/_electrical_component_json.py
Show resolved
Hide resolved
8ce9622 to
7ae1238
Compare
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.
OK, this looks good in general to me. I think most comments are FYI about ideas I have for the final version in client-common, but there are a few comments about a couple of small issues.
About the commit split, I think by now the split makes this PR (and the git history) harder to read, as not only there a lot of commits, but also commits are not very isolated, they look more like checkpoints while editing files, which makes it very hard to review and very easy to get lost among the unrelated changes.
I would strongly suggest to improve the commit splitting before merging. I don't think we need a lot of commits here, as 95% of the changes here is just creating new files, and all of that is probably fine to include in just one commit.
So splitting in 2 commits, one adding all the new files, and then a second commit adding the new client method, for me would be a huge improvement already.
Ideally I would split a bit more, also not a lot, but something like:
- First commit adding the new files verbatim as you copy them from the microgrid API client, using the old common version.
- Copy all the files in the current state overwriting those original files. With this we get one commit where we can clearly see all the changes that were necessary to go from common API pre-v0.8 to v0.8, which will be very useful when migrating the microgrid API client too.
- Unrelated fixes, like re-exporting the
PermissionDeniedexception (ideally one per unrelated change, but I'm also fine to do them all in one commit if it makes things easier). - Add the new client method.
- Add the tests for the new client method.
- Add the
get_microgrid_detailsmethod test, or even leave those for a separate PR, but not sure, maybe you need to include them here if you are removing old tests for that method). If splitting is complicated, also one commit with all test-related changes would be fine too.
| @enum.unique | ||
| class BatteryType(enum.Enum): | ||
| """The known types of batteries.""" | ||
|
|
||
| UNSPECIFIED = electrical_components_pb2.BATTERY_TYPE_UNSPECIFIED | ||
| """The battery type is unspecified.""" | ||
|
|
||
| LI_ION = electrical_components_pb2.BATTERY_TYPE_LI_ION | ||
| """Lithium-ion (Li-ion) battery.""" | ||
|
|
||
| NA_ION = electrical_components_pb2.BATTERY_TYPE_NA_ION | ||
| """Sodium-ion (Na-ion) battery.""" |
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.
I'm thinking about removing these enums in the final version for client-common. With the current approach we can easily specify battery/inverter/etc. types and different component categories using Python classes, so providing the enums too doesn't add any value and introduces more ways to do the same thing, which can be confusing for users.
We don't need to change anything else, but just FYI and to know your opinion, in case I'm missing something.
| type: BatteryType | int | ||
| """The type of this battery. |
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.
If/when we remove the enum, this field will be gone here, and only added to UnrecognizedBattery as just type: int.
Again, just FYI.
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 encoder ended up being pretty generally useful! I think we should definitely move this encoder to frequenz-core/frequenz-client-common in the future.
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.
| ): self._encode(value) | ||
| for key, value in o.items() | ||
| } | ||
| if isinstance(o, (list, tuple, set, frozenset)): |
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.
nitpick:
| if isinstance(o, (list, tuple, set, frozenset)): | |
| elif isinstance(o, (list, tuple, set, frozenset)): |
|
Oh, and BTW and just for the records, I didn't review much of the |
…utilities This commit introduces a comprehensive set of classes and functions for managing electrical components within the microgrid framework. Key additions include: - Definitions for various electrical components such as batteries, inverters, chargers, and more, each with specific attributes and categories. - Protobuf message loading functions to convert messages into corresponding Python objects. - JSON encoding utilities for serializing component data for API interactions. These changes lay the groundwork for enhanced functionality and integration within the microgrid system. Signed-off-by: eduardiazf <[email protected]>
This commit includes the following changes: - Updated `setuptools_scm` and various dependencies in `pyproject.toml` to their latest versions. - Changed the GitHub Actions checkout action from version 4 to 5 in CI workflows. - Removed unused test files and helper functions related to the Assets API client, including type definitions and mock data. These updates enhance the project's dependency management and streamline the testing setup. Signed-off-by: eduardiazf <[email protected]>
…improvements This commit introduces several key updates to the Assets API client, including: - New method `list_microgrid_electrical_components()` for retrieving electrical components in a microgrid. - Updated CLI command `assets-cli electrical-components <microgrid-id>` for displaying electrical components. - Improved error handling and type safety in the client. - Enhanced JSON encoding for better serialization of enum keys. - Added new exceptions for better error management. These changes significantly enhance the functionality and usability of the Assets API client within the Frequenz microgrid framework. Signed-off-by: eduardiazf <[email protected]>
This commit includes several updates to the Assets API client tests and utility functions: - Refactored test structure for better organization and clarity, including renaming test files and updating docstrings. - Introduced new test cases for retrieving microgrid details and electrical components, enhancing coverage for the Assets API client. - Added utility functions to streamline test case management and improve readability. - Updated the JSON encoder logic in the AssetsJSONEncoder class for better handling of container types. These changes enhance the maintainability and robustness of the testing framework for the Assets API client. Signed-off-by: eduardiazf <[email protected]>
4d50f08 to
24d835c
Compare
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.
Awesome 🎉
Thanks for the patience! I will create a follow-up issues with pending comments where appropriate.
Signed-off-by: Eduardo Diaz <[email protected]>
The merge-base changed after approval.
Summary
This PR introduces comprehensive support for electrical components in the Frequenz Assets API client, including new data types, enums, CLI commands, and enhanced serialization capabilities.
Changes Made
🆕 New Features
ElectricalComponentclass with support for batteries, EV chargers, inverters, grid connection points, and power transformersBattery,EvCharger,Inverter,GridConnectionPoint,PowerTransformer)Metricenum supporting various electrical measurements (DC voltage/current/power, AC frequency/voltage, phase-to-phase voltages)Lifetimeclass for tracking component operational periodsMetricConfigBoundsandBoundclasses for component metric limits🔧 Enhanced Functionality
get_electrical_components()method to retrieve electrical components from a microgridelectrical-componentscommand for querying component dataMicrogridandElectricalComponentobjects📚 Documentation & CLI
🔄 Dependencies
frequenz-client-commonfrom>=0.3.2to>=0.3.6Examples
CLI Usage
Query electrical components for a specific microgrid:
Sample Output
[ { "id": 414, "microgrid_id": 226, "name": "battery1", "category": 5, "manufacturer": "Commeo", "model_name": "Commeo HV", "operational_lifetime": { "start": null, "end": null }, "rated_bounds": { "102": { "lower": 9.0, "upper": 45.0 }, "101": { "lower": 5.0, "upper": 95.0 } }, "type": 1 }, { "id": 415, "microgrid_id": 226, "name": "Grid1", "category": 1, "manufacturer": null, "model_name": null, "operational_lifetime": { "start": null, "end": null }, "rated_bounds": {}, "rated_fuse_current": 10 }, { "id": 416, "microgrid_id": 226, "name": "Relay1", "category": 7, "manufacturer": null, "model_name": null, "operational_lifetime": { "start": null, "end": null }, "rated_bounds": {} } ]Testing
Breaking Changes
None - this is a purely additive feature that extends existing functionality.