-
Notifications
You must be signed in to change notification settings - Fork 20
Update to use the microgrid API v0.18 #1283
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
base: v1.x.x
Are you sure you want to change the base?
Conversation
60cf5b2 to
a996c07
Compare
llucax
left a comment
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 think these are all the TODOs/FIXMEs I wrote when doing the update. It would be good to get some feedback.
src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py
Outdated
Show resolved
Hide resolved
src/frequenz/sdk/microgrid/_power_distributing/_component_status/_battery_status_tracker.py
Outdated
Show resolved
Hide resolved
shsms
left a comment
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.
Only read a little bit of the big commit
| and bool( | ||
| { | ||
| ComponentStateCode.EV_CHARGING_CABLE_LOCKED_AT_EV, | ||
| ComponentStateCode.EV_CHARGING_CABLE_LOCKED_AT_STATION, | ||
| } | ||
| & self.states | ||
| or { | ||
| ComponentStateCode.EV_CHARGING_CABLE_PLUGGED_AT_EV, | ||
| ComponentStateCode.EV_CHARGING_CABLE_PLUGGED_AT_STATION, | ||
| } | ||
| & self.states |
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.
we probably need to drop the *_AT_STATION codes. Cable connected to the station doesn't mean the other end of the cable has a car attached.
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.
But this is checking that is connected at both ends, the EV and the station. it seems like the new API is splitting the LOCKED and PLUGGED into both ends, so the intention here was that we make sure both ends are connected to return true for is_ev_connected.
@tiyash-basu-frequenz maybe can also help here.
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.
Thinking a bit more maybe you mean it is redundant because ia EV charging station can only know the EV end is connected if the station end is also connected? Also I was thinking maybe I should add more variables to make it more readable:
has_errors = ComponentStateCode.ERROR not in self.states
is_authorized = (
ComponentErrorCode.UNAUTHORIZED not in self.errors
and ComponentErrorCode.UNAUTHORIZED not in self.warnings
)
both_ends_are locked = bool(
{
ComponentStateCode.EV_CHARGING_CABLE_LOCKED_AT_EV,
ComponentStateCode.EV_CHARGING_CABLE_LOCKED_AT_STATION,
}
& self.states)
both_ends_are_plugged = bool(
{
ComponentStateCode.EV_CHARGING_CABLE_PLUGGED_AT_EV,
ComponentStateCode.EV_CHARGING_CABLE_PLUGGED_AT_STATION,
}
& self.states)
return not has_errors and is_authorized and (both_ends_are_locked or both_ends_are_plugged)This also makes me think that I'm not sure if a combination of "plugged at EV-locked at station" should also not be considered as "connected".
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.
After talking to @tiyash-basu-frequenz I will update to:
def is_ev_connected(self) -> bool:
"""Check whether an EV is connected to the charger.
Returns:
When the charger is not in an error state, whether an EV is connected to
the charger.
"""
has_error = ComponentStateCode.ERROR not in self.states
is_authorized = (
ComponentErrorCode.UNAUTHORIZED not in self.errors
and ComponentErrorCode.UNAUTHORIZED not in self.warnings
)
is_connected_at_ev = bool(
{
ComponentStateCode.EV_CHARGING_CABLE_LOCKED_AT_EV,
ComponentStateCode.EV_CHARGING_CABLE_PLUGGED_AT_EV,
}
& self.states
)
is_connected_at_station = bool(
{
ComponentStateCode.EV_CHARGING_CABLE_LOCKED_AT_STATION,
ComponentStateCode.EV_CHARGING_CABLE_PLUGGED_AT_STATION,
}
& self.states
)
return (
not has_error
and is_authorized
and is_connected_at_ev
and is_connected_at_station
)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.
There's likely only one cable-state. So it would either say LOCKED_AT_STATION (meaning there's cable, but no car) or LOCKED_AT_EV (meaning there's car, and therefore cable).
So we can't use AT_STATION states for this check.
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.
Again, question for @tiyash-basu-frequenz as what I understood from the meeting with him is in v0.18 we should check for both.
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, in that case, the new and and and function looks good.
src/frequenz/sdk/microgrid/_power_distributing/_component_status/_battery_status_tracker.py
Outdated
Show resolved
Hide resolved
src/frequenz/sdk/microgrid/_power_distributing/_component_status/_battery_status_tracker.py
Outdated
Show resolved
Hide resolved
src/frequenz/sdk/microgrid/_power_distributing/_component_status/_battery_status_tracker.py
Outdated
Show resolved
Hide resolved
src/frequenz/sdk/microgrid/_power_distributing/_component_status/_ev_charger_status_tracker.py
Outdated
Show resolved
Hide resolved
2ed4884 to
c71fd23
Compare
shsms
left a comment
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.
Still got a number of files to go through.
| active_power=0.0, | ||
| component_state=EVChargerComponentState.READY, | ||
| cable_state=EVChargerCableState.EV_PLUGGED, | ||
| ) | ||
| states={ | ||
| ComponentStateCode.READY, | ||
| ComponentStateCode.EV_CHARGING_CABLE_PLUGGED_AT_EV, | ||
| ComponentStateCode.EV_CHARGING_CABLE_PLUGGED_AT_STATION, | ||
| }, | ||
| ).to_samples() |
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.
cable plugged at EV necessarily means cable LOCKED at station.
And it was not the case with the service to send two states for the cable, and I don't think it needs to change now, it is already obvious, so we'd just be sending redundant information.
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.
Apparently it already changed, at least this is what I understood from a meeting with @tiyash-basu-frequenz
| component_state=EVChargerComponentState.READY, | ||
| cable_state=EVChargerCableState.EV_LOCKED, | ||
| ) | ||
| states={ | ||
| ComponentStateCode.READY, | ||
| ComponentStateCode.EV_CHARGING_CABLE_PLUGGED_AT_EV, | ||
| ComponentStateCode.EV_CHARGING_CABLE_LOCKED_AT_STATION, | ||
| }, |
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 is not an exact match.
Old says EV LOCKED. new says EV PLUGGED.
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.
Oh, good catch! Will fix.
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.
The test still passed because actually all permutations are considered as connected I guess.
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.
(fixed anyway)
|
I'm working on splitting the big WIP commit, so maybe you want to wait for that before you continue with the review. |
c71fd23 to
7281f4d
Compare
7281f4d to
ecbd757
Compare
|
OK, this should be ready for a final review, I split the big WIP commit into smaller commits. The commit split is still not the best, but it should be much more reviewable now. I left a few unrelated changes/improvements (like the removal of unreachable code) for a follow-up PR with just cleanups/improvements that are not strictly part of the v0.18 update (although there still a few unrelated commits in this PR too). |
56f5263 to
b3e35e7
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.
Pull Request Overview
This PR migrates the SDK from the old frequenz-client-microgrid API to a new unified streaming API. The main changes involve updating component models, metrics, and data fetching mechanisms to use the new client library structure.
Key Changes
- Replaces the old component model (
Component,Connection,ComponentCategory) with the new structured component types (GridConnectionPoint,BatteryInverter,Meter, etc.) andComponentConnection - Migrates from
ComponentMetricIdto the newMetricenum - Updates data streaming from type-specific methods (e.g.,
battery_data(),inverter_data()) to a unifiedreceive_component_data_samples_stream()method - Introduces
ComponentDataSamplesas the unified data model replacing type-specific data classes - Updates component graph filtering from category-based to type-based filtering (
matching_typesinstead ofcomponent_categories)
Reviewed Changes
Copilot reviewed 77 out of 78 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils/mock_microgrid_client.py | Refactored mock client to use new unified streaming API with ComponentDataSamples |
| tests/utils/graph_generator.py | Updated to create specific component types instead of generic Components |
| tests/utils/component_graph_utils.py | Updated to use new component types and connections |
| tests/utils/component_data_wrapper.py | Migrated from old state/error enums to new ComponentStateCode/ErrorCode |
| tests/utils/component_data_streamer.py | Updated to send ComponentDataSamples via to_samples() |
| tests/timeseries/*.py | Updated metric references from ComponentMetricId to Metric |
| tests/microgrid/*.py | Comprehensive updates to component creation and graph operations |
| src/frequenz/sdk/timeseries/*.py | Updated formula generators and streaming to use new Metric enum |
| src/frequenz/sdk/timeseries/battery_pool/*.py | Migrated battery pool to use new metrics and data fetching |
| src/frequenz/sdk/microgrid/_data_sourcing/*.py | Updated data sourcing to export Metric instead of ComponentMetricId |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py
Show resolved
Hide resolved
pyproject.toml
Outdated
| "frequenz-client-microgrid >= 0.9.0, < 0.10.0", | ||
| "frequenz-client-common >= 0.3.2, < 0.4.0", | ||
| #"frequenz-client-microgrid >= 0.18.0, < 0.19.0", | ||
| "frequenz-client-microgrid @ git+https://github.com/frequenz-floss/frequenz-client-microgrid-python@refs/heads/v0.18.x", |
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.
Is there a PR for the client already? Can't find one.
b3e35e7 to
ce1ee07
Compare
|
Updated the dependency, I think it would be best to merge when the review is ready, so marking as ready. I just realized I also need to update the release notes. |
|
Super, you can remove WIP from the commit message as well. I will continue reviewing now. |
This introduces the new microgrid API v0.18. We also need to bump the frequenz-client-common dependency to at least v0.3.6, so it can work with the new microgrid client. Signed-off-by: Leandro Lucarella <[email protected]>
The microgrid API now doesn't support not reporting a rated fuse for a grid connection, so we don't need to test for that case anymore. Signed-off-by: Leandro Lucarella <[email protected]>
The battery pool bounds calculation is buggy and these tests are wrong (see frequenz-floss#1180). By switching to using ranges of bounds, the buggy behaviour changes and make these tests fail. Fixing them is difficult without switching to using ranges natively first, so we just skip these tests for now. Signed-off-by: Leandro Lucarella <[email protected]>
This will help us to make sure all sub-classes are properly updated when doing changes to method signatures. Signed-off-by: Leandro Lucarella <[email protected]>
The new API doesn't return component data packets with one timestamp and several metrics anymore, so we write an adaptation layer to convert individual metrics to `ComponentData` wrappers so we don't need to update every location where streaming data is being consumed. Signed-off-by: Leandro Lucarella <[email protected]>
This change updates the SDK to use the new `Metric` enum from `frequenz.client.microgrid.metrics` (and the new `TransitionalMetric`) in place of the old `ComponentMetricId`. It adapts internal types and mappings to accept `Metric | TransitionalMetric` where appropriate. Imports, request/channel naming, and data-extraction maps across the microgrid data sourcing and timeseries modules (battery pool, voltage/ frequency streamers, formula engine, etc.) have been updated to the new metric names (for example `ACTIVE_POWER` → `AC_ACTIVE_POWER`, etc.), and a `TransitionalMetric` shim is used to preserve compatibility for metrics that are still referenced by older code paths. Tests and benchmarks were updated accordingly. Signed-off-by: Leandro Lucarella <[email protected]>
For non data-streaming methods, update to use the new client method names. For data-streaming methods, use the new adaptation functions in `_old_component_data.py`. Because `set_component_power_active()` can now return a the time when the command expires, we also need to update the tests we use to run them. Note that the data-streaming method in the new client version is not async (it returns a Receiver synchronously) so `await`s are also removed. For the `ComponentMetricFetcher` we also remove the `async` for the wrapper `_subscribe()` methods. The mock microgrid client was mostly rewritten to adjust it to the new unified data streaming method. Signed-off-by: Leandro Lucarella <[email protected]>
API is too generic, a more specific name makes the code more clear. Signed-off-by: Leandro Lucarella <[email protected]>
The new `Component` class renamed `component_id` with `id`, so we need to update all our references. Signed-off-by: Leandro Lucarella <[email protected]>
The name in the microgrid API will be changed again to `Microgrid` in the near future, so internally we just use `microgrid` instead of `microgrid_info`. Signed-off-by: Leandro Lucarella <[email protected]>
The new API client provides a mapping of `ComponentCategory` and `XxxType`s to Python classes. This commit updates most uses of component categories with these classes. This also includes some updates of imports for `Component` and `ComponentCategory` and some changes on specific component type, like `GridConnectionPoint`'s metadata. Finally, the component graph interface to get components was also updated to use types instead of categories, for which the API was slightly changed to better represent the filtering criteria. The keyword for arguments were changed from `component_ids` to `matching_ids` and `component_categories` to `matching_types`. Signed-off-by: Leandro Lucarella <[email protected]>
While we are changing the interface of component retrieval in the component graph, we take the opportunity to make its interface more generic. We now accept any Iterable for `matching_ids` and `matching_types`, as well as one individual items. Most uses of it filter by a single item anyway so it is annoying to have to wrap it in a Iterable. Most uses of those methods were updated to use the single-item overload. Signed-off-by: Leandro Lucarella <[email protected]>
The new API client can return also an `int` as a component category when the client doesn't know the category number. Signed-off-by: Leandro Lucarella <[email protected]>
To match the new component graph filtering arguments for `components()`, the `connections()` method now also have its argument names updated: * `start` -> `matching_sources` * `end` -> `matching_destinations` Signed-off-by: Leandro Lucarella <[email protected]>
Like with component graph `components()`, now we support passing a single element to `matching_sources` and `matching_destinations`. The `components()` function is also simplified to make filtering more compact. Also updates uses with a single element to remove the unnecessary container. Signed-off-by: Leandro Lucarella <[email protected]>
Many tests build mock components, so now this components need to be created as concrete subclasses of `Component`. Some test also has some minor readability improvements. Signed-off-by: Leandro Lucarella <[email protected]>
The new state snapshots in the v0.18 client provide errors and warnings using a different structure, so we need to account for that. State code were also unified and there is no more specific enums for cable state and other component-specific states, all states are part of the `ComponentStateCode` enum and the component data can have many state codes in it, while before it contained only one. In previous versions EV chargers only used one `PLUGGED` or `LOCKED` status but now the cable status was split in 2 connector ends: station and EV, so now all combinations of those need to be accounted for. This might change again on the API side as it ended up a bit messy. Also the old component data wrappers now can't be frozen because the `ComponentData` class is not frozen (this is to allow parsing from the new telemetry messages easier for phases). Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
ce1ee07 to
3e08f5b
Compare
This PR migrates the SDK from the old
frequenz-client-microgridusing the old microgrid API v0.15 to the new using the API v0.18. The main changes involve updating component models, metrics, and data fetching mechanisms to use the new client library structure.