Fix ordering issues with sets of ints
#1177
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These component IDs are stored in sets, which don't have a order
guarantee, and looking at component IDs in exceptions with random order
makes it hard to identify if a particular component is in the list or
not.
This also makes it easier to write tests that are not flaky or depend
on the component's order.
To improve the exceptions also `", ".join()` is used to get less noise
in the exception message (like `frozenset({4, 2, 6})` vs `2, 4, 5`).
This makes exceptions easier to read because it is easier to look for
a component ID
Signed-off-by: Leandro Lucarella <[email protected]>
These tests rely on the order in which components are returned from the component graph using `all_batteries[:2]` to get the first 2 batteries. But the component graph returns a `set`, which doesn't have any ordering guarantees, so if the Python implementation changes, the hashing can change and this resulting order could be different, breaking the tests. This is why we are now specifying IDs to use explicitly (these are the IDs that were used when the tests were written). This can also change in the future if generator of mock components changes, as it might produce different IDs, so this solution is also not bullet-proof. Signed-off-by: Leandro Lucarella <[email protected]>
The inverters were considered as sorted to have the largest exclusion bounds first, but since they were passed as a set, there is not ordering guarantees. We now sort them to ensure we are allocating the excess power to inverters with more capacity first. Signed-off-by: Leandro Lucarella <[email protected]>
When testing the power distribution algorithm, we can't rely on which amount of power goes to each inverter when all inverters have the same bounds, because they are stored in a `set`, which provide no order guarantee, and when distributing power, power is only prioritized using bounds. Signed-off-by: Leandro Lucarella <[email protected]>
llucax
requested review from
ela-kotulska-frequenz
and removed request for
a team
github-actions
bot
added
the
part:tests
github-actions
bot
added
the
part:microgrid
There was a problem hiding this comment.
Pull Request Overview
This PR fixes ordering issues with sets of integers in tests and core microgrid components by using sorted sequences and explicit IDs to ensure deterministic behavior. Key changes include:
- Updating tests to avoid relying on an undefined order from sets by using sorted lists and explicit assertions.
- Adjusting error messages in microgrid components to display IDs in sorted order.
- Modifying distribution algorithms to process inverter IDs in a deterministic order.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/timeseries/_battery_pool/test_battery_pool.py | Changed test setup to explicitly assert and use sorted battery IDs rather than relying on set order. |
| tests/actor/power_distributing/test_battery_distribution_algorithm.py | Introduced assert_any_result to compare distributions without depending on order. |
| src/frequenz/sdk/microgrid/_power_managing/_matryoshka.py | Updated error message to list component IDs in sorted order. |
| src/frequenz/sdk/microgrid/component_graph.py | Improved error message for undefined components by sorting IDs. |
| src/frequenz/sdk/microgrid/_power_distributing/_distribution_algorithm/_battery_distribution_algorithm.py | Sorted inverter IDs in the multi-inverter pairing logic to ensure determinism. |
| tests/actor/_power_managing/test_matryoshka.py | Adjusted error message expectations to match the new sorted order output. |
Comments suppressed due to low confidence (1)
tests/timeseries/_battery_pool/test_battery_pool.py:212
- [nitpick] The test now asserts hard-coded component IDs (8 and 11), which may become brittle if the mock component generator changes. Consider parameterizing the expected IDs or clearly documenting the assumptions about these values.
assert 8 in all_batteries
When testing the `FormulaFormatter`, the `microgrid.pv_pool().power` and `microgrid.grid.power` formulas were used. This pulls a lot of complexity in, as the whole microgrid needs to be mocked. It also makes the test nondeterministic, as the components returned by the component graph, which are used to build the formulas, are returned as a set, which doesn't provide order, so the resulting formula string will vary from run to run. To test higher order formulas, we now build custom formulas instead. Signed-off-by: Leandro Lucarella <[email protected]>
shsms
approved these changes
Mar 17, 2025
github-project-automation
bot
moved this from Review in progress
to Review approved
in Python SDK Roadmap
github-project-automation
bot
moved this from Review approved
to Done
in Python SDK Roadmap
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sets of
inthave a special behavior in Python (at least CPython), as it seems the hashing function is theintitself, resulting insetbeing ordered when iterated. But this is not guaranteed and could change at any time.The code is unfortunately sometimes relaying in this
setorder, and this PR fixes it by usingsortedwhen needed or making tests more robust and work reliable when the order is not guaranteed.This is necessary to transition to using the new microgrid client
ComponentIdto represent component IDs.