Skip to content

Update base-client min version to v0.11#155

Closed
Marenz wants to merge 2 commits into
frequenz-floss:v0.x.xfrom
Marenz:base-client
Closed

Update base-client min version to v0.11#155
Marenz wants to merge 2 commits into
frequenz-floss:v0.x.xfrom
Marenz:base-client

Conversation

@Marenz

@Marenz Marenz commented Jun 2, 2025

Copy link
Copy Markdown
Contributor

No description provided.

Copilot AI review requested due to automatic review settings June 2, 2025 08:50
@Marenz Marenz requested review from a team as code owners June 2, 2025 08:50

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the minimum version of frequenz-client-base to v0.11, adds test assertions for new streaming event types, and documents the change in the release notes.

  • Bumped frequenz-client-base version constraint in pyproject.toml.
  • Extended tests/test_client.py to assert StreamStarted and StreamRetrying events.
  • Updated RELEASE_NOTES.md to mention new streaming notification types.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
tests/test_client.py Add imports and repeated assertions for new stream event types.
pyproject.toml Bump frequenz-client-base dependency to >=0.11.0, <0.12.0.
RELEASE_NOTES.md Document handling of StreamStarted, StreamRetrying, and StreamFatalError.
Comments suppressed due to low confidence (1)

tests/test_client.py:1069

  • The tests cover retry logic but don’t include a StreamFatalError scenario. Consider adding a test case that triggers and asserts StreamFatalError to match the release notes.
assert isinstance(await receiver.receive(), StreamStarted)

Comment thread tests/test_client.py
Comment thread RELEASE_NOTES.md Outdated
@github-actions github-actions Bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) labels Jun 2, 2025
Comment thread RELEASE_NOTES.md Outdated

@llucax llucax left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, wait! You need to also update all the type hints for the client streaming methods, they are only returning XxxData now, but they should return also StreamingEvents or whatever it was called.

@Marenz Marenz force-pushed the base-client branch 3 times, most recently from 321c0c4 to 79ef470 Compare June 2, 2025 09:27
@Marenz

Marenz commented Jun 2, 2025

Copy link
Copy Markdown
Contributor Author

Updated. CI will fail until we release base-client v0.11

@github-actions github-actions Bot added the part:client Affects the client code label Jun 2, 2025

@llucax llucax left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still missing all the component-specific methods using _new_component_data_receiver(), like meter_data(), inverter_data(), etc.

@Marenz

Marenz commented Jun 2, 2025

Copy link
Copy Markdown
Contributor Author

It's a bit inconsistent that there is one method stream_ that streams data and a bunch of mixed others that might or might not stream something.

@llucax

llucax commented Jun 2, 2025

Copy link
Copy Markdown
Contributor

It's a bit inconsistent that there is one method stream_ that streams data and a bunch of mixed others that might or might not stream something.

The xxx_data() methods are old, in the v0.17 branch all will have the same naming pattern (but we'll also have only one method for all component types, so it won't matter so much). The sensor support was added recently so I wanted to use a more "modern" interface, so people that start using it have a simpler upgrade path.

llucax
llucax previously approved these changes Jun 2, 2025
@Marenz Marenz force-pushed the base-client branch 2 times, most recently from cb98fa1 to 98ec5c8 Compare June 2, 2025 13:05
Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>
@llucax llucax marked this pull request as draft June 3, 2025 09:53
@llucax

llucax commented Jun 3, 2025

Copy link
Copy Markdown
Contributor

I'm changing this to a draft until we decide what we do with the new events, if we really want to mix them with real data.

@llucax

llucax commented Jun 3, 2025

Copy link
Copy Markdown
Contributor

I'm thinking that another alternative if we want to move forward more quickly (in case this is blocking something), we could just filter the events here in the client for now, and keep sending only data via the streams. So basically we contain the breaking change in the client-base for now until we find a solution we are happy with.

@llucax

llucax commented Jun 3, 2025

Copy link
Copy Markdown
Contributor

But this will quickly spread though other clients, so we need to find a sustainable solution for the base-client soon, we shouldn't end up with all clients filtering the events internally (except for dispatch).

@Marenz Marenz closed this Jul 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:client Affects the client code part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants