Skip to content

Drop empty Matryoshka buckets #1155

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jan 27, 2025
Merged

Drop empty Matryoshka buckets #1155

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7555366
Drop buckets without any proposals
shsms Jan 22, 2025
21c96fb
Drop the source from the bucket if a `None` proposal is received
shsms Jan 22, 2025
7929c71
Test that empty buckets get dropped when old proposals are dropped
shsms Jan 24, 2025
c256280
Test that proposals and empty buckets get dropped on `None` proposals
shsms Jan 24, 2025
370d61b
Update release notes
shsms Jan 24, 2025
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
10 changes: 9 additions & 1 deletion src/frequenz/sdk/microgrid/_power_managing/_matryoshka.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,15 @@ def calculate_target_power(
bucket = self._component_buckets.setdefault(component_ids, set())
if proposal in bucket:
bucket.remove(proposal)
bucket.add(proposal)
if (
proposal.preferred_power is not None
or proposal.bounds.lower is not None
or proposal.bounds.upper is not None
):
bucket.add(proposal)
elif not bucket:
del self._component_buckets[component_ids]
_ = self._target_power.pop(component_ids, None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need the _ = . Also is it a pop instead of a del because component_id might not be in self._target_power?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have been using an extra pedantic lsp server that warns about discarding return values. And because I have been hit by such bugs before, I decided to not disable it.

And yes, pop because there's one case where component IDs won't be in target_power, and that's when the first request ever sent to a battery pool is a None, (None, None).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which linter is that? I did a quick search and didn't found any linter that has that feature. I agree is a good lint to have, otherwise it looks very arbitrary, as we have many other places where values are silently discarded. It would be nice to enabled it for everyone so we don't end up with inconsistent code (that it might also be very noisy for the people using the pendantic tool).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been using https://docs.basedpyright.com/latest/ and enjoying it a lot. I've been using it as an LSP server, but it is also available as a CLI tool.


# If there has not been any proposal for the given components, don't calculate a
# target power and just return `None`.
Expand Down