-
Notifications
You must be signed in to change notification settings - Fork 18
Upgrade to microgrid client v0.7 #1182
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.
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
Conversation
Draft to avoid merging accidentally because it depends on frequenz-floss/frequenz-client-microgrid-python#122 and a release, but it is really ready for a review. |
There was a problem hiding this 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 upgrades the project to use microgrid client v0.7 by replacing many usages of raw integer component IDs with the new ComponentId type and updating dependency/version references. Key changes include:
- Updating dependency constraints in pyproject.toml and documentation in RELEASE_NOTES.md and mkdocs.yml.
- Replacing int‑based component IDs with ComponentId in multiple modules throughout the SDK.
- Adjusting type annotations, function signatures, and caches accordingly to support the new microgrid client types.
Reviewed Changes
Copilot reviewed 67 out of 67 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
pyproject.toml | Updated dependency for frequenz-client-microgrid to use a git reference for v0.7. |
RELEASE_NOTES.md | Documented upgrade to v0.7 and changes in how IDs are handled. |
mkdocs.yml | Commented out the previous version’s documentation objects inventory for microgrid client. |
Multiple SDK modules | Converted int types to ComponentId in function signatures, caches, documentation, and type hints. |
benchmarks/power_distribution/power_distributor.py | Updated benchmark function signatures to use ComponentId instead of int. |
Comments suppressed due to low confidence (3)
pyproject.toml:32
- Consider replacing the temporary git dependency with the official v0.7 release reference once it is published to ensure reproducible builds.
# "frequenz-client-microgrid >= 0.7.0, < 0.8.0",
src/frequenz/sdk/microgrid/_power_distributing/_component_managers/_battery_manager.py:188
- [nitpick] Since the method now returns ComponentId instead of int, please verify that all dependent modules and downstream usages are updated accordingly.
def component_ids(self) -> collections.abc.Set[ComponentId]:
benchmarks/power_distribution/power_distributor.py:40
- [nitpick] Updating the batteries type from int to ComponentId may affect benchmark logic; please ensure these changes are validated across all benchmark tests.
async def send_requests(batteries: set[ComponentId], request_num: int) -> list[Result]:
788dd7f
to
9c959c2
Compare
9c959c2
to
443237b
Compare
Ready for review now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could make a few more commits; it's fine if they don't pass the tests individually. Or at least make fixup commits for review and squash before merging. Otherwise, it is too hard to keep track of where you are if you have to take several breaks while reviewing.
I can only see breaking the commit on a per-file basis, but that doesn't really bring much, if you want to resume the review after a while and splitting on a file-by-file basis is enough, then you can just use GitHub feature of marking files as read in the review. If you think there was another way to split the commit, I'm interested! |
I see, didn't know that. I'll try. |
Now an amd test is also failing :-| |
Oh dear, failing in Python 3.12 which I'm not using to test locally. Is the component graph tests, so I wonder if it is the |
Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
Signed-off-by: Leandro Lucarella <[email protected]>
443237b
to
6a48d83
Compare
There was a problem hiding this 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 upgrades the microgrid client dependency to v0.7 and updates all component identifier types from int to the new wrapper class ComponentId, ensuring type safety and consistency across the SDK.
- Updated import statements and type annotations to use ComponentId instead of int.
- Adjusted dependency constraints in pyproject.toml and documentation links in mkdocs.yml to match the new client version.
Reviewed Changes
Copilot reviewed 69 out of 69 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/frequenz/sdk/microgrid/_power_distributing/power_distributing.py | Replaced int with ComponentId in processing and pending request dictionaries and method signatures. |
src/frequenz/sdk/microgrid/_power_distributing/_distribution_algorithm/_battery_distribution_algorithm.py | Updated type hints for battery and inverter mappings and power distribution dictionaries. |
src/frequenz/sdk/microgrid/_power_distributing/_component_status/* | Updated component identifier types in status trackers and related methods. |
src/frequenz/sdk/microgrid/_power_distributing/_component_managers/* | Changed signatures and internal dictionaries to use ComponentId consistently. |
src/frequenz/sdk/microgrid/_data_sourcing/* | Modified type hints and caches to use ComponentId. |
pyproject.toml, mkdocs.yml, RELEASE_NOTES.md | Adjusted version constraints and documentation to reflect the upgrade. |
benchmarks/power_distribution/power_distributor.py | Updated type definitions in benchmark routines to use ComponentId. |
Ready for another try at merging, thanks @shsms for debugging the test failures! ❤️ |
Use the new microgrid client
ComponentId
andMicrogridId
.