Timer plugin #6
Timer plugin #6
Conversation
maxfischer2781
left a comment
maxfischer2781
left a comment
There was a problem hiding this comment.
Thanks for the PR, this looks like a useful tool.
There are quite a lot of comments from me on the implementation since it can be done much more efficiently. Please take them into account unless you have a good reason not to.
That said, I want to point out that your code was pretty good to understand; all these comments are there because you made it clear what you want to do.
Let's look at the docs and tests once the code has settled.
| def str_to_time(value: str) -> datetime_time: | ||
| """Convert a HH:MM string into a :class:`datetime.time` object.""" | ||
| try: | ||
| hours_str, minutes_str = value.strip().split(":", 1) | ||
| hours, minutes = int(hours_str), int(minutes_str) | ||
| except ValueError as exc: | ||
| raise ValueError(f"invalid time specification {value}") | ||
| return datetime_time(hour=hours, minute=minutes) |
There was a problem hiding this comment.
Unless I am missing some extra feature here, please use datetime's native parsing. The equivalent of this function is datetime.strptime(value, '%H:%M').time().
| assert interval > 0, "Interval must be a positive integer." | ||
| self.interval = interval |
There was a problem hiding this comment.
Since you are working on a schedule, there is no need to have a polling interval. You can simply asyncio.sleep until the next schedule time.
I recommend to just remove it. See comments on the run method for auxiliary changes.
| assert interval > 0, "Interval must be a positive integer." | |
| self.interval = interval |
|
|
||
| schedule = {str_to_time(key): value for key,value in schedule.items()} | ||
| self.schedule = schedule | ||
| self.latest_sched_demand = self._refresh_demand() |
There was a problem hiding this comment.
This attribute doesn't seem to be used outside of a redundant setter. I recommend to just remove it.
| self.latest_sched_demand = self._refresh_demand() |
| self, | ||
| target: Pool, | ||
| schedule: Schedule, | ||
| interval: int = 300, |
There was a problem hiding this comment.
See first comment on self.interval.
| interval: int = 300, |
| @demand.setter | ||
| def demand(self, value: float) -> None: | ||
| # Ignore user supplied demand and always enforce the scheduled value. | ||
| self.target.demand = self.latest_sched_demand |
There was a problem hiding this comment.
self.target.demand should already equal self.latest_sched_demand. You don't have to do anything here.
| self.target.demand = self.latest_sched_demand | |
| pass |
|
|
||
|
|
||
| @service(flavour=asyncio) | ||
| class Timer(PoolDecorator): |
There was a problem hiding this comment.
Maybe this should be a Controller instead. Since it doesn't allow setting the demand, there seems to be no point adding further Decorators or Controllers around it.
| schedule = {str_to_time(key): value for key,value in schedule.items()} | ||
| self.schedule = schedule |
There was a problem hiding this comment.
In _refresh_demand is an assertion that the value here is positive. Please do the validation here already. Since the data must be sorted for various algorithms, either checking or enforcing this here seems prudent.
I also recommend to rename the key/value to what they actually represent.
| schedule = {str_to_time(key): value for key,value in schedule.items()} | |
| self.schedule = schedule | |
| schedule = {str_to_time(time): demand for time, demand in schedule.items()} | |
| assert all(demand >= 0 for demand in schedule.values()) | |
| self.schedule = dict(sorted(schedule.items())) |
| while True: | ||
| self._refresh_demand() | ||
| await asyncio.sleep(self.interval) | ||
|
|
||
| def _refresh_demand(self) -> float: | ||
| """Look up the demand that should currently be active.""" | ||
| latest_sched_time = latest_timestamp(self.schedule.keys()) | ||
| self.latest_sched_demand = self.schedule[latest_sched_time] | ||
| self.target.demand = self.latest_sched_demand | ||
|
|
||
| assert self.latest_sched_demand >= 0.0 | ||
| assert self.target.demand >= 0.0 | ||
|
|
||
| return self.latest_sched_demand |
There was a problem hiding this comment.
Since you are operating on a fixed schedule, there is no need to poll and fetch the current time slot again and again. Instead, I recommend to endlessly loop through the schedule and sleep precisely until you have to take action.
| while True: | |
| self._refresh_demand() | |
| await asyncio.sleep(self.interval) | |
| def _refresh_demand(self) -> float: | |
| """Look up the demand that should currently be active.""" | |
| latest_sched_time = latest_timestamp(self.schedule.keys()) | |
| self.latest_sched_demand = self.schedule[latest_sched_time] | |
| self.target.demand = self.latest_sched_demand | |
| assert self.latest_sched_demand >= 0.0 | |
| assert self.target.demand >= 0.0 | |
| return self.latest_sched_demand | |
| # repeatedly sleep until the next scheduled change of demand | |
| # this automatically fast-forwards through past events, meaning | |
| # no special handling for the first day is needed | |
| today = date.today() | |
| while True: | |
| for start_time, demand in schedule.items(): | |
| start_delta = datetime.combine(today, start_time) - datetime.now() | |
| await asyncio.sleep(start_delta.total_seconds()) | |
| self.target.demand = demand | |
| today += timedelta(days=1) |
The timedelta must also be imported from datetime.
| TimeInput = Union[str, datetime_time] | ||
| Schedule = Mapping[TimeInput, float] |
There was a problem hiding this comment.
The initializer always calls str_to_time on the Schedule key, meaning that only str keys are acceptable.
| def latest_timestamp( | ||
| times: Iterable[datetime_time], | ||
| *, | ||
| reference: datetime_time | None = None, # easier to test with | ||
| ) -> datetime_time: | ||
| """Return the latest timestamp with respect to the current time.""" | ||
| ordered_times = sorted(times) # ascending | ||
| current_time = datetime.now().time() if reference is None else reference | ||
| for candidate in reversed(ordered_times): # descending | ||
| if candidate <= current_time: | ||
| return candidate | ||
| # All configured times are in the future relative to ``current_time``. | ||
| # return latest timestamp | ||
| return ordered_times[-1] |
There was a problem hiding this comment.
This is probably no longer needed, see later comments. If you want to keep it:
current_timestamporrecent_timestampis probably a better name (without knowing the implementation, "latest" is likely to be read as "most late" instead of "most recent").- ordering should already be a prerequisite and established outside this function. Sorting the input again and again is a waste since a) the actual user input is likely sorted anyways and b) it's simple to sort once when reading the input.
2 participants
The Timer plugin enforces a recurring, time-of-day schedule for the demand of a target pool (see doc).
Implemented: