Add tests for choosing default rechunking method #8843
Conversation
Unit Test ResultsSee test report for an extended history of previous test failures. This is useful for diagnosing flaky tests. 25 files ± 0 25 suites ±0 10h 19m 18s ⏱️ + 7m 24s For more details on these failures, see this check. Results for commit 88bf685. ± Comparison against base commit ea7d35c. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Thanks @hendrikmakait. Just a couple of minor comments but overall LGTM
| ) | ||
|
|
||
| assert x2.chunks == new | ||
| assert np.all(await c.compute(x2) == a) |
There was a problem hiding this comment.
Why not use da.assert_eq here instead? I assume there's some reason -- just curious
There was a problem hiding this comment.
Ah, probably something to do with the client being async
| ], | ||
| ) | ||
| @gen_cluster(client=True) | ||
| async def test_rechunk_heuristic(c, s, a, b, new, expected_algorithm): |
There was a problem hiding this comment.
I'm a little surprised we're testing this here instead of over in dask/dask, where the definition of the heuristic lives. I don't mean this to be a blocking comment -- I'm glad there's test coverage for this somewhere : )
I'm a little out of date on how we're choosing where rechunking-related tests go. Why did you put this test here instead of in dask/dask#11337?
There was a problem hiding this comment.
All existing tests (that I'm aware of) around rechunk configuration live in test_rechunk.py. I don't think there's a perfect place to put these as these two components are generally intertwined and P2P requires a distributed cluster. I could see two things happening:
- Move the logic around choosing between P2P and tasks on a distributed cluster over into
distributed. This way, it lives in the same repo as the tests. - Add a test to
daskthat simply tests that we usetasks-based rechunking when not using a distributed cluster.
There was a problem hiding this comment.
Ah, I see. Agreed that this is sort of a gray area where there's not a clear and obvious place for these to live. What you have here is fine and we can always move things around later if we want.
|
thx |
Sibling to dask/dask#11337
pre-commit run --all-files