Deprecate cleanup fixture; replace with gonna_run_dask fixture

[gjoseph92]

Addresses #6822 (comment), where some tests were using the cleanup fixture for its behavior setting default config values (such as disabling the profiler).

I didn't like that muddying of concerns. cleanup should just assert that things aren't leaked. If you want config values, that should be separate IMO.

cleanup seemed to be currently used like "throw this in whenever you're going to launch a dask cluster, can't hurt". I just don't like the naming of that, so this adds a new gonna_run_dask fixture for convenience, which combines cleanup + config_for_cluster_tests (which is exactly what cleanup used to be be before #6822).

This deprecates the cleanup fixture right now and raises an error pointing you to gonna_run_dask. Maybe that's a bad idea? I just didn't see many/any standalone uses of cleanup that actually seemed necessary (either in things that weren't making threads/processes, or already were using loop, which runs cleanup itself).

This would break things for any downstream projects using cleanup directly.

cc @graingert

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

[gjoseph92]

I'm really unsure whether or not I like this. gonna_run_dask generally seems like a good idea, but IDK if cleanup should be deprecated. It's also not clear to me whether anywhere you'd use gonna_run_dask you should just be using other things like loop, cluster, gen_cluster, etc.

The way in which fixtures are used seems very tangled and inconsistent. Maybe we should just put config_for_cluster_tests back inside cleanup and not touch anything, and not try to impose any logical structure.

[github-actions]

Unit Test Results

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

       15 files  ±0         15 suites  ±0   6h 31m 10s ⏱️ + 3m 20s
  2 992 tests ±0    2 904 ✔️ +2       88 💤 ±0  0 ❌  - 2 
22 189 runs  ±0  21 139 ✔️ ±0  1 050 💤 +2  0 ❌  - 2 

Results for commit 6386486. ± Comparison against base commit bf53760.

♻️ This comment has been updated with latest results.

[fjetter]

The "what fixtures should be deprecated" conversation probably belongs into #6806

I suggest to get started on that one before introducing any deprecations really. We can define the fixtures we want to be public there and then deprecate basically all of utils_test in one go in favor of a public testing module and a private one. The private one can then be iterated on quickly to find a good match for our needs

[graingert]

Suggested change

	raise RuntimeError(
	"The `distributed.utils_test.cleanup` fixture is deprecated. "
	"You should not be using this fixture directly. In most cases, use:\n"
	" * `gen_cluster` if you just need to run a cluster\n"
	" * `client` fixture or `cluster` contextmanager if you need a cluster in subprocesses\n"
	" * `loop` + `popen` to call the dask CLIs yourself, and connect a Client to it using the `loop`\n"
	"These methods all will check for resource leaks and set up default config values for you. "
	"If none of these methods work, and you want to continue launching dask your own (likely deprecated) "
	"way in tests (like calling `Client(..., loop=None)`, then use the `gonna_run_dask` fixture to "
	"set config values for tests, and check for resource leaks."
	)

[graingert]

we're going to break everyone who uses this fixture anyway, so we can just use the fact that independent fixtures need importing even if they are added as simple dependencies to get the news out

Suggested change

	@pytest.fixture
	def cleanup():
	@pytest.fixture
	def cleanup(gonna_run_dask):
	# cleanup is deprecated in favor of gonna_run_dask see https://github.com/dask/distributed/pull/6854/
	return gonna_run_dask

[graingert]

I don't like the name "gonna_run_dask", however once "cleanup" had this new name it was obvious to me which tests didn't need it, so clearly the new name is better, and I'm happy to go with it if nothing else is suggested, which puts me at a begrudging +1

the problems I have with the name are:

• it seems a bit informal, which is ok,
• it's technically wrong because really it's nothing to do with dask it's specifically about "distributed"

some paints for your bikeshed:

@pytest.fixture
def distributes():
    with _check_clean(), config_for_cluster_tests():
        yield

[ian-r-rose]

I kind of like the informality of gonna_run_dask (though I take your point that gonna_run_distributed is a bit more accurate). Having the more conversational tone is one way of signposting that this is, in some sense, not the usual path for tests. Reminds me a bit of React's dangerouslySetInnerHTML.