Initial structure

[Marenz]

• Prepare dependencies for dispatch
• Fix labeler path
• Add initial structure

[shsms]

Could you describe the motivation for this change? There's from frequenz import dispatch now? Aren't we only having namespace packages under frequenz like frequenz.{actor, client, api, etc} and putting code under that?

Or is this going to be a completely independent thing like frequenz.channels?

[Marenz]

The structure is a kind of experiment that luca suggested to me to try.
Basically, as the user shouldn't interact directly with the actor, the high-level interface should just directly offer the dispatch arrived/is ready channels like shown in https://github.com/frequenz-io/frequenz-apps-draft/blob/main/dispatch_exec/dispatch.py

[shsms]

maybe more readable with:

while next_time := self.calculate_next_run(dispatch, datetime.now(tz=timezone.utc)):
    ...

# and after the loop
logging.info("Dispatch finished: %s", dispatch)
self._scheduled.pop(dispatch.id)

[Marenz]

then I can't reuse now easily with your suggestion, but I modified it a bit..

        def next_run_info() -> tuple[datetime, datetime] | None:
            now = datetime.now(tz=timezone.utc)
            next_run = self.calculate_next_run(dispatch, now)

            if next_run is None:
                return None

            return now, next_run

        while pair := next_run_info():
            now, next_time = pair

            if next_time - now > _MAX_AHEAD_SCHEDULE:
                await asyncio.sleep(_MAX_AHEAD_SCHEDULE.total_seconds())
                continue

            _logger.info("Dispatch %s scheduled for %s", dispatch.id, next_time)
            _logger.info("Sleeping for %s", next_time - now)
            await asyncio.sleep((next_time - now).total_seconds())

            _logger.info("Dispatch ready: %s", dispatch)
            await self._ready_dispatch_sender.send(dispatch)

        _logger.info("Dispatch finished: %s", dispatch)
        self._scheduled.pop(dispatch.id)

[shsms]

The structure is a kind of experiment that luca suggested to me to try.
Basically, as the user shouldn't interact directly with the actor, the high-level interface should just directly offer the dispatch arrived/is ready channels like shown in https://github.com/frequenz-io/frequenz-apps-draft/blob/main/dispatch_exec/dispatch.py

Then maybe we keep frequenz.actor.dispatch as it is, and add frequenz.dispatch separately, such that frequenz.dispatch can eventually go into the SDK?

Either way, the reason for the change is not obvious at all, so it would be worth the effort to explain it in the commit message and in the PR description.

[Marenz]

I don't see it much as "change of structure", given that previously there was basically nothing except empty or dummy files ;)

Anyway, I found what I was missing. The structure change is an attempt to test the rewrite of the structure according to https://github.com/frequenz-floss/frequenz-sdk-python/wiki/APIs-stack-and-repositories-structure#completely-rewrite-of-the-structure

[Marenz]

• Added link to structure rewrite description in commit
• implemented proper scheduling with recurrence based on python-dateutils
• can use more tests, but that can be added later (test suggestions welcome, part from the create/delete/update tests that are there/planned)

[shsms]

Looks good to me.

[stefan-brus-frequenz]

Suggestion:

This class provides a highlevel interface to the dispatch API. It 
allows for receiving of new dispatches, and dispatches labelled as 'ready'.

I would also suggest adding a small blurb about the difference between the two.

[stefan-brus-frequenz]

Could you clarify how this happens in this doc?

For example, "- If the start time of the new dispatch is different from the old one, the old one is cancelled."

[stefan-brus-frequenz]

Added a couple of optional comments about the docs, code overall LGTM, but my python knowledge is limited.

[llucax]

OK, quite a few comments, in particular about the design.

Important

Please let's not discuss the topics here, as discussion could get lengthy. I will create separate issues (or PRs where possible) and link them when I'm done.

[llucax]

My mental picture of how this should have worked was, instead of having one task per dispatch, to just have one task that calculates the next run for all dispatches and task the one that needs to run next, then sleeps only for that time, sends the ready dispatch through the channel, and calculates the next dispatch to be run again.

I think this scheme should be simpler and more memory efficient, as you don't need one task per dispatch. Do you see any advantages with the current approach?

I guess it might be a bit more CPU efficient, as the next dispatch for all dispatches doesn't need to be calculated every time a dispatch is ready, but this could be optimized fairly easily by saving all the next run calculation for all dispatches in a sorted list and only calculate the next run for the dispatch that was just ready. Then we just need to invalidate this list each time dispatches are changed.

[llucax]

This one might be too much work to implement, so I think it is better to discuss it first, and probably it is better to sense the general feeling here first, if it's not a trivial "let's go with this approach" (or let's NOT) before creating an issue, so please go ahead and leave comments here.

[Marenz]

I think my only concern with your suggestion is that it might accidentally skip dispatches that are very close together, though I can't come up for a good explanation why it would, just an .. gut feeling to pay extra attention on that area when implementing it..

I was also wondering if the awaiting for sending might take a bit of time and thus delays the notification for the next very close dispatch to be executed. What if we waited longer than the next dispatches scheduled time, how do we make sure we don't skip it?

[llucax]

I think my only concern with your suggestion is that it might accidentally skip dispatches that are very close together, though I can't come up for a good explanation why it would, just an .. gut feeling to pay extra attention on that area when implementing it..

If done properly, I don't think it should. I didn't invented this, I've seen this (or a similar approach) suggested in libev as a very efficient way to implement timers (see point 4).

I was also wondering if the awaiting for sending might take a bit of time and thus delays the notification for the next very close dispatch to be executed. What if we waited longer than the next dispatches scheduled time, how do we make sure we don't skip it?

If we await on the send takes a long time, I guess the next notification will be delayed too as it will use the same channel to notify, right? We could do the send in a separate task but then we end up with many tasks again, but I think sends will be serialized in any case because they operate in the same channel, so using a different task is probably not having any real effect.

[llucax]

Is this cast needed because dateutil lacks type info or what?

[llucax]

This one is too small to create an issue. So also please comment here.

[Marenz]

Yah, I was getting src/frequenz/dispatch/actor.py:279: error: Returning Any from function declared to return "datetime | None" [no-any-return]

[llucax]

OK, it is probably best to add a comment when using cast, so readers don't have to guess in the future.