Skip to content

Conversation

@llucax
Copy link
Contributor

@llucax llucax commented Jan 8, 2025

This PR modifies the DispatchManagingActor to re-create the actor when a new dispatch is received and the actor should start running instead of just start/stop the actor. To create the actor a factory function is used, which passes the initial dispatch information to the actor, so it can be properly initialized instead of having the initialization in 2 steps, the creation and the receiving of the dispatch update.

This also removes the support for multiple actors, as it was a very niche feature and makes the code more complicated.

I was also planning to make the updates sender required to simplify the code even further, it should also be very niche for someone not wanting to receive dispatch updates, almost bordering the bug.

Finally, making the DispatchManagingActor a background service instead of an actor and have it manage its own channel. And maybe integrating it to the dispatcher, so the user interface could potentially be as simple as:

dispatcher = Dispatcher(...)
dispatcher.start_handling(
    dispatch_type,
    actor_factory=lambda initial_dispatch, dispatch_updates_receiver: MyActor.new_with_dispatch(..., initial_dispatch, dispatch_update_receiver),
    # in the future maybe also a dispatch merge strategy here
)
...
dispatcher.stop_handling(dispatch_type)

This commit modifies the `DispatchManagingActor` to re-create the actor
when a new dispatch is received and the actor should start running
instead of just start/stop the actor. To create the actor a factory
function is used, which passes the initial dispatch information to the
actor, so it can be properly initialized instead of having the
initialization in 2 steps, the creation and the receiving of the dispatch
update.
@llucax llucax mentioned this pull request Jan 8, 2025
@github-actions github-actions bot added the part:dispatcher Affects the high-level dispatcher interface label Jan 8, 2025
"""
if dispatch.type != self._dispatch_type:
_logger.debug("Ignoring dispatch %s", dispatch.id)
_logger.debug(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this check anymore as the new running_state sender supports filtering by type

[actor] if isinstance(actor, Actor) else actor
)
self._actor_factory = actor_factory
self._actor: Actor | None = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was going to change to self._actors: dict[int, Actor] where the key is int as the dispatch ID, allowing all incoming dispatches to run or frozenset[int] allowing only one dispatch for a given set of batteries.

The power setting actor is using int as key. Is that coming in a separate PR @Marenz?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, that's a good point but a separate issue. This was a list of actors in the past only because we some use case actor needed to instantiate many actors when a dispatch arrived only because that actor was not designed properly, this is why this PR removes that.

We need to support the use case you mention too, but I would discuss it elsewhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this PR might not be merged in a LONG time, while the feature you mention should be implemented soon)

@Marenz
Copy link
Contributor

Marenz commented Jan 23, 2025

Fyi, I started on a new branch a redesign based on this and the other or, so don't invest time in this currently

@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests labels Jan 27, 2025
@Marenz
Copy link
Contributor

Marenz commented Jan 27, 2025

Also, I now accidentally pushed into this pr/branch, sorry. My PR is here: #103

@llucax
Copy link
Contributor Author

llucax commented Jan 29, 2025

Restored. Although since you took over this I think I will just close. We can reopen if for some strange reason it would become relevant again.

@llucax llucax closed this Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:dispatcher Affects the high-level dispatcher interface part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants