Skip to content

Update frequenz-api-microgrid to v0.15.1 #416

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
Merged
merged 10 commits into from
Aug 4, 2023
Merged

Update frequenz-api-microgrid to v0.15.1 #416

merged 10 commits into from
Aug 4, 2023

Conversation

shsms
Copy link
Contributor

@shsms shsms commented May 31, 2023 •
edited
Loading

Closes #412

To be merged after v0.21 is released.

@shsms shsms added this to the v0.22.0 milestone May 31, 2023
@shsms shsms self-assigned this May 31, 2023
@shsms shsms requested a review from a team as a code owner May 31, 2023 16:42
@github-actions github-actions bot added part:actor Affects an actor ot the actors utilities (decorator, etc.) part:data-pipeline Affects the data pipeline 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 part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) labels May 31, 2023
@github-actions github-actions bot removed the part:docs Affects the documentation label Jun 1, 2023
@shsms shsms changed the title Update frequenz-api-microgrid to v0.14.0 Update frequenz-api-microgrid to v0.15.1 Jun 12, 2023
@llucax llucax self-requested a review June 12, 2023 20:30
llucax
llucax reviewed Jun 14, 2023
View reviewed changes
Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

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

About the "Disable pylint rules that started failing after updating to v0.15.1" commit, it looks extremely fishy. I'd rather investigate what broke and try to fix it before merging all these new ignores.

tests/microgrid/mock_api.py
def GetMicrogridMetadata(
self, request: Empty, context: grpc.ServicerContext
) -> MicrogridMetadata:
"""/nitrogen.Nitrogen/GetMicrogridMetadata method stub."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this nitrogen.Nitrogen/ still a thing? Not to address it in this PR, but to know if we should create an issue to update this or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably not. the api spec no-longer has any mention of nitrogen, so we need to remove all mentions of it from the sdk as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

#555

src/frequenz/sdk/actor/power_distributing/_battery_status.py
@@ -12,10 +12,13 @@
from enum import Enum
from typing import Iterable, Optional, Set, TypeVar, Union

# pylint: disable=no-name-in-module
Copy link
Contributor

Choose a reason for hiding this comment

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

Any ideas why is this suddenly failing? It would be good to find out where this regressions comes from (and ideally fix it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I'm guessing because some protobuf dep version changed in the api repo. That's why I put them all in a single comment, so it can be removed easily once we figure out the issue 🤞🏽

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 actually have a theory now:

Pylint doesn't look at the type annotations, it just looks at the code other other modules to see if the desired functions are there. This works only if the names are defined in python code in the modules. But protoc has not been doing that for a while now, and is just using dynamic module creation during import, and we appear to have to just use the type hints to know what's going on. If you look at one of the generated files, this becomes clear, like .nox/pylint/lib/python3.11/site-packages/frequenz/api/microgrid/inverter_pb2.py

And it was working until now, because we were using a very old version of protoc in the api repo.

@shsms shsms mentioned this pull request Jun 30, 2023
Closed
@llucax llucax modified the milestones: v0.22.0, v0.23.0 Jul 4, 2023
@shsms
Copy link
Contributor Author

shsms commented Jul 19, 2023

Rebased to latest and resolved conflicts.

@shsms shsms force-pushed the api-0.14 branch 2 times, most recently from 7578a7f to 1a1981a Compare July 26, 2023 13:14
@shsms shsms mentioned this pull request Jul 26, 2023
Merged
@llucax
Copy link
Contributor

llucax commented Jul 27, 2023

New conflicts. BTW, please re-request a review when you want me to have a look at this again 🙏

@shsms shsms force-pushed the api-0.14 branch 3 times, most recently from 714fdcb to 425f8a0 Compare July 30, 2023 16:11
@llucax llucax modified the milestones: v0.23.0, v0.24.0 Aug 2, 2023
@shsms
Copy link
Contributor Author

shsms commented Aug 2, 2023

Will add release notes when we are closer to merging, to not have to deal with conflicts.

@llucax
Copy link
Contributor

llucax commented Aug 3, 2023

Why not merging it now?

@shsms shsms force-pushed the api-0.14 branch 2 times, most recently from 9862668 to ae96aa5 Compare August 3, 2023 11:09
@github-actions github-actions bot added the part:docs Affects the documentation label Aug 3, 2023
@shsms
Copy link
Contributor Author

shsms commented Aug 3, 2023 •
edited
Loading

I've rebased on your clear-release notes PR.

Also the last code commit (e484f37) has changed. The previous version was ignoring the missing component type data, which was wrong.

llucax
llucax previously approved these changes Aug 4, 2023
View reviewed changes
@shsms shsms dismissed llucax’s stale review August 4, 2023 15:34

The merge-base changed after approval.

shsms added 10 commits August 4, 2023 17:37
@shsms
Update frequenz-api-microgrid to version 0.15.1
aba0576 
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms
Add inclusion/exclusion bounds to low-level component data types
74a9b92 
These follow the inclusion/exclusion bounds in the microgrid api.

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms
Use inclusion bounds to replace older bounds fields
be64e00 
Support for exclusion bounds will be added separately.

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms
Use ComponentCategory from frequenz-api-common package
1638430 
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms
Update MicrogridGrpcClient and tests to use latest grpc methods
f3e3d41 
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms
Update mock api to mock the v0.15.1 of the api
f1203d4 
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms
Import Bounds from the frequenz-api-common package
228c6c4 
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms
Disable pylint rules that started failing after updating to v0.15.1
c103d1c 
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms
Fetch component_type from component metadata
4708546 
The component type is now part of `Metadata`.  This commit also adds a
test for non-NONE inverter types.

Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms
Update RELEASE_NOTES.md
1fffefa 
Signed-off-by: Sahas Subramanian <sahas.subramanian@proton.me>
@shsms shsms enabled auto-merge August 4, 2023 15:41
@shsms shsms added this pull request to the merge queue Aug 4, 2023
Merged via the queue into frequenz-floss:v0.x.x with commit aa75e4f Aug 4, 2023
@shsms shsms deleted the api-0.14 branch August 4, 2023 16:33
github-merge-queue bot pushed a commit that referenced this pull request Aug 8, 2023
@llucax
Stream exclusion bounds from the battery pool (#537)
d7c524d 
Based on #416, to be merged after upgrading the Microgrid API dependency
to v0.15.1.

Also closes #524
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Marenz Marenz Marenz approved these changes

@llucax llucax llucax left review comments

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

Assignees

@shsms shsms

Labels
part:actor Affects an actor ot the actors utilities (decorator, etc.) part:data-pipeline Affects the data pipeline 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 part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)
Projects
None yet
Milestone
v0.24.0
Development

Successfully merging this pull request may close these issues.

Use the new microgrid API release v0.15.1
3 participants
@shsms @llucax @Marenz