Build against component-graph v0.6.0#91
Conversation
Update the frequenz-microgrid-component-graph dependency to v0.6.0. This release rewrites the formula fallback engine and adds the component-first default for per-category formulas: the component reading is the primary source and the meter is the fallback. Update the steam-boiler and wind-turbine formula tests to match the new default, add a test that pins the consumer formula's new meter-subtraction grouping, and document the Python-visible changes (formula terms, consumer formula grouping, validation error format) in the release notes and in the phantom-loads stub docstring. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
component-graph flipped the prefer_meters_in_component_formulas default to false (component reading primary, meter as fallback). Mirror it in the binding so an explicit ComponentGraphConfig() matches both the no-config path and the upstream default, instead of forcing the old meter-first behavior. Update the type stub and the preference tests. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
Pure terminology: the per-category formulas' non-meter source is a component, matching component-graph's vocabulary (the flag is prefer_meters_in_component_formulas). Rename _DEVICE_PRIMARY and the "device" wording in the type stub and preference tests to "component"; no behavior change. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
github-actions
Bot
added
part:docs
The stub-drift test compared only method and parameter names, so a default changed on one side only (the Rust pyo3 signature or the type stub) would pass CI while type checkers report the wrong default. The prefer_meters_in_component_formulas flip is exactly this kind of change. Add a test that compares the __init__ default values from the stub AST against the runtime signature. Defaults that are not plain literals are marked and fail the comparison loudly; classes without an introspectable runtime signature can not be checked; the known set of those classes is pinned, so a new silent skip also fails the test. Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
shsms
changed the title
e76fbe4 to
a9fd1a7
Compare
llucax
left a comment
llucax
left a comment
There was a problem hiding this comment.
🤖 rAIview (AI review here!)
Solid, well-scoped PR: the v0.6.0 dependency bump, the default flip to component-first per-category formulas, the device→component rename, and the new stub-drift default-value test are all clean, and the final HEAD state is consistent (CI green). No functional bugs or security concerns found in the merged result.
The main point worth addressing is a commit-split issue: the wind/steam formula expectation updates were placed in df7d2cc, but the default flip they depend on only happens in the following commit cdcb3cf, so df7d2cc fails its own suite (bisect hazard, and the commit message claims to "match the new default" that isn't set yet). Beyond that, only low-severity items: the newly-documented breaking InvalidGraphError message format and the phantom-load meter-subtraction edge case are untested, and one docstring paragraph under include_phantom_loads_in_consumer_formula reads as contradictory. Details inline.
| assert ( | ||
| graph.wind_turbine_formula(wind_turbine_ids={ComponentId(3)}) | ||
| == "COALESCE(#2, #3, 0.0)" | ||
| == "COALESCE(#3, #2, 0.0)" |
There was a problem hiding this comment.
🟡 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 true → false 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
- src/graph.rs: the default is still
trueatdf7d2cc; flipped only incdcb3cf. - tests/test_formula_preferences.py: still asserts default → meter-first at
df7d2cc, contradicting the updated assertions here.
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.
|
|
||
| - 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. |
There was a problem hiding this comment.
🔵 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
- tests/test_microgrid_component_graph.py:466: checks only the exception type, not the new message prefix.
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.
| 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 |
There was a problem hiding this comment.
🔵 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.
| ) | ||
|
|
||
|
|
||
| def test_consumer_formula_meter_subtraction() -> None: |
There was a problem hiding this comment.
🔵 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.
llucax
left a comment
llucax
left a comment
There was a problem hiding this comment.
LGTM and the AI review seems to only point to minor issues.
I didn't get the last commit though, I don't know exactly what kind of black magic the tests are doing with introspection, but being only tests I am not too concerned.
Approving.
2 participants
Version 0.6.0 of the
frequenz-microgrid-component-graphRust crate is released. This PR builds the Python bindings against it and aligns the binding's defaults, docs, and tests with the new release.Changes
frequenz-microgrid-component-graphcrate from 0.5 to v0.6.0. The release rewrites the formula fallback engine: formulas can now contain meter-subtraction and diamond terms, and the consumer formula measures the components behind one internal meter as one group.prefer_meters_in_component_formulastoFalse(component reading primary, meter fallback), matching the new upstream default.__init__default values between the type stub and the runtime signature, so a default flipped on only one side fails CI.Breaking
prefer_meters_in_component_formulas=Trueto restore the old meter-first order (order only; the new fallback terms are used either way).InvalidGraphErrormessages starting withGraph validation failed:(one indented line per problem); some graph-building errors keep the oldInvalidGraph:format.