Dispatch Managing Actor by Marenz · Pull Request #54 · frequenz-floss/frequenz-dispatch-python · GitHub

Reset release notes
Add dispatch runner

Just a few really minor comments to check for, LGTM otherwise

I would say all comments are mostly bike shedding, but in particular I think not using configuration and use dispatch update is important, I think it can get a bit confusing for people dealing with dispatches and configuration (as in config manager) too. I really think we should replace all `configuration mentions to update/dispatch update.

Also I found the example a bit confusing, which ideally it shouldn't be the case, the example should help make things less confusing :P

Maybe "Stopped by a dispatch".

I don't like update much in this context either though. How about DispatchParameterUpdate

I don't like update much in this context either though. How about DispatchParameterUpdate

Why don't you like update? Adding Parameter doesn't make it better IMHO, there is no concept of a dispatch parameter in the API. Another option would be change instead of update, but for me update sounds more accurate in this context, this is why I wonder what you don't like about update.

I see there was not much progress or replies on my comments. I guess you are focusing on getting shit to work, which sounds totally reasonable, but just saying that I didn't review much in this round.

Very nice to see the new scheduling algorithm! 🎉 Way better than what we had before.

Lots of comments, so here is a summary:

Things I think needs to be addressed before merging:

These commits need a commit message with a body that describes what's going on, it is really hard to review them without more information of what was wrong and how it was fixed:
- Fix wrong running behaviors with duration=None
- Rewrite internal architecture to be more flexible
- Add dispatch runner (less important but it would be nice, is not a trivial class)
Finish the renaming of config -> update
The hack in the tests (session-wide event_loop fixture), we can't really merge this if something is so fucked up that makes tests hang or make the interpreter segfault 😬
Figure out if we really need to support multiple actors, it is still make noise in my head, my feeling is we should encourage having one actor to handle one dispatch type, if that actor needs other supporting actors, it should be the actor itself in charge of spawning them.
If we are doing more renaming (like DispatchManagingActor and payload -> options), it might be good to do them now too to avoid having breaking changes in the future.

The rest is minor or too big for this PR, so I will create follow-up issues.

bikeshedding: Payload sounds weird to me. For a whole dispatch it makes sense (or at least it was the best we could find) but in this context maybe something like options is more intuitive? To be honest I think options might be a good choice for the specs too.

how about inputs?

Yeah, that could work too, I think we discussed options at some point but wasn't perfectly accurate as some "options" might not be optional. I guess inputs is an improvement in that sense. Maybe parameters could work too.

And I still think "update" sucks and "configuration" was better. "Update" doesn't fit at all in the whole naming scheme

Addressed points 1 - 5

A few more instances of config and we need support for multiple actors for now (temporarily). I will also create issues for the stuff we can address in the future.

Yeah, that could work too, I think we discussed options at some point but wasn't perfectly accurate as some "options" might not be optional. I guess inputs is an improvement in that sense. Maybe parameters could work too.

Added multiple actors back
fixed confi -> up stuff

OK, this seems to be related to an update to https://github.com/pytest-dev/pytest-asyncio/releases/tag/v0.24.0. Issue: pytest-dev/pytest-asyncio#924

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

* Dispatch.running() was always returning RUNNING for duration None * Dispatch.next_run_after() was made aware that there is no next run for inf. Duration * Dispatch._until() now raises when used with Dispatch.None * Actor: delay marking a Dispatch as deleted, otherwise checks wrongly detect it as no longer running. Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

The internal scheduling architecture was rewritten: Instead of using one task per dispatch that will start/stop them, we now have a queue of events and a timer that is rescheduled to the earliest next event each time it triggers. Previously we had to do complicated re-scheduling and cancelling of tasks, now we always just update the event in the queue. Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

A useful actor to help control and manange another actor using dispatches. Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

As recommended by the deprecation warning Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

The merge-base changed after approval.

It looks like now cross-arch tests are consistently failing. This must be definitely a timing issue, the slowest the system is, the more likely we hit this bug and tests hang. Created an issue about it:

Flaky hanging tests after merging #54 #66

Marenz requested a review from a team as a code owner September 11, 2024 15:30

Marenz requested a review from ela-kotulska-frequenz September 11, 2024 15:30

github-actions Bot added part:docs Affects the documentation part:dispatcher Affects the high-level dispatcher interface labels Sep 11, 2024

Marenz force-pushed the runner branch from 4c88807 to b3bdfdc Compare September 11, 2024 15:33

github-actions Bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:actor Affects the dispatching actor labels Sep 11, 2024

Marenz force-pushed the runner branch from b3bdfdc to 7e3419b Compare September 12, 2024 11:46

daniel-zullo-frequenz reviewed Sep 12, 2024

View reviewed changes

Comment thread tests/test_runner.py Outdated

Comment thread tests/test_runner.py Outdated

Comment thread src/frequenz/dispatch/_actor_runner.py Outdated

Marenz force-pushed the runner branch from 7e3419b to c5ef8b9 Compare September 12, 2024 15:13

llucax reviewed Sep 13, 2024

View reviewed changes

github-actions Bot added the part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) label Sep 16, 2024

llucax reviewed Sep 17, 2024

View reviewed changes

Comment thread RELEASE_NOTES.md Outdated

Marenz force-pushed the runner branch 3 times, most recently from 1509e1b to ba6bd9f Compare September 23, 2024 15:42

llucax reviewed Sep 24, 2024

View reviewed changes

Marenz force-pushed the runner branch from ba6bd9f to de4682a Compare September 24, 2024 11:47

llucax reviewed Sep 24, 2024

View reviewed changes

This was referenced Sep 24, 2024

Pass the whole Dispatch object when reporting dispatch updates #55

Closed

Improve documentation example for DispatchManagingActor #56

Open

Marenz force-pushed the runner branch from de4682a to 74f20d4 Compare September 24, 2024 13:14

Marenz changed the title Dispatch Actor Runner Actor Dispatch Managing Actor Sep 24, 2024

Marenz force-pushed the runner branch 2 times, most recently from bc76d7e to 7003801 Compare September 24, 2024 16:14

Marenz removed this pull request from the merge queue due to a manual request Sep 30, 2024

Marenz dismissed llucax’s stale review via 29cbf75 September 30, 2024 08:14

Marenz force-pushed the runner branch from 75f5487 to 29cbf75 Compare September 30, 2024 08:14

llucax mentioned this pull request Sep 30, 2024

Improve resiliency of long-running async tasks frequenz-floss/frequenz-sdk-python#1081

Merged

llucax previously approved these changes Sep 30, 2024

View reviewed changes

Marenz added 11 commits September 30, 2024 12:52

Send STOPPED message when duration is reached

cd5bf23

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Add dispatch mananging actor

0b671ed

A useful actor to help control and manange another actor using dispatches. Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

DispatchManagingActor: Support starting/stopping of multiple actors

432b3cb

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Fix wrong Dispatch.running() behavior with duration=None

bcd5c0f

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

DispatchActor: Update timer correctly after schedule modifications

611a4eb

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Set dependency to latest dispatch-client

14d4510

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Update event_loop to explicitly set scope session

0287a09

As recommended by the deprecation warning Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Wrap scheduler to tolerate somehow invalid dispatches

820dcc3

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Add test that notifications are sent at startup

039d22e

Signed-off-by: Mathias L. Baumann <mathias.baumann@frequenz.com>

Marenz force-pushed the runner branch from 29cbf75 to 039d22e Compare September 30, 2024 10:56

llucax approved these changes Sep 30, 2024

View reviewed changes

Marenz added this pull request to the merge queue Sep 30, 2024

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 30, 2024

Marenz added this pull request to the merge queue Sep 30, 2024

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 30, 2024

Marenz added this pull request to the merge queue Sep 30, 2024

Merged via the queue into frequenz-floss:v0.x.x with commit 2606895 Sep 30, 2024

Marenz deleted the runner branch September 30, 2024 14:13

llucax mentioned this pull request Oct 1, 2024

Flaky hanging tests after merging #54 #66

Closed

llucax mentioned this pull request Oct 1, 2024

Tests sometimes hang #61

Closed

+                      match running:
+                          case RunningState.STOPPED:
+                              _logger.info("Stopping dispatch...")
+                              await self._stop_actors("Dispatch stopped")

               [tool.pytest.ini_options]
               testpaths = ["tests", "src"]
               asyncio_mode = "auto"
+              asyncio_default_fixture_loop_scope = "function"

+                  dry_run: bool
+                  """Whether this is a dry run."""
+                  payload: dict[str, Any]

Dispatch Managing Actor by Marenz · Pull Request #54 · frequenz-floss/frequenz-dispatch-python · GitHub

Repository navigation

Dispatch Managing Actor - #54

Dispatch Managing Actor#54Marenz merged 11 commits intofrequenz-floss:v0.x.xfrequenz-floss/frequenz-dispatch-python:v0.x.xfrom Marenz:runnerMarenz/frequenz-dispatch-python:runnerCopy head branch name to clipboard

Conversation

Marenz commented Sep 11, 2024

Uh oh!

daniel-zullo-frequenz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llucax Sep 13, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Marenz commented Sep 13, 2024

Uh oh!

llucax commented Sep 13, 2024

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llucax Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

Marenz Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

llucax Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Marenz commented Sep 24, 2024

Uh oh!

Marenz commented Sep 24, 2024

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

llucax Sep 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Marenz commented Sep 24, 2024

Uh oh!

llucax Sep 30, 2024

Choose a reason for hiding this comment

Uh oh!

Dispatch Managing Actor#54
Marenz merged 11 commits into
frequenz-floss:v0.x.xfrom
Marenz:runner