Drop empty Matryoshka buckets #1155
Conversation
shsms
requested review from
daniel-zullo-frequenz
and removed request for
a team
github-actions
bot
added
part:tests
In the matryoshka implementation there can't be overlapping buckets. So keeping unused buckets forever would mean that it will block other component permutations in future requests. Signed-off-by: Sahas Subramanian <[email protected]>
And if that leads to an empty bucket, drop the bucket too. Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
Signed-off-by: Sahas Subramanian <[email protected]>
shsms
force-pushed
the
drop-old-buckets
branch
from
0606e83 to
370d61b
Compare
|
I will update #1146 to incorporate these changes after this is merged. |
| bucket.add(proposal) | ||
| elif not bucket: | ||
| del self._component_buckets[component_ids] | ||
| _ = self._target_power.pop(component_ids, None) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
| # succeed. And it will fail until then. | ||
| with pytest.raises( | ||
| NotImplementedError, | ||
| match=re.escape( |
If buckets remain for ever in the power manager's Matryoshka instances, proposals with overlapping component IDs will never get accepted. But if empty buckets go away as soon as they become empty, then it becomes possible to propose requests for components without conflicting with old unused component ID buckets.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
This is getting a weird error saying |
2 participants
If buckets remain for ever in the power manager's Matryoshka instances, proposals with overlapping component IDs will never get accepted. But if empty buckets go away as soon as they become empty, then it becomes possible to propose requests for components without conflicting with old unused component ID buckets.