Skip to content

Extract abstract base classes from pools and pool ref stores#1391

Merged
simonvoelcker merged 6 commits intofrequenz-floss:v1.x.xfrom
simonvoelcker:extract_abstract_pool
May 6, 2026
Merged

Extract abstract base classes from pools and pool ref stores#1391
simonvoelcker merged 6 commits intofrequenz-floss:v1.x.xfrom
simonvoelcker:extract_abstract_pool

Conversation

@simonvoelcker
Copy link
Copy Markdown
Contributor

@simonvoelcker simonvoelcker commented Apr 22, 2026

This is preparation for the introduction of a new subclass for steam boiler pools.

There was a lot of duplicated code between the three existing types of pools and their respective references stores, with just a bit of drift.

@github-actions github-actions Bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:data-pipeline Affects the data pipeline labels Apr 22, 2026
@simonvoelcker simonvoelcker force-pushed the extract_abstract_pool branch from d55998e to 4f167b2 Compare April 22, 2026 13:06
@simonvoelcker simonvoelcker added the cmd:skip-release-notes It is not necessary to update release notes for this PR label Apr 22, 2026
@simonvoelcker simonvoelcker force-pushed the extract_abstract_pool branch 2 times, most recently from 48759b1 to 5f6e41f Compare April 22, 2026 13:19
@simonvoelcker simonvoelcker marked this pull request as ready for review April 22, 2026 13:26
@simonvoelcker simonvoelcker requested a review from a team as a code owner April 22, 2026 13:26
@simonvoelcker simonvoelcker requested review from Marenz and removed request for a team April 22, 2026 13:26
@simonvoelcker simonvoelcker self-assigned this Apr 22, 2026
Comment thread src/frequenz/sdk/timeseries/component_pool/_component_pool.py
Comment thread src/frequenz/sdk/timeseries/abstract_pool/_abstract_pool.py Outdated
Comment thread src/frequenz/sdk/timeseries/ev_charger_pool/_ev_charger_pool_reference_store.py Outdated
Comment thread src/frequenz/sdk/timeseries/component_pool/_component_pool.py
@simonvoelcker simonvoelcker requested a review from Marenz April 24, 2026 12:59
@simonvoelcker
Copy link
Copy Markdown
Contributor Author

@Marenz could you have another look please? 🙃

llucax

This comment was marked as low quality.

@llucax llucax linked an issue May 4, 2026 that may be closed by this pull request
llucax

This comment was marked as low quality.

llucax

This comment was marked as low quality.

Copy link
Copy Markdown
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.

Pretty nice! 🎉

The main concern is the breaking changes. If we decide to add any, we would need to update the release notes to mention it too. Maybe adding tests for the parts that didn't fail even if we introduced breaking changes would be a good idea too.

I'm not completely against a "Address PR comments" commit but since amending the previous commit should be trivial, I would go with that. Also "Make AbstractPool generic over ref store and report types. Extract abstract base class for reports." feels like maybe the second part should be the commit body instead of part of a (long) title?

Finally I wonder if something like ComponentPool could make more sense than AbstractPool, thinking about maybe at some point we want to use them more generically, maybe some lib can accept any pool, not really caring about which kind of components are in it, and just wants to propose power, maybe ComponentPool reads more easily than AbstractPool. Or if we really want to highlight the abtractness, maybe use AbstractComponentPool?

Comment thread src/frequenz/sdk/timeseries/pv_pool/_pv_pool.py Outdated
Comment thread src/frequenz/sdk/timeseries/abstract_pool/__init__.py Outdated
Comment thread src/frequenz/sdk/timeseries/component_pool/_component_pool.py
Comment thread src/frequenz/sdk/timeseries/battery_pool/_battery_pool.py Outdated
Comment thread src/frequenz/sdk/timeseries/battery_pool/messages.py Outdated
@simonvoelcker simonvoelcker force-pushed the extract_abstract_pool branch 2 times, most recently from 1d4cdfe to d44b98d Compare May 5, 2026 12:57
@github-actions github-actions Bot added the part:docs Affects the documentation label May 5, 2026
@github-actions github-actions Bot added the part:microgrid Affects the interactions with the microgrid label May 5, 2026
Signed-off-by: Simon Völcker <simon.voelcker@frequenz.com>
Signed-off-by: Simon Völcker <simon.voelcker@frequenz.com>
@simonvoelcker simonvoelcker force-pushed the extract_abstract_pool branch from 46d00ac to 12f0978 Compare May 5, 2026 13:46
Extract abstract base class for reports.

Signed-off-by: Simon Völcker <simon.voelcker@frequenz.com>
Signed-off-by: Simon Völcker <simon.voelcker@frequenz.com>
@simonvoelcker simonvoelcker force-pushed the extract_abstract_pool branch from 12f0978 to 93dd6b8 Compare May 5, 2026 13:56
Copy link
Copy Markdown
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.

Damn, I just realized batteries_id is an argument for the reference pool, not the battery pool (I always think the pool should be constructed passing the IDs 😱).

Since reference stores are private (not part of the public modules), we don't actually need the deprecation path, we can just rename it (so we can remove the override for the ctor).

Sorry I didn't realized this before 😬

Besides these very minor comments, this is good to go for me.

Comment thread examples/battery_pool.py Outdated
Comment thread src/frequenz/sdk/microgrid/_power_managing/_power_managing_actor.py Outdated
Comment thread src/frequenz/sdk/microgrid/_power_managing/_power_managing_actor.py Outdated
Comment thread src/frequenz/sdk/microgrid/_power_managing/_power_managing_actor.py Outdated
Comment thread src/frequenz/sdk/timeseries/battery_pool/_battery_pool_reference_store.py Outdated
Signed-off-by: Simon Völcker <simon.voelcker@frequenz.com>
Signed-off-by: Simon Völcker <simon.voelcker@frequenz.com>
@simonvoelcker simonvoelcker force-pushed the extract_abstract_pool branch from 93dd6b8 to dc9a076 Compare May 6, 2026 11:57
Copy link
Copy Markdown
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.

LGTM, removing the protocol, if possible, is optional and low-prio IMHO, so approving.

@github-project-automation github-project-automation Bot moved this from To do to Review approved in Python SDK Roadmap May 6, 2026
@llucax llucax added this to the v1.0.0-rc2206 milestone May 6, 2026
@simonvoelcker simonvoelcker added this pull request to the merge queue May 6, 2026
Merged via the queue into frequenz-floss:v1.x.x with commit 770d768 May 6, 2026
8 checks passed
@simonvoelcker simonvoelcker deleted the extract_abstract_pool branch May 6, 2026 13:52
@github-project-automation github-project-automation Bot moved this from Review approved to Done in Python SDK Roadmap May 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cmd:skip-release-notes It is not necessary to update release notes for this PR part:data-pipeline Affects the data pipeline part:docs Affects the documentation part:microgrid Affects the interactions with the microgrid part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

Development

Successfully merging this pull request may close these issues.

3 participants