Skip to content

Change type of TargetComponents #170

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
Closed
wants to merge 2 commits into from
Closed

Change type of TargetComponents #170

wants to merge 2 commits into from

Conversation

ela-kotulska-frequenz
Copy link

@ela-kotulska-frequenz ela-kotulska-frequenz commented May 26, 2025 •
edited
Loading

No description provided.

@ela-kotulska-frequenz ela-kotulska-frequenz self-assigned this May 26, 2025
@Copilot Copilot AI review requested due to automatic review settings May 26, 2025 15:47
@ela-kotulska-frequenz ela-kotulska-frequenz requested review from a team as code owners May 26, 2025 15:47
@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.) part:test-utils Affects the test utilities part:dispatcher labels May 26, 2025
Copilot
Copilot AI reviewed May 26, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

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 TargetComponents API to use ComponentId wrappers instead of raw integers, adjusts all related serialization logic and tests, and exposes the new type in the public API.

  • Swapped list[int] for list[ComponentId] in TargetComponents and conversion functions
  • Updated tests, CLI mocks, and data generators to wrap targets in ComponentId
  • Added frequenz-client-microgrid dependency and documented the change in release notes

Reviewed Changes

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

Show a summary per file
File Description
tests/test_proto.py Updated target lists to use ComponentId
tests/test_cli.py Wrapped CLI mock targets in ComponentId and adjusted type hints
src/frequenz/client/dispatch/types.py Changed TargetComponents alias, import, and protobuf converters
src/frequenz/client/dispatch/test/generator.py Updated random generator to yield ComponentId
src/frequenz/client/dispatch/init.py Exported TargetComponents in __all__
pyproject.toml Added frequenz-client-microgrid dependency
RELEASE_NOTES.md Documented new ComponentId usage and dependency
Comments suppressed due to low confidence (2)

src/frequenz/client/dispatch/types.py:86

  • The pattern case list(component_ids) is a class pattern and may not match arbitrary lists as expected. Consider using a sequence pattern like case [*component_ids] if all(...).
case list(component_ids) if all(isinstance(id, ComponentId) for id in component_ids):

pyproject.toml:43

  • Align this dependency's indentation with the other entries in [tool.poetry.dependencies] for consistency.
"frequenz-client-microgrid >= 0.7.0, < 0.8.0",

llucax
llucax reviewed May 27, 2025
View reviewed changes
pyproject.toml
@@ -40,6 +40,7 @@ dependencies = [
"frequenz-api-dispatch == 1.0.0-rc1",
"frequenz-client-base >= 0.8.0, < 0.10.0",
"frequenz-client-common >= 0.1.0, < 0.4.0",
"frequenz-client-microgrid >= 0.7.0, < 0.8.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not do this yet. If needed we should move it to frequenz-client-common. I will do a PR soon.

Copy link
Author

Choose a reason for hiding this comment

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

But I need this now to move actor to latest SDK :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess if it is temporary is OK, but please create an issue so we don't forget to remove the dependency in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

On a second thought, it is not necessary to start using ComponentId, you can always convert between it and int freely (cid = ComponentId(1) / id = int(cid)).

Copy link
Author

@ela-kotulska-frequenz ela-kotulska-frequenz May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

TargetComponents is part of DispatchRequest.
Actor receives DispatchRequest and needs to create BatteryPool from the target.
BatteryPool expects set[ComponentId] :)

I can to change every actor to change int to ComponentId.
But after we add this, I would have to revert this change in every actor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Mmmm, damn. I guess then I'm back to the "OK as a temporary thing" comment 😬

Copy link
Author

Choose a reason for hiding this comment

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

please create an issue so we don't forget to remove the dependency in the future.

#172

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use typing.SupportsInt and support the unknown to int castable ComponentsId that way

Copy link
Author

@ela-kotulska-frequenz ela-kotulska-frequenz May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

how would it fix it? The problem with Actor will still be there. They except ComponentId, get int.

Copy link
Author

Choose a reason for hiding this comment

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

Marenz had awesome idea to make this change temporarily somewhere else :)
frequenz-floss/frequenz-dispatch-python#156

@github-actions github-actions bot added the part:cli Affects the command-line interface label May 27, 2025
@ela-kotulska-frequenz
Make TargetComponents public
1d9fc31 
Because it is used in other code.

Signed-off-by: Elzbieta Kotulska <[email protected]>
@ela-kotulska-frequenz
Change type of TargetComponents to return ComponentId
22be2c2 
* `frequenz.client.dispatch.TargetComponents` is now public
* its type has changed from `list[int] | list[ComponentCategory]`
to `list[ComponentId] | list[ComponentCategory]`.
* This change introduces a new dependency on
`frequenz-client-microgrid` (>= v0.7.0, < 0.8.0) for the
`frequenz.client.microgrid.ComponentId` type.

Signed-off-by: Elzbieta Kotulska <[email protected]>
@Marenz
Copy link
Contributor

Marenz commented May 27, 2025

Closed in favor of frequenz-floss/frequenz-dispatch-python#156

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

Copilot code review Copilot Copilot left review comments

@Marenz Marenz Awaiting requested review from Marenz

@llucax llucax Awaiting requested review from llucax

Assignees

@ela-kotulska-frequenz ela-kotulska-frequenz

Labels
part:cli Affects the command-line interface part:dispatcher part:docs Affects the documentation part:test-utils Affects the test utilities 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
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ela-kotulska-frequenz @Marenz @llucax