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

Fix CI by using client.persist(collection) instead of collection.persist() #9020

Merged
merged 4 commits into from
Mar 12, 2025

Conversation

hendrikmakait
Copy link
Member

CI has been pretty broken ever since we merged dask/dask#11790.

This PR fixes that.

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

@jacobtomlinson
Copy link
Member

Am I right in understaning that this is because collection.persist() no longer works with an async client?

@hendrikmakait
Copy link
Member Author

Am I right in understaning that this is because collection.persist() no longer works with an async client?

Yes, this raises by default. There is a fallback option but that also warns, so I opted for the explicit usage of the async client.

@@ -3407,68 +3407,6 @@ def test_default_get(loop_in_thread):
assert dask.base.get_scheduler() == pre_get


@gen_cluster(config={"scheduler": "sync"}, nthreads=[])
async def test_get_scheduler_default_client_config_interleaving(s):
Copy link
Member Author

Choose a reason for hiding this comment

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

get_scheduler() never returns an async client now, so this test has become obsolete.

@hendrikmakait hendrikmakait marked this pull request as ready for review March 11, 2025 15:44
@hendrikmakait hendrikmakait requested a review from fjetter as a code owner March 11, 2025 15:44
Copy link
Contributor

Unit Test Results

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

    27 files  ± 0      27 suites  ±0   11h 37m 34s ⏱️ + 15m 0s
 4 100 tests  -  1   3 988 ✅ + 76    111 💤 ±0  1 ❌  -  76 
51 404 runs   - 14  49 107 ✅ +799  2 296 💤  - 1  1 ❌  - 811 

For more details on these failures, see this check.

Results for commit 755b9ea. ± Comparison against base commit ad6f7d9.

This pull request removes 1 test.
distributed.tests.test_client ‑ test_get_scheduler_default_client_config_interleaving

Copy link
Member

@jacobtomlinson jacobtomlinson left a comment

Choose a reason for hiding this comment

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

Looks good thanks for resolving this @hendrikmakait.

CI failures appear unrelated.

@jacobtomlinson jacobtomlinson merged commit 7df4356 into dask:main Mar 12, 2025
30 of 33 checks passed
@hendrikmakait hendrikmakait deleted the fix-ci branch March 12, 2025 09:57
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