Skip to content

metrics.monotonic() is not monotonic on Windows #7558

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.

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

Merged
merged 1 commit into from
Feb 20, 2023

Conversation

crusaderky
Copy link
Collaborator

@crusaderky crusaderky commented Feb 20, 2023

Every 10 minutes, the monotonic timer can go backwards a few milliseconds. This leads to unit test failures such as

t0 = monotonic()
sleep(0.2)
t1 = monotonic()
assert 0.2 <= t1 - t0

AssertionError:
assert 0.2 <= 0.19631620000018302

@crusaderky crusaderky self-assigned this Feb 20, 2023
@github-actions
Copy link
Contributor

Unit Test Results

See test report for an extended history of previous test failures. This is useful for diagnosing flaky tests.

       24 files  ±  0         24 suites  ±0   10h 23m 15s ⏱️ - 8m 14s
  3 337 tests +  1    3 235 ✔️ +1     102 💤 ±0  0 ±0 
39 343 runs  +12  37 481 ✔️ +5  1 862 💤 +7  0 ±0 

Results for commit 4f7a4f6. ± Comparison against base commit fef78d4.

@crusaderky
Copy link
Collaborator Author

coverage failure is due to #7469
Ready for review and merge

Copy link
Member

@fjetter fjetter left a comment

Choose a reason for hiding this comment

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

This resync looks a bit scary, especially since it's a potentially endless running loop that may or may not be triggered whenever we call time.

while True:
    do_stuff()
    if not_fast_enough():
        continue

That's obviously not new... new/changed code LGTM


perf_times = [t[1] for t in times if t[0] == first][:-1]
assert len(perf_times) >= min_samples - 1, perf_times
self.delta = first - sum(perf_times) / len(perf_times)
Copy link
Member

Choose a reason for hiding this comment

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

I know this code was not added in this PR so feel free to ignore.

I'm struggling to understand what this delta is representing. Is this trying to estimate the resolution?

@fjetter
Copy link
Member

fjetter commented Feb 20, 2023

FWIW There is an interesting conversation about the monotonic windows clock over in python/cpython#88494
This problem might go away eventually if CPython decides to use a different API to get the time from Windows but this was already moved from 3.11 to 3.12 so nothing we can wait for.

@crusaderky crusaderky merged commit e387b76 into dask:main Feb 20, 2023
@crusaderky crusaderky deleted the windows_monotonic branch February 20, 2023 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants