Dispatch Managing Actor #54

Marenz · 2024-09-11T15:30:30Z

Reset release notes
Add dispatch runner

daniel-zullo-frequenz

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

tests/test_runner.py

src/frequenz/dispatch/_actor_runner.py

llucax

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

src/frequenz/dispatch/_actor_runner.py

llucax · 2024-09-13T07:00:14Z

src/frequenz/dispatch/_actor_runner.py

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


Maybe "Stopped by a dispatch".

src/frequenz/dispatch/_actor_runner.py

Marenz · 2024-09-13T08:51:28Z

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

llucax · 2024-09-13T10:26:55Z

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.

llucax

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.

RELEASE_NOTES.md

llucax

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.

src/frequenz/dispatch/_actor_runner.py

llucax · 2024-09-24T06:41:49Z

src/frequenz/dispatch/_actor_runner.py

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


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.

src/frequenz/dispatch/_actor_runner.py

src/frequenz/dispatch/_dispatch.py

src/frequenz/dispatch/actor.py

tests/test_frequenz_dispatch.py

Marenz · 2024-09-24T07:51:56Z

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

Marenz · 2024-09-24T11:47:47Z

Addressed points 1 - 5

llucax

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.

src/frequenz/dispatch/_managing_actor.py

src/frequenz/dispatch/_actor_runner.py

llucax · 2024-09-24T12:48:01Z

src/frequenz/dispatch/_actor_runner.py

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


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.

src/frequenz/dispatch/_managing_actor.py

tests/test_mananging_actor.py

Marenz · 2024-09-24T13:14:46Z

Added multiple actors back
fixed confi -> up stuff

llucax · 2024-09-30T08:25:24Z

pyproject.toml

@@ -165,6 +165,7 @@ disable = [
 [tool.pytest.ini_options]
 testpaths = ["tests", "src"]
 asyncio_mode = "auto"
+asyncio_default_fixture_loop_scope = "function"


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 <[email protected]>

* 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 <[email protected]>

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 <[email protected]>

A useful actor to help control and manange another actor using dispatches. Signed-off-by: Mathias L. Baumann <[email protected]>

Signed-off-by: Mathias L. Baumann <[email protected]>

As recommended by the deprecation warning Signed-off-by: Mathias L. Baumann <[email protected]>

Signed-off-by: Mathias L. Baumann <[email protected]>

The merge-base changed after approval.

llucax · 2024-10-01T06:54:51Z

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

tests/test_runner.py Outdated Show resolved Hide resolved

tests/test_runner.py Outdated Show resolved Hide resolved

src/frequenz/dispatch/_actor_runner.py Outdated Show resolved Hide resolved

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

RELEASE_NOTES.md Outdated Show resolved Hide resolved

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

Open

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 <[email protected]>

Add dispatch mananging actor

0b671ed

A useful actor to help control and manange another actor using dispatches. Signed-off-by: Mathias L. Baumann <[email protected]>

DispatchManagingActor: Support starting/stopping of multiple actors

432b3cb

Signed-off-by: Mathias L. Baumann <[email protected]>

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

bcd5c0f

Signed-off-by: Mathias L. Baumann <[email protected]>

DispatchActor: Update timer correctly after schedule modifications

611a4eb

Signed-off-by: Mathias L. Baumann <[email protected]>

Set dependency to latest dispatch-client

14d4510

Signed-off-by: Mathias L. Baumann <[email protected]>

Update event_loop to explicitly set scope session

0287a09

As recommended by the deprecation warning Signed-off-by: Mathias L. Baumann <[email protected]>

Wrap scheduler to tolerate somehow invalid dispatches

820dcc3

Signed-off-by: Mathias L. Baumann <[email protected]>

Add test that notifications are sent at startup

039d22e

Signed-off-by: Mathias L. Baumann <[email protected]>

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
14 checks passed

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dispatch Managing Actor #54

Dispatch Managing Actor #54

Marenz commented Sep 11, 2024

daniel-zullo-frequenz left a comment

llucax left a comment

llucax Sep 13, 2024

Marenz commented Sep 13, 2024

llucax commented Sep 13, 2024

llucax left a comment

llucax left a comment

llucax Sep 24, 2024

Marenz Sep 24, 2024

llucax Sep 24, 2024

Marenz commented Sep 24, 2024

Marenz commented Sep 24, 2024

llucax left a comment

llucax Sep 24, 2024

Marenz commented Sep 24, 2024

llucax Sep 30, 2024

llucax commented Oct 1, 2024

Dispatch Managing Actor #54

Dispatch Managing Actor #54

Conversation

Marenz commented Sep 11, 2024

daniel-zullo-frequenz left a comment

Choose a reason for hiding this comment

llucax left a comment

Choose a reason for hiding this comment

llucax Sep 13, 2024

Choose a reason for hiding this comment

Marenz commented Sep 13, 2024

llucax commented Sep 13, 2024

llucax left a comment

Choose a reason for hiding this comment

llucax left a comment

Choose a reason for hiding this comment

llucax Sep 24, 2024

Choose a reason for hiding this comment

Marenz Sep 24, 2024

Choose a reason for hiding this comment

llucax Sep 24, 2024

Choose a reason for hiding this comment

Marenz commented Sep 24, 2024

Marenz commented Sep 24, 2024

llucax left a comment

Choose a reason for hiding this comment

llucax Sep 24, 2024

Choose a reason for hiding this comment

Marenz commented Sep 24, 2024

llucax Sep 30, 2024

Choose a reason for hiding this comment

llucax commented Oct 1, 2024