BatteryPool should be more explicit about using all components or a given set of component ids

[Marenz]

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:

• Add an explicit parameter use_all_components: bool
• Also add a new getter using_all_components: bool to explicitly know if it is using all components or a a specific set

Use cases

No response

Alternatives and workarounds

No response

Additional context

No response

[llucax]

So calling without either some component_ids or use_all_components will be an error? I don't like that it makes the interface more complicated and that you can end up with some exception if you call it with the wrong combination of arguments.

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.

[Marenz]

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.

So calling without either some component_ids or use_all_components will be an error?

No I think it' still fine to have use_all as default, it just shouldn't be use_all if you pass an empty set or None, imo, that mixes the two different cases too much.

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.

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)

[llucax]

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 USE_ALL and make the signature: new_pool(batteries: USE_ALL | Set[int] = USE_ALL)? Then we can make passing an empty set an error and I think the interface should be pretty clear and explicit.

Then we can expose the requested batteries as a property (which might be different to the "currently used batteries", we should expose this as a separate property), which will have the info if you requested all or a fixed set.

[llucax]

@jh2007github this might be a good issue for you to pick up.

[shsms]

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.

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: https://github.com/frequenz-floss/frequenz-sdk-python/blob/2faab71a75b1eae1ff35f7a188e39aa5a26bb187/src/frequenz/sdk/timeseries/battery_pool/_battery_pool_reference_store.py#L79-L83

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 battery_pool.component_ids would be stable irrespective of how it was created.

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 USE_ALL rather than an empty set as code to automatically fetch the components.

[llucax]

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 new_battery_pool(get_all_batteries()). Super explicit also and we get an extra convenience feature of being able to retrieve all components of a category, but it requires extra work and it is considerably more verbose.

[Marenz]

Then it is new_battery_pool(get_all_batteries()).

While I do like that idea, I think we should try to make the most common case the most convenient one, too. Maybe this + factory function?

[llucax]

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"?

[Marenz]

your suggestion as is, additionally a convenience function to do exactly what your suggestion says

[llucax]

Also taking some distance from the topic and just reading pool = microgrid.new_battery_pool() for me there is not much other option than getting a pool with all the available batteries. So not sure if we really need a change here. I guess the original issue was it was confusing to have pool = microgrid.new_battery_pool(set()) get all the batteries, and this is true, so back to new_pool(batteries: USE_ALL | set[int] = USE_ALL), this sounds reasonable (it is still new_battery_pool() the default to get all) and we can make receiving an empty set an error.