Skip to content
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

Jump to bottom

Allow optionally forcing power requests #365

Merged
merged 7 commits into from
May 25, 2023
Merged

Allow optionally forcing power requests #365

merged 7 commits into from
May 25, 2023

Conversation

daniel-zullo-frequenz
Copy link
Contributor

A power request might need to be forced to implement safety mechanisms, even when some components might be
seemingly failing (i.e. when there is not proper consumption information, the user wants to slowly discharge batteries to
prevent potential peak breaches).

Fixes #321

@github-actions github-actions bot added part:actor Affects an actor ot the actors utilities (decorator, etc.) part:docs Affects the documentation part:microgrid Affects the interactions with the microgrid part:power-management Affects the management of battery power and distribution part:tests Affects the unit, integration and performance (benchmarks) tests labels Apr 26, 2023
@daniel-zullo-frequenz daniel-zullo-frequenz self-assigned this Apr 26, 2023
@github-actions github-actions bot removed part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:microgrid Affects the interactions with the microgrid part:power-management Affects the management of battery power and distribution labels Apr 27, 2023
@leandro-lucarella-frequenz leandro-lucarella-frequenz changed the title Add functionality to force power requests Allow optionally forcing power requests May 5, 2023
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests labels May 5, 2023
@daniel-zullo-frequenz
Copy link
Contributor Author

Added unit tests:

  • Check all batteries are used when none battery is working (missing test not entirely related to force power request)
  • Extend test that check all batteries are used when none battery is working and the power request is forced
  • Test all batteries are used when one of them is not working and the power request is forced
  • Test a battery is used even the metric SoC, capacity or power bounds are currently set to NaN.

Note that the current patch/solution depends on the metrics cache, therefore if the cache is not available for a non-working battery then the battery won't be used even though the request is forced. This needs to be discussed as it was my personal assumption rather something explicitly added in the issue #312 or agreed with the team.
fyi @sahas-subramanian-frequenz

@daniel-zullo-frequenz
Copy link
Contributor Author

Note that the current patch/solution depends on the metrics cache, therefore if the cache is not available for a non-working battery then the battery won't be used even though the request is forced. This needs to be discussed as it was my personal assumption rather something explicitly added in the issue #312 or agreed with the team.

It was agreed that the power need to be distributed equally between the batteries if there is no previous metrics cached and the request is forced.

@daniel-zullo-frequenz daniel-zullo-frequenz marked this pull request as ready for review May 19, 2023 08:31
@daniel-zullo-frequenz daniel-zullo-frequenz requested a review from a team as a code owner May 19, 2023 08:31
@leandro-lucarella-frequenz
Copy link
Contributor

FYI, if this is blocking @shsms to move fast with his PR, I'm fine to merge this as is and address the remaining comments in a separate PR. We need to make sure to get the public API right before the release, but other minor improvements like the dataclass stuff or internal argument names can be worked out even after the release.

@daniel-zullo-frequenz
Fix wrong documentation in tests
9797c16 
Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
@daniel-zullo-frequenz
Use future annotations in power distribution tests
366462c 
Future annotations is already imported and it should
be used instead.

Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
@github-actions github-actions bot added the part:power-management Affects the management of battery power and distribution label May 25, 2023
@daniel-zullo-frequenz
Test all batteries are used if none of them works
6116e95 
A test was not added when the power distributing actor
was updated in the past to use all the batteries for
cases where all of them are reported as not working
batteries.

Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
@daniel-zullo-frequenz
Amend existent power distributing test
a1bf628 
To make it consistent with recently added
power distributing tests.

Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
@daniel-zullo-frequenz
Simplify working batteries check
7ee1bf3 
The _get_working_batteries method had unnecessary complexity
and was redundant as it was returning the original set of batteries
if there was any working battery.
Therefore, this commit removes the _get_working_batteries method and
simplifies the check for working batteries.

Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
@daniel-zullo-frequenz
Add battery request check for empty set
0540381 
Requests with empty batteries set need to be rejected.

Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
@daniel-zullo-frequenz daniel-zullo-frequenz force-pushed the feature/force-power-request branch 2 times, most recently from 662467f to c24b165 Compare May 25, 2023 11:46
@daniel-zullo-frequenz
Copy link
Contributor Author

I've updated following most of the suggestions. However we still need to make our minds about the attribute name to force the power requests. Also documentation needs to be improved/amended. I´d suggest to review it again focus in the logic and we can address the naming and documentation updates in follow-up PRs to unblock Sahas

leandro-lucarella-frequenz
leandro-lucarella-frequenz reviewed May 25, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

Except from my own mistakes, LGTM :D

src/frequenz/sdk/actor/power_distributing/power_distributing.py Outdated Show resolved Hide resolved
src/frequenz/sdk/actor/power_distributing/power_distributing.py Outdated Show resolved Hide resolved
shsms
shsms previously approved these changes May 25, 2023
View reviewed changes
Copy link
Contributor

@shsms shsms left a comment

Choose a reason for hiding this comment

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

An improvement for a subsequent PR could be to use timestamps coming from the microgrid component data, rather than repeated calls to time.monotonic_ns, when creating the cache entries.

@daniel-zullo-frequenz
Allow optionally forcing power requests
7fa7b70 
A power request might need to be forced to implement
safety mechanisms, even when some components might be
seemingly failing (i.e. when there is not proper consumption
information, the user wants to slowly discharge batteries to
prevent potential peak breaches).

Signed-off-by: Daniel Zullo <daniel.zullo@frequenz.com>
@daniel-zullo-frequenz daniel-zullo-frequenz added this pull request to the merge queue May 25, 2023
Merged via the queue into frequenz-floss:v0.x.x with commit ca8a734 May 25, 2023
8 checks passed
@daniel-zullo-frequenz daniel-zullo-frequenz deleted the feature/force-power-request branch May 25, 2023 14:31
This was referenced May 30, 2023
Merged
Open
llucax added a commit that referenced this pull request Jun 19, 2023
@llucax
Bump pytest-mock from 3.10.0 to 3.11.1 (#440)
4ac6286 
Bumps [pytest-mock](https://github.com/pytest-dev/pytest-mock) from
3.10.0 to 3.11.1.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a
href="https://github.com/pytest-dev/pytest-mock/releases">pytest-mock's
releases</a>.</em></p>
<blockquote>
<h2>v3.11.1</h2>
<ul>
<li>
<p>Fixed introspection for failed <code>assert_has_calls</code> (<a
href="https://redirect.github.com/pytest-dev/pytest-mock/issues/365">#365</a>).</p>
</li>
<li>
<p>Updated type annotations for <code>mocker.patch</code> and
<code>mocker.spy</code> (<a
href="https://redirect.github.com/pytest-dev/pytest-mock/issues/364">#364</a>).</p>
</li>
</ul>
</blockquote>
</details>
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/pytest-dev/pytest-mock/blob/main/CHANGELOG.rst">pytest-mock's
changelog</a>.</em></p>
<blockquote>
<h2>3.11.1 (2023-06-15)</h2>
<ul>
<li>
<p>Fixed introspection for failed <code>assert_has_calls</code>
(<code>[#365](https://github.com/pytest-dev/pytest-mock/issues/365)</code>_).</p>
</li>
<li>
<p>Updated type annotations for <code>mocker.patch</code> and
<code>mocker.spy</code>
(<code>[#364](https://github.com/pytest-dev/pytest-mock/issues/364)</code>_).</p>
</li>
</ul>
<p>.. _<a
href="https://redirect.github.com/pytest-dev/pytest-mock/issues/365">#365</a>:
<a
href="https://redirect.github.com/pytest-dev/pytest-mock/pull/365">pytest-dev/pytest-mock#365</a>
.. _<a
href="https://redirect.github.com/pytest-dev/pytest-mock/issues/364">#364</a>:
<a
href="https://redirect.github.com/pytest-dev/pytest-mock/pull/364">pytest-dev/pytest-mock#364</a></p>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/pytest-dev/pytest-mock/commit/d3e73f2e93f7b93eba0a36e17e43bafd969da4fe"><code>d3e73f2</code></a>
Explicitly specify the tag name during GitHub Release</li>
<li><a
href="https://github.com/pytest-dev/pytest-mock/commit/5668efe23e01673af9905febeefd9a9791b023f4"><code>5668efe</code></a>
Update CHANGELOG for 3.11.0</li>
<li><a
href="https://github.com/pytest-dev/pytest-mock/commit/c9a4d13ab4e808ef02250898a79c07b2acf76f61"><code>c9a4d13</code></a>
Improve CI</li>
<li><a
href="https://github.com/pytest-dev/pytest-mock/commit/5818160717a16b7f8867d950ed60b9dc3349ec8d"><code>5818160</code></a>
Add assert_has_calls_wrapper (<a
href="https://redirect.github.com/pytest-dev/pytest-mock/issues/365">#365</a>)</li>
<li><a
href="https://github.com/pytest-dev/pytest-mock/commit/355b5aae1a1a9b3f2c17b2a6bbfde9980967bceb"><code>355b5aa</code></a>
Fix return type annotation for patch and spy (<a
href="https://redirect.github.com/pytest-dev/pytest-mock/issues/364">#364</a>)</li>
<li><a
href="https://github.com/pytest-dev/pytest-mock/commit/8ba681201488bec69f27f6e9e2a56988a01f0d1b"><code>8ba6812</code></a>
[pre-commit.ci] pre-commit autoupdate (<a
href="https://redirect.github.com/pytest-dev/pytest-mock/issues/362">#362</a>)</li>
<li><a
href="https://github.com/pytest-dev/pytest-mock/commit/786eaaf416fed8204a8aebb29cbf71527228a9f6"><code>786eaaf</code></a>
[pre-commit.ci] pre-commit autoupdate (<a
href="https://redirect.github.com/pytest-dev/pytest-mock/issues/360">#360</a>)</li>
<li><a
href="https://github.com/pytest-dev/pytest-mock/commit/1cb146af21ffd0b5be24703419cf4a192c4ec0ab"><code>1cb146a</code></a>
Push tag manually</li>
<li><a
href="https://github.com/pytest-dev/pytest-mock/commit/9608e9659b1f062bb09915f3ef1c538ca7cb5c5f"><code>9608e96</code></a>
Update permissions for deploy workflow</li>
<li><a
href="https://github.com/pytest-dev/pytest-mock/commit/c778ee73bd516d465dca3512d099682ba32108fa"><code>c778ee7</code></a>
Fix deployment workflow (<a
href="https://redirect.github.com/pytest-dev/pytest-mock/issues/358">#358</a>)</li>
<li>Additional commits viewable in <a
href="https://github.com/pytest-dev/pytest-mock/compare/v3.10.0...v3.11.1">compare
view</a></li>
</ul>
</details>
<br />

[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pytest-mock&package-manager=pip&previous-version=3.10.0&new-version=3.11.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)

</details>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leandro-lucarella-frequenz leandro-lucarella-frequenz leandro-lucarella-frequenz left review comments

@shsms shsms shsms approved these changes

@christianparpart christianparpart Awaiting requested review from christianparpart christianparpart was automatically assigned from frequenz-floss/python-sdk-team

@Marenz Marenz Awaiting requested review from Marenz

Assignees

@daniel-zullo-frequenz daniel-zullo-frequenz

Labels
part:actor Affects an actor ot the actors utilities (decorator, etc.) part:docs Affects the documentation part:power-management Affects the management of battery power and distribution part:tests Affects the unit, integration and performance (benchmarks) tests
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a force option in PowerDistributingActor Request
4 participants
@daniel-zullo-frequenz @shsms @leandro-lucarella-frequenz @Marenz