Redesign access to migrogrid.grid() #707
Conversation
| @@ -170,6 +171,25 @@ def ev_charger_pool( | |||
| ) | |||
| return self._ev_charger_pools[key] | |||
|
|
|||
| def grid( | |||
| self, | |||
| ) -> _grid.Grid | None: | |||
There was a problem hiding this comment.
This is generally great, but I think _grid.Grid will not render correctly in the docs, you might need to do the class import like the rest (from ._grid import Grid). You might want to check the generated docs (pip install -e .[dev-mkdocs]; mkdocs serve).
| if _grid.get() is None: | ||
| component_graph = connection_manager.get().component_graph | ||
| _grid.initialize(component_graph.components()) | ||
| return _grid.get() |
There was a problem hiding this comment.
The idea was to remove the _grid.initialize() and just move the initialization to _DataPipeline like we do with the battery_pool, etc. (so this method is just implemented as return _get().grid())
I mean, in terms of interface, this is enough for v1.0 (as we are getting the public interface we wanted), but in terms of making the code more maintainable I would do the move, so we have only one way to initialize microgrid objects. If it's not a lot of work I would do this for v1.0 but if it is we can leave it for afterwards. Also I didn't check if it is 100% feasible and if it really fits _DataPipeline, which is running some actors, so there is also the chance that this approach is not great either, please don't trust me blindly and judge it from yourself :)
There was a problem hiding this comment.
we call grid modules .initialize in the _DataPipeline initialization already. these 3 lines have been a left-over due to the fact that i needed a way to re-initializell in the unit tests. But we've agreed to another way on the test-side already. I've dropped the deadbeaf :)
There was a problem hiding this comment.
I guess you forgot to push then 🧠 💨
There was a problem hiding this comment.
gimme a second. i just wanted to ensure it runs locally
There was a problem hiding this comment.
This is better but my comments from before still apply.
What I had in mind was to sort of rewrite the Grid object to behave more like a BatteryPool object, so instead of having a POD object and an initialize() global, having the initialization as the Grid.__init__(). Then in src/frequenz/sdk/microgrid/_data_pipeline.py, in the _DataPipeline.__init__(), instantiate the Grid object (the "singleton" and save it in self, and then have _DataPipeline.grid() return this singleton. Actually I would make the instantiation of the singleton lazy, like we start the power distribution actor, so it is only initialized if it is really used.
| @@ -21,6 +21,8 @@ | |||
| from ..microgrid.component import Component | |||
| from ..timeseries._grid_frequency import GridFrequency | |||
| from . import connection_manager | |||
| from ._grid import Grid | |||
| from ._grid import get as get_grid | |||
There was a problem hiding this comment.
Just in case, FYI, you can still do:
from ._grid import Grid
from . import _gridAnd use _grid.get() for example for the internal stuff, and use Grid for the public stuff.
This really was not clear from the original ticket #673. :( I'll give that a try though |
Thanks! Yeah, the issue is not super in depth, but depending on who is going to address it might be more or less obvious, so it is hard to find a good balance between spending a big amount of time doing a very in depth spec and just set a title for someone that already knows what needs to be done. We can always sync before starting a new issue to make sure the scope is clear. But in any case, as I said before, for 1.0 I consider the changes in this PR enough, as it fixes the interface issue, which is the most important. So we could also merge as it and do further refactoring in a separate PR. |
@llucax Oh this is great to hear, I'm also willing to do the follow-up PR after the merge of this one. :-) |
|
The CI is failing because of |
a7461bc to
4a42d93
Compare
release notes have been added. I was sick for a week and wonder if I was missing something else (the non-API facing change was agreed to be done in a follow-up PR). Anything else, @llucax? :) |
Signed-off-by: Christian Parpart <christian@parpart.family>
4a42d93 to
767e52c
Compare
There was a problem hiding this comment.
I just tried building the documentation locally and the only problem that was originally obvious is the documentation:
By making the _grid module private, the microgrid._grid.Grid class is not visible through the docs. But we can't call it microgrid.grid either, because we want to keep that name for the function.
The microgrid.battery_pool function for example returns timeseries.battery_pool.BatteryPool, and bypasses this problem differently.
This needs some restructuring, and because we already have plans to do that, I have no objections to taking this in as it is now.
i'll be working on this in a follow-up PR now :) |
Changelog missing. And probably @llucax has some opinions on top of it. :)
Closes #673.