Skip to content
Merged
Show file tree
Hide file tree
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
60 changes: 30 additions & 30 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,4 @@ crate-type = ["cdylib"]

[dependencies]
pyo3 = "0.29.0"
frequenz-microgrid-component-graph = "0.5"
frequenz-microgrid-component-graph = "0.6.0"
10 changes: 10 additions & 0 deletions RELEASE_NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,16 @@

- Bumped PyO3 to 0.29 to pull in the fix for RUSTSEC out-of-bounds read advisory GHSA-36hh-v3qg-5jq4 (`nth`/`nth_back` on list/tuple iterators).

- Updated the `frequenz-microgrid-component-graph` Rust crate to v0.6.0. This changes some outputs that are visible from Python:

- The formula fallback engine was rewritten. Formulas can now contain meter-subtraction and diamond terms. Exact formula strings can differ from the previous release, even for the same graph.

- The consumer formula now measures the non-consumer components behind one internal meter as one group: it subtracts `COALESCE(#meter, component readings...)` instead of each component on its own. Note: if such a meter also carries a load that is not in the component graph, that load is now subtracted together with the group while the meter reading is used.

- Graph validation failures now raise `InvalidGraphError` with a message that starts with `Graph validation failed:`, followed by one indented line per problem. The old format was `InvalidGraph: <description>`. Note: some errors found while building the graph, for example a missing grid component, still use the old format. Code that matches on the message text of validation failures must be updated.

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.

🔵 New InvalidGraphError message format is documented as breaking but untested

This release note flags a user-facing breaking change: graph validation failures now raise InvalidGraphError with a message starting Graph validation failed: (one indented line per problem), replacing the old InvalidGraph: <description> format, and explicitly warns that "Code that matches on the message text of validation failures must be updated."

No test pins this new format. The only related test, tests/test_microgrid_component_graph.py:466, asserts the exception type only (pytest.raises(InvalidGraphError)), not the message. A future crate bump could silently change the message again with nothing to catch it.

Impact

A behavior the PR itself calls out as breaking for downstream code has no regression guard. Since the message is now a documented contract, drift would go unnoticed until it breaks a consumer.

Related locations

Suggested tests

Build a graph that fails validation (rather than fails at graph construction) and assert the message starts with Graph validation failed:, so the documented format is pinned.


🔵 Low Severity: nice-to-have coverage for a documented breaking behavior.


- `prefer_meters_in_component_formulas` now defaults to `False`: per-category formulas use the component reading as the primary source and the meter as the fallback. Set it to `True` to restore the old meter-first order. This changes only the order of the sources; the new fallback terms from v0.6.0 are used either way.

## New Features

<!-- Here goes the main new features and examples or instructions on how to use them -->
Expand Down
21 changes: 13 additions & 8 deletions python/frequenz/microgrid_component_graph/__init__.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ class ComponentGraphConfig:
allow_unspecified_inverters: bool = False,
disable_fallback_components: bool = False,
include_phantom_loads_in_consumer_formula: bool = False,
prefer_meters_in_component_formulas: bool = True,
prefer_meters_in_component_formulas: bool = False,
formula_overrides: FormulaOverrides | None = None,
) -> None:
"""Initialize this instance.
Expand All @@ -49,26 +49,31 @@ class ComponentGraphConfig:
the measurements of successor meters from the measurements of their
predecessor meters. When `false`, consumer formula is generated by
excluding production and battery components from the grid measurements.
The non-consumer components behind one internal meter are excluded
as one group: the meter reading first, the component readings as
the fallback. While the meter reading is used, a load behind

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.

🔵 Docstring: meter-subtraction note under the "when false" branch reads as contradictory

This grouping description is placed inside the include_phantom_loads_in_consumer_formula "When false" branch (the mode that, per the line just above, generates the consumer formula without including phantom loads). Yet it then states that "a load behind that meter that is not in the component graph is then also excluded together with the group" — i.e. a phantom load being subtracted in the very mode that is supposed to exclude phantom-load handling.

This is the exact subtlety the release note calls out (RELEASE_NOTES.md:15), and it is accurate, but as written under the "when false" bullet it reads as self-contradictory and is easy to misread.

Impact

Readers configuring include_phantom_loads_in_consumer_formula may be confused about which loads are actually included/excluded in the default (False) mode.

Suggested fix

Reword to make explicit that this is a side effect of grouping-by-meter (the meter reading is used as the primary term, so anything behind that meter — including an off-graph load — is captured by that single term), not the phantom-load feature being partially enabled.


🔵 Low Severity: documentation clarity only.

that meter that is not in the component graph is then also
excluded together with the group.
prefer_meters_in_component_formulas: Default policy for the per-category
formulas. When `True` (the default), the meter measurement is the
primary source and the device measurement is the fallback for
formulas. When `False` (the default), the component measurement is the
primary source and the meter measurement is the fallback for
`battery_formula`, `chp_formula`, `pv_formula`, `wind_turbine_formula`,
`ev_charger_formula`, and `steam_boiler_formula`. When `False`, the
device is primary and the meter is the fallback. Has no effect on
`ev_charger_formula`, and `steam_boiler_formula`. When `True`, the
meter is primary and the component is the fallback. Has no effect on
`grid_formula`, `consumer_formula`, `producer_formula`, or any of the
coalesce formulas.
formula_overrides: Per-formula overrides for the meter/device preference;
formula_overrides: Per-formula overrides for the meter/component preference;
see `FormulaOverrides`. Each entry, when set, takes precedence over
`prefer_meters_in_component_formulas` for that formula.
"""

class FormulaOverrides:
"""Per-formula overrides for the meter/device preference.
"""Per-formula overrides for the meter/component preference.

Each parameter is `None` by default, meaning the corresponding formula
follows the global `prefer_meters_in_component_formulas` setting on
`ComponentGraphConfig`. Setting `True` forces the meter as primary
for that formula; `False` forces the device.
for that formula; `False` forces the component.
"""

def __init__(
Expand Down
2 changes: 1 addition & 1 deletion src/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ impl ComponentGraphConfig {
allow_unspecified_inverters = false,
disable_fallback_components = false,
include_phantom_loads_in_consumer_formula = false,
prefer_meters_in_component_formulas = true,
prefer_meters_in_component_formulas = false,
formula_overrides = None,
))]
fn new(
Expand Down
34 changes: 17 additions & 17 deletions tests/test_formula_preferences.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@

"""Behavioral tests for `ComponentGraphConfig` formula preferences.

These tests build a small, controllable graph (Grid -> Meter -> Device)
These tests build a small, controllable graph (Grid -> Meter -> Component)
for each per-category formula method and assert the actual formula
output for the four meter/device-preference combinations:
output for the four meter/component-preference combinations:

* default config -> meter primary
* global False -> device primary
* default config -> component primary
* global True -> meter primary
* per-formula override = True -> meter primary (override wins)
* per-formula override = False -> device primary (override wins)
* per-formula override = False -> component primary (override wins)

The drift test in `test_stub_drift.py` only checks signatures; this
file catches actual mis-wiring -- a swapped override, an inverted
Expand Down Expand Up @@ -46,9 +46,9 @@
_MGRID = MicrogridId(1)

# In every per-category topology built below, component #2 is the meter
# and #3 is the device that appears in the formula.
# and #3 is the component that appears in the formula.
_METER_PRIMARY = "COALESCE(#2, #3, 0.0)"
_DEVICE_PRIMARY = "COALESCE(#3, #2, 0.0)"
_COMPONENT_PRIMARY = "COALESCE(#3, #2, 0.0)"

# Each per-category graph builder takes an optional `config` and returns a
# graph rooted at the same Grid -> Meter pair, so the assertion strings
Expand Down Expand Up @@ -104,7 +104,7 @@ def _battery_graph(
config: ComponentGraphConfig | None = None,
) -> ComponentGraph[Any, Any, Any]:
# Grid -> Meter -> BatteryInverter -> Battery; the formula references
# the inverter (#3) as the device, the battery (#4) doesn't appear.
# the inverter (#3) as the component, the battery (#4) doesn't appear.
return ComponentGraph(
components={
_grid(),
Expand Down Expand Up @@ -190,26 +190,26 @@ def _steam_boiler_graph(


@pytest.mark.parametrize("build_graph,method,override_field", _CATEGORIES)
def test_default_config_prefers_meter(
def test_default_config_prefers_component(
build_graph: GraphBuilder,
method: str,
override_field: str, # pylint: disable=unused-argument
) -> None:
"""Default config selects the meter as the primary source."""
"""Default config selects the component as the primary source."""
formula = getattr(build_graph(None), method)(None)
assert formula == _METER_PRIMARY
assert formula == _COMPONENT_PRIMARY


@pytest.mark.parametrize("build_graph,method,override_field", _CATEGORIES)
def test_global_false_prefers_device(
def test_global_true_prefers_meter(
build_graph: GraphBuilder,
method: str,
override_field: str, # pylint: disable=unused-argument
) -> None:
"""Setting `prefer_meters_in_component_formulas=False` selects the device."""
config = ComponentGraphConfig(prefer_meters_in_component_formulas=False)
"""Setting `prefer_meters_in_component_formulas=True` selects the meter."""
config = ComponentGraphConfig(prefer_meters_in_component_formulas=True)
formula = getattr(build_graph(config), method)(None)
assert formula == _DEVICE_PRIMARY
assert formula == _METER_PRIMARY


@pytest.mark.parametrize("build_graph,method,override_field", _CATEGORIES)
Expand All @@ -229,13 +229,13 @@ def test_override_true_wins_over_global_false(
def test_override_false_wins_over_global_true(
build_graph: GraphBuilder, method: str, override_field: str
) -> None:
"""A `False` per-formula override flips to device despite a `True` global."""
"""A `False` per-formula override flips to component despite a `True` global."""
config = ComponentGraphConfig(
prefer_meters_in_component_formulas=True,
formula_overrides=FormulaOverrides(**{override_field: False}),
)
formula = getattr(build_graph(config), method)(None)
assert formula == _DEVICE_PRIMARY
assert formula == _COMPONENT_PRIMARY


def _empty_graph() -> ComponentGraph[Any, Any, Any]:
Expand Down
40 changes: 37 additions & 3 deletions tests/test_microgrid_component_graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,11 @@ def test_wind_turbine_graph() -> None:
}

# 3. Test Formula Generation
# References the Meter (ID 2) measuring the Turbine (ID 3).
# Component-first by default: the turbine (ID 3) is the primary source,
# the meter (ID 2) the fallback.
assert (
graph.wind_turbine_formula(wind_turbine_ids={ComponentId(3)})
== "COALESCE(#2, #3, 0.0)"
== "COALESCE(#3, #2, 0.0)"

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.

🟡 Broken intermediate commit: default-flip test expectations land before the flip

Commit df7d2cc (Build against component-graph v0.6.0) updates the test_wind_turbine_graph and test_steam_boiler_graph assertions from meter-first COALESCE(#2, #3, 0.0) to component-first COALESCE(#3, #2, 0.0). However, the actual default flip (prefer_meters_in_component_formulas truefalse in src/graph.rs and __init__.pyi) happens only in the next commit cdcb3cf (Default to component-first per-category formulas).

At df7d2cc the runtime default is still meter-first, so:

  • these two updated assertions expect component-first output that the binding does not yet produce, and
  • tests/test_formula_preferences.py::test_default_config_prefers_meter (unchanged at that commit) still asserts the default is meter-first.

The two files directly contradict each other and the test suite is red at df7d2cc. The commit message also states it updates the tests "to match the new default", but that default is not set in this commit.

Impact

git bisect can land on a commit with a failing suite, and anyone checking out df7d2cc in isolation sees failures unrelated to their change. The commit message does not match the diff. The final merged state is correct (CI is green on HEAD), so this only affects history quality — harmless under squash-merge, but a real hazard if commits are preserved.

Related locations

Suggested fix

Move the wind/steam expectation updates (this hunk and the steam one) into the default-flip commit cdcb3cf, or move the graph.rs/stub default flip into df7d2cc, so each commit is self-consistent and its suite is green. Adjust the df7d2cc message accordingly.


🟡 Medium Severity: does not affect the merged result, but breaks bisectability and the commit message is inaccurate; best fixed before merge unless the PR is squash-merged.

)

# 4. Test Topology (Successors/Predecessors)
Expand Down Expand Up @@ -179,10 +180,43 @@ def test_steam_boiler_graph() -> None:
}
assert (
graph.steam_boiler_formula(steam_boiler_ids={ComponentId(3)})
== "COALESCE(#2, #3, 0.0)"
== "COALESCE(#3, #2, 0.0)"
)


def test_consumer_formula_meter_subtraction() -> None:

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.

🔵 consumer_formula meter-subtraction: only the clean happy path is covered

This new test pins the clean grouping MAX(#2 - COALESCE(#3, #4, 0.0), 0.0), which is good. But the release notes and the stub docstring highlight a more consequential, subtle behavior of the new grouping: when the internal meter also carries a load that is not in the component graph, that off-graph load is now subtracted together with the group (because the meter reading is the primary term). That semantic change — the whole point of "meter-subtraction as one group" — is not exercised by any test.

Impact

The most user-visible risk of the v0.6.0 rewrite (a phantom load being netted into a producer group) is undocumented by tests, so a future regression in that grouping would pass CI.

Suggested tests

Add a case where meter #3 has both the PV inverter and a second successor that maps to an off-graph/phantom load, and assert the resulting consumer formula groups them under the single COALESCE(#3, ...) term as documented.


🔵 Low Severity: additional edge-case coverage for the headline behavior of this release.

"""Test the consumer formula's meter-subtraction grouping.

The non-consumer components behind one internal meter are subtracted
as one group: the meter reading first, the component readings as the
fallback.
"""
graph: microgrid_component_graph.ComponentGraph[
Component, ComponentConnection, ComponentId
] = microgrid_component_graph.ComponentGraph(
components={
GridConnectionPoint(
id=ComponentId(1),
microgrid_id=MicrogridId(1),
rated_fuse_current=100,
),
Meter(id=ComponentId(2), microgrid_id=MicrogridId(1)),
Meter(id=ComponentId(3), microgrid_id=MicrogridId(1)),
SolarInverter(id=ComponentId(4), microgrid_id=MicrogridId(1)),
},
connections={
# Grid -> Grid Meter -> PV Meter -> PV Inverter
ComponentConnection(source=ComponentId(1), destination=ComponentId(2)),
ComponentConnection(source=ComponentId(2), destination=ComponentId(3)),
ComponentConnection(source=ComponentId(3), destination=ComponentId(4)),
},
)

# The PV group behind meter #3 is subtracted from the grid meter #2 as
# one COALESCE term, and consumption is clamped at zero.
assert graph.consumer_formula() == "MAX(#2 - COALESCE(#3, #4, 0.0), 0.0)"


def test_relay_is_passthrough() -> None:
"""`Relay` maps to cg's `Breaker`, a pass-through category.

Expand Down
Loading
Loading