-
Notifications
You must be signed in to change notification settings - Fork 17
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
BatteryPool should be more explicit about using all components or a given set of component ids #949
Comments
So calling without either some Also, why it is important to know if it was created with all available components or not? It would be good to explain the user case a little bit. |
It's fundamentally different if you use the pool with "use everything you can, even new things" and "use this fixed set of ids" and right now it's non-trivial to find out which of those two cases it is, once the pool is created. and because it is so fundamentally different, I feel it should be setup differently, too.
No I think it' still fine to have
Probably becoming less relevant once the SDK does pool caching or so, but it would help to then decide if you want to recreate the pool because you need a different set of ids (or all) |
OK, I see your point now, but I if the issue is the interface being confusing and error prone, then I think your suggestion introduces other set of issues, as we have an overlapping set of options. What about creating a sentinel value Then we can expose the requested |
@jh2007github this might be a good issue for you to pick up. |
Sorry, a bit late, but this is not entirely accurate. When nothing is specified, it uses the available components at start. It doesn't pick up new or dropped components dynamically: frequenz-sdk-python/src/frequenz/sdk/timeseries/battery_pool/_battery_pool_reference_store.py Lines 79 to 83 in 2faab71
It just checks the component graph one time, so even components that are not connected, but present in the component graph are added. So there's no real difference between how they behave, and using Also, using newly added components is not really possible without a restart. The component graph doesn't get updated once setup. We were discussing in the early days about having a trigger to automatically update the component graph when there are changes and rebuild the formulas, and reconfigure the power distributor, etc. But that's all a lot of work, and hasn't been prioritized because it happens so rarely and we've just been restarting the apps. All that said, I'm still ok with the proposal from Luca of having |
Third option is to take only a set, without any special meaning, and having a simple function to get all components of a particular category. Then it is |
While I do like that idea, I think we should try to make the most common case the most convenient one, too. |
Yeah, even when I like explicitness a lot, I also have my doubts if it is just too long for the most common case. What do you mean by "this + factory function"? |
your suggestion as is, additionally a convenience function to do exactly what your suggestion says |
Also taking some distance from the topic and just reading |
What's needed?
Working a bit with the scenario of possibly changing component ids in the FCR actor, I found it cumbersome that
the empty set had the special meaning of "use all components".
Proposed solution
I propose we:
use_all_components: bool
using_all_components: bool
to explicitly know if it is using all components or a a specific setUse cases
No response
Alternatives and workarounds
No response
Additional context
No response
The text was updated successfully, but these errors were encountered: