Skip to content
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

Restructure test_watch_requires_lock_to_run to avoid flakes #6469

Merged
merged 3 commits into from
May 31, 2022

Conversation

hendrikmakait
Copy link
Member

distributed/tests/test_profile.py fails occasionally (e.g., see #6444 (comment)). This test restructures the test to avoid timing-based flakes.

Closes #xxxx

  • Tests added / passed
  • Passes pre-commit run --all-files

)
thread.daemon = True
thread.start()

log = watch(interval="10ms", cycle="50ms", stop=stop_profile)
log = watch(interval="10ms", cycle="50ms", stop=stop_profiling)

start = time() # wait until thread starts up
Copy link
Member

@graingert graingert May 27, 2022

Choose a reason for hiding this comment

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

how about using an event to wait for the profiler thread to start, and grabbing the profile_thread in a nonlocal?

stop_profiling_called = threading.Event()
profile_thread = None

def stop_profiling():
    nonlocal profile_thread
    profile_thread = threading.current_thread()
    stop_profiling_called.set()

    return time() > start + 0.500

stop_profiling_called.wait(2)
sleep(0.5)
assert len(log) == 0
release_lock = True

profile_thread.join(2)
thread.join(2)

Copy link
Member Author

@hendrikmakait hendrikmakait May 27, 2022

Choose a reason for hiding this comment

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

Instead of

start = time() # wait until thread starts up
while threading.active_count() < start_threads + 2:
assert time() < start + 2
sleep(0.01)
you mean? [The edit made it clearer]. Good idea, I just stole that logic from test_watch, so I'll adjust both of them for more reliable behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, adjusted the structure to be more event-based.

@github-actions
Copy link
Contributor

github-actions bot commented May 27, 2022

Unit Test Results

       15 files  ±    0         15 suites  ±0   5h 43m 1s ⏱️ - 50m 6s
  2 821 tests +    2    2 736 ✔️ +    9    82 💤 +  1  3  - 8 
20 595 runs   - 301  19 675 ✔️  - 270  915 💤  - 24  5  - 7 

For more details on these failures, see this check.

Results for commit 06500d4. ± Comparison against base commit 1346671.

♻️ This comment has been updated with latest results.

@hendrikmakait
Copy link
Member Author

Failures are unrelated.

@mrocklin
Copy link
Member

@graingert do you approve (I'm not reviewing this closely)

@mrocklin mrocklin merged commit c2b28cf into dask:main May 31, 2022
crusaderky pushed a commit to crusaderky/distributed that referenced this pull request Jun 1, 2022
)

distributed/tests/test_profile.py fails occasionally (e.g., see dask#6444 (comment)). This test restructures the test to avoid timing-based flakes.
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.

3 participants