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

Add config option to disable profiling and disable it in many tests per default #6490

Merged
merged 9 commits into from
Jun 2, 2022

Conversation

hendrikmakait
Copy link
Member

@hendrikmakait hendrikmakait commented Jun 1, 2022

Closes #6075

This PR adds the distributed.worker.profile.enabled config option which defaults to True. If set to False, profiling is deactivated. For any tests that transitively make use of the distributed.utils_test._reconfigure() context manager, profiling is now deactivated unless explicitly requested.

/profile and /profile-server endpoints now show a message and have disabled buttons if profiling is deactivated:

/profile
ProfileDisabled

/profile-server
ProfileServerDisabled

Out of Scope:

  • Disable profiling per default for all tests. A follow-up issue will deal with the question if we should make _reconfigure() a global fixture or generally implement a global mechanism to set some config options per default

@mrocklin
Copy link
Member

mrocklin commented Jun 1, 2022

In principle this seems fine to me

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2022

Unit Test Results

       15 files         15 suites   6h 30m 11s ⏱️
  2 836 tests   2 752 ✔️   81 💤 3
21 016 runs  20 069 ✔️ 944 💤 3

For more details on these failures, see this check.

Results for commit 89cdea6.

♻️ This comment has been updated with latest results.

@hendrikmakait hendrikmakait marked this pull request as ready for review June 2, 2022 08:02
@hendrikmakait
Copy link
Member Author

Failing tests seem to be unrelated and have flaked before.

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.

Code looks good. One small question about a test but otherwise we're good to merge

ptp = ProfileServer(s)
assert "disabled" in ptp.subtitle.text
start = time()
await asyncio.sleep(1)
Copy link
Member

Choose a reason for hiding this comment

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

Do you actually need to wait for a second? with the configured intervals/cycles I would expect this to be a fraction of a second such that test runtime is lower

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to be on the safe side wrt. CI randomly suspending execution for some time. Writing this down, I realize that this would cause a false negative flake, i.e. this test would not fail even though it is broken, and it should still fail whenever CI does not suspend execution, very much including local machines. So I guess it's okay to reduce the sleep less and have this test occasionally pass on CI should it ever break.

Copy link
Member Author

Choose a reason for hiding this comment

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

Reduced to 100ms

@fjetter fjetter merged commit 6d85a85 into dask:main Jun 2, 2022
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.

Disable profiler in tests?
3 participants