Skip to content

Make the timer generic and support periodic timers #79

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 20 commits into from
May 9, 2023
Merged

Make the timer generic and support periodic timers #79

merged 20 commits into from
May 9, 2023

Conversation

@leandro-lucarella-frequenz
Copy link
Contributor

@leandro-lucarella-frequenz leandro-lucarella-frequenz commented Mar 15, 2023 •
edited
Loading

The current timer provided is very convenient to implement timers, but it's not very helpful to implement a peridic timer that doesn't accumulate drift.

This PR makes the timer configurable via missing ticks policies and adds 3 policies: TriggerAllMissing, SkipMissingAndResync and SkipMissingAndDrift.

For convenience, 2 alternative constructors are provided for the most common cases: Timer.timeout() and Timer.periodic(). Timeout timers always drift and periodic timers never.

The new Timer also uses the asyncio loop monotonic clock, and returns the drift (difference between when the timer should have triggered and it actually did) instead of a datetime with the time of trigger.

It is also now possible to control when this timer is started, and can be stopped and reset/restarted at will.

Fixes #78.

@leandro-lucarella-frequenz leandro-lucarella-frequenz added this to the v0.14.0 milestone Mar 15, 2023
@leandro-lucarella-frequenz leandro-lucarella-frequenz added the type:enhancement New feature or enhancement visitble to users label Mar 15, 2023
@github-actions github-actions bot added part:docs Affects the documentation part:core Affects the core types (`Sender`, `Receiver`, exceptions, etc.) labels Mar 15, 2023
@leandro-lucarella-frequenz
Copy link
Contributor Author

I'm marking this as a draft only because it is lacking tests, but it should be ready to review (I did some basic testing manually). I would like to get feedback before spending too much time with tests (as I would like to introduce also async-solipsism here).

@github-actions github-actions bot added part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) labels Mar 16, 2023
@leandro-lucarella-frequenz leandro-lucarella-frequenz mentioned this pull request Mar 16, 2023
Closed
sahas-subramanian-frequenz
sahas-subramanian-frequenz reviewed Apr 17, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks good to me. If we don't go the consolidating TimerTick route I mentioned, we might want to add a docstring line here explaining the difference from utils.Timer, and a line in utils.Timer saying when people should use a PeriodicTimer instead.

@github-actions github-actions bot removed the part:tests Affects the unit, integration and performance (benchmarks) tests label Apr 20, 2023
@leandro-lucarella-frequenz
Copy link
Contributor Author

Updated according to what we talked about. So now PeriodicTimer can have different behaviors when ticks are missed and can replace Timer, which I misunderstood how it worked. It is still a WIP but the implementation of the timer should be "finished" (I only need to review the docs). On some local tests it seems to work as expected.

I didn't include the datetime as we don't have any use cases for it yet and it is complex enough as it is, we can add it in the future if needed.

The tests are failing because of styling issues, it would be good if you can review it @sahas-subramanian-frequenz to see if you are good with it now so I can finish the docs, PR and removing Timer.

@github-actions github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Apr 27, 2023
@leandro-lucarella-frequenz
Copy link
Contributor Author

Fixed some bugs and added some tests. I need to add some more but I consider this almost done. Not ready for merging because of the above, but not a draft anymore either.

@leandro-lucarella-frequenz leandro-lucarella-frequenz marked this pull request as ready for review April 28, 2023 14:19
@leandro-lucarella-frequenz leandro-lucarella-frequenz requested a review from a team as a code owner April 28, 2023 14:19
@leandro-lucarella-frequenz leandro-lucarella-frequenz marked this pull request as draft May 3, 2023 07:29
@leandro-lucarella-frequenz
Copy link
Contributor Author

Since nobody looked at this yet, I'm converting to draft again because when doing more tests and using hypothesis to test better some cases, it become evident that using floats internally and doing modulo math and comparisons is very problematic in cases where values differ a lot in magnitude or significant digits, so I'm considering to use int internal representation (in ms or so).

I also don't like that delay_tolerance doesn't apply to every missed tick behavior, and that we need to handle every behavior in ready(), which is not scalable nor customizable, so I'll use classes to represent behaviors instead.

So a couple of major changes are coming.

@leandro-lucarella-frequenz leandro-lucarella-frequenz marked this pull request as ready for review May 4, 2023 11:20
@leandro-lucarella-frequenz
Copy link
Contributor Author

This PR is now ready for review for real. There are still quite a few commits that could be squashed together, but since this PR took so much work to get right (still to be seen :D), I'd prefer to keep the history as is, as it shows better the problems found while developing it. I even wonder if the problems found here with floats might affect the SDK too, in particular classes doing alignment over floats (like the resampler).

Because of the above, I recommend to only go commit-by-commit to have a quick look to see if commits are consistent, but for the logic, it is probably better to review the final diff, because both the way the timer presents time internally and the missing tick policies were completely rewritten in the PR itself.

daniel-zullo-frequenz
daniel-zullo-frequenz reviewed May 8, 2023
View reviewed changes
llucax added 13 commits May 8, 2023 15:14
@llucax
Add a periodic timer receiver
77cfb34 
The current timer provided is very convenient to implement timers, but
it's not very helpful to implement a peridic timer that doesn't
accumulate drift.

This commit adds a `util.PeriodicTimer` that ensures that drift is not
accumulated and that the receiver will be ready exactly after one
`interval` has passed (not counting any delay introduced by the event
loop being busy with another task).

It is also possible to control when this timer is started, and can be
stopped and reset/restarted.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Add missing ticks behavior
4c59581 
The `PeriodicTimer` can now be created using different ways to deal with
missing ticks, including what the current `Timer` does.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Remove util.Timer
5514359 
`Timer` is replaced by more flexible `PeriodicTimer`.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Re-format pyproject.toml
f7aab65 
According to https://taplo.tamasfe.dev/.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Set the default tolerance to 0 for SKIP_AND_DRIFT
859fe6e 
This is the most sensible default for this tick behaviour.

At some point we might want to refactor this so behaviours are classes
instead of an enum, so they can take their own options (with the
defaults that make more sense) and even calculate the next tick on their
own.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Move the missing tick handling to a separate function
73f9e00 
The ready() method got too long, so it makes sense to split the missing
tick handling to a separate method.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Convert MissedTickBehavior to classes
2749d89 
There are several advantages of using classes instead of enum for the
missed tick behavior:

* The `delay_tolerance` option doesn't make sense for all behavior, so
  now each behavior can have its own parameters
* New behaviors can be implemented without having to modify the
  `PeriodicTimer`
* Easier to test

This commit doesn't update the tests because when trying to update them
and use the `hypothesis` package to test the different behaviors, it was
evident that using floats to represent times is a recipe for disaster,
specially when the monotonic clock is way bigger than the interval. For
behaviors when more math than a simple addition is needed (for example,
modulo), it is very likely to have wrong calculations on those
circumstances.

It is even necessary to adapt the current tests to use a bit less than
the tolerance in some case to cope with floating point errors.

A following commit will use `int`s internally to represent the time,
intervals and drifts.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Use int to represent time internally
aeec684 
As pointed out in the previous commit, using float to represent time
can lead to extremely hard to debug issues in cases where the magnitude
of the amounts to compare or operate with, or the amount of relevant
digits are very different.

To avoid this we use ints to represent time internally, all in
microseconds.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Rearrange calculate_next_tick_time() arguments
29e6a69 
This is a aesthetic change, just to group both argument that represent
times (a tick happened and should have happened).

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Rename MissedTickBehavior to MissedTickPolicy
4349244 
The name behavior was inspired by Rust's Tokio timers (Interval[1]), but
it doesn't sound very nicely when talking about a "behavior". A policy
seems to be a better match for this.

[1] https://docs.rs/tokio/latest/tokio/time/fn.interval.html

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Add missing test descriptions
5070159 
Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Make sure the tolerance is positive
9b7857d 
Otherwise nasty things can happen.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Add more tests to the PeriodicTimer
ff74826 
We use the hypothesis library to test policies more thoroughly and add
some more tests for timer construction and most notably a test for a
timer using the `SkipMissedAndResync` policy.

This commit also include some minor naming and documention improvements
in existing tests.

Signed-off-by: Leandro Lucarella <[email protected]>
daniel-zullo-frequenz
daniel-zullo-frequenz reviewed May 8, 2023
View reviewed changes
Copy link
Contributor

@daniel-zullo-frequenz daniel-zullo-frequenz left a comment

Choose a reason for hiding this comment

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

I only have a cosmetic comment to check for and LGTM. Really good work with the documentation and examples which helps a lot to the review process, also the changes well separated in different commits

llucax added 5 commits May 8, 2023 15:20
@llucax
Rename PeriodicTimer to Timer
8e540d0 
The current implementation is really not a periodic timer, that's
determined by the used missed ticks policy, so it makes sense to go back
to call it just Timer.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Make missed tick policy argument mandatory
06873a0 
Because now the timer is generic, it doesn't make a lot of sense to
provide a default policy for missing ticks.  The argument is also now
a positional-only argument, as is not optional and has a very specific
type it makes more sense to avoid the keyword argument.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Fix positional-only arguments
652f075 
The syntax is `f(pos_only, /)`, not `f(/, pos_only)`.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Add constructors for timeouts and periodic timers
5e772ce 
This commit adds two new constructors: `timeout(delay)` and
`periodic(period, skip_missed_ticks=False)` as these are the most common
use cases.  This should provide a more user friendly experience for the
vast majority of users.

Signed-off-by: Leandro Lucarella <[email protected]>
@llucax
Update RELEASE_NOTES for the next release
390439f 
Add missing changes and a summary.

Signed-off-by: Leandro Lucarella <[email protected]>
@leandro-lucarella-frequenz
Copy link
Contributor Author

Updated to remove all mentions to castrated ram (wether) :trollface:

sahas-subramanian-frequenz
sahas-subramanian-frequenz approved these changes May 8, 2023
View reviewed changes
Copy link
Contributor

Choose a reason for hiding this comment

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

I like the new naming scheme so much better than PeriodicTimer 🎉

@leandro-lucarella-frequenz
Copy link
Contributor Author

I know, and me too. Thanks for pushing back!

@leandro-lucarella-frequenz leandro-lucarella-frequenz changed the title Add a periodic timer receiver Make the timer generic and support periodic timers May 9, 2023
@leandro-lucarella-frequenz leandro-lucarella-frequenz merged commit 5126a17 into frequenz-floss:v0.x.x May 9, 2023
@leandro-lucarella-frequenz leandro-lucarella-frequenz deleted the periodic-timer branch May 9, 2023 07:18
leandro-lucarella-frequenz pushed a commit that referenced this pull request May 9, 2023
@llucax
Fix FileWatcher DELETE event reporting and rearrange tests (#86)
8ae4186 
This pull request have 2 different but related main changes:

* The `FileWatcher` was skipping `DELETE` events, except on very weird
cases where the file was deleted and re-created just before the event
was triggered by `awatch()`.

This also extends the integration test to test both situations (delete
events when a file exist and when it doesn't).

Because there is usually a considerable delay between a change in the
filesystem happens and `awatch()` emits an event it is difficult to
decide if we should filter events based on the file existing or not. For
example, if a create event is received but before is triggered the file
was removed, should we emit the create event or not? Or the other way
around, should we emit a delete event if a file was created again before
the event was triggered.

It is probably best to leave this decision to the user, and let them
deal with races.

* `FileWatcher` tests were rearranged and split between unit and
integration tests.

The existing tests were really integration tests, as they are not only
testing `FileWatcher`, but also `Select` and `PeriodicTimer`, which is
confusing when there is a bug in any of the other classes as we might
get test failures here (this actually happened during the development of
#79).
    
Because of this the current tests were moved to
`test/util/test_integration.py`. We also mark the tests with the
`pytest` mark `integration` to make it more clear they are integration
tests, and to enable an easy way to only run (exclude) integration
tests.

New unit tests were added to the current test file, properly mocking
`awatch` to avoid interactions with the OS.

There are many more integration tests posing as unit tests, but this PR
only takes care of `FileWatcher`. With time we should move the rest too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@daniel-zullo-frequenz daniel-zullo-frequenz daniel-zullo-frequenz approved these changes

@matthias-wende-frequenz matthias-wende-frequenz Awaiting requested review from matthias-wende-frequenz

@mathias-baumann-frequenz mathias-baumann-frequenz Awaiting requested review from mathias-baumann-frequenz

+1 more reviewer

@sahas-subramanian-frequenz sahas-subramanian-frequenz sahas-subramanian-frequenz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@leandro-lucarella-frequenz leandro-lucarella-frequenz

Labels

part:core Affects the core types (`Sender`, `Receiver`, exceptions, etc.) part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:tooling Affects the development tooling (CI, deployment, dependency management, etc.) type:enhancement New feature or enhancement visitble to users

Projects

None yet

Milestone

v0.15.0

Development

Successfully merging this pull request may close these issues.

Add a periodic timer

4 participants

@leandro-lucarella-frequenz @sahas-subramanian-frequenz @daniel-zullo-frequenz @llucax