-
-
Notifications
You must be signed in to change notification settings - Fork 720
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
Ensure shuffle split default durations uses proper prefix #4991
Conversation
default_time = parse_timedelta( | ||
dask.config.get("distributed.scheduler.default-task-durations")[split_prefix] | ||
) | ||
assert default_time <= 1e-6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot assert on the actual known runtime since at this point it is/was measured already. I want to ensure that the actual config value is correct. There are other tests ensuring that these config values are used if they exist
@@ -1845,6 +1845,37 @@ async def test_get_task_duration(c, s, a, b): | |||
assert len(s.unknown_durations["slowinc"]) == 1 | |||
|
|||
|
|||
@gen_cluster(client=True) | |||
async def test_default_task_duration_splits(c, s, a, b): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm this test seems to raise TimeoutError
s in a few CI builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure this is connected to improper cluster shutdowns I've been observing recently. I am using almost identical code for the stealing test with the exception that I'm not awaiting the computation here. I'll add a wait in here and hope this is gone afterwards
ff03de9
to
00a4226
Compare
I had one green-ish after the last commit. I retriggered again to see if the test causes trouble. for reference, the one failure I had before was |
One CI failure. Maybe ok? |
This CI failure is #4859 I would declare the test as "stable enough" after two reruns and go ahead with merging this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fjetter!
The test is bit heavy considering that I simply want to verify that the prefix name of shuffle doesn't go unnoticed. but it works and still runs fast so I'm fine with it 😬
black distributed
/flake8 distributed
/isort distributed