Skip to content

Tokenization-related test tweaks (backport from #8185)#8499

Merged
hendrikmakait merged 1 commit intodask:mainfrom
crusaderky:tests_cosmetic
Feb 15, 2024
Merged

Tokenization-related test tweaks (backport from #8185)#8499
hendrikmakait merged 1 commit intodask:mainfrom
crusaderky:tests_cosmetic

Conversation

@crusaderky
Copy link
Collaborator

Backport from #8185, which may not happen in the short term future.
This unblocks dask/dask#10883.

@crusaderky crusaderky requested a review from fjetter as a code owner February 6, 2024 17:45
second.wait()

fs = c.map(func, [first] * 5, [second] * 5)
fs = c.map(func, [first] * 5, [second] * 5, key=[f"x{i}" for i in range(5)])
Copy link
Collaborator Author

@crusaderky crusaderky Feb 6, 2024

Choose a reason for hiding this comment

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

With dask/dask#10883, func no longer tokenizes to 5 different random values, so we ended up with a single key.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2024

Unit Test Results

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

    27 files  +    1      27 suites  +1   10h 15m 49s ⏱️ + 40m 16s
 3 971 tests ±    0   3 860 ✅  -     1    109 💤 ± 0  2 ❌ + 1 
49 932 runs  +2 548  47 637 ✅ +2 495  2 293 💤 +64  2 ❌  - 11 

For more details on these failures, see this check.

Results for commit e9fd17a. ± Comparison against base commit 9a9468c.

♻️ This comment has been updated with latest results.


@gen_cluster(client=True, nthreads=[("127.0.0.1", 1), ("127.0.0.1", 2)])
async def test_dont_steal_worker_restrictions(c, s, a, b):
future = c.submit(slowinc, 1, delay=0.10, workers=a.address)
Copy link
Member

Choose a reason for hiding this comment

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

It looks like this was used to seed the task duration. I think that by removing this, this test will be vulnerable to changes to the default task duration and could potentially fail to raise.

Copy link
Member

Choose a reason for hiding this comment

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

I see that we assert that (almost) the same scenario does indeed balance in test_work_steal_no_kwargs. Maybe it makes sense to adjust the delay there and add a comment that references that this tests a precondition of the others?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I expanded on the cleanup on test_steal and, since it's unrelated to tokenization, moved it to a separate PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

@hendrikmakait hendrikmakait left a comment

Choose a reason for hiding this comment

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

Thanks, @crusaderky!

@hendrikmakait hendrikmakait merged commit bbe578f into dask:main Feb 15, 2024
@crusaderky crusaderky deleted the tests_cosmetic branch February 15, 2024 13:06
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