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

Make CI happy again #8560

Merged
merged 47 commits into from
Mar 8, 2024
Merged

Make CI happy again #8560

merged 47 commits into from
Mar 8, 2024

Conversation

milesgranger
Copy link
Contributor

@milesgranger milesgranger commented Mar 7, 2024

xref: dask/dask-expr#945

Tried to split each 'fix' into it's own commit, I think some may need their own follow-up issue.

Copy link
Contributor

github-actions bot commented Mar 7, 2024

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   10h 26m 24s ⏱️ + 1h 48m 13s
 4 048 tests  -     2   3 932 ✅ +  109    110 💤  -   3   6 ❌  - 108 
50 828 runs  +5 654  48 459 ✅ +6 274  2 336 💤 +180  33 ❌  - 800 

For more details on these failures, see this check.

Results for commit 1ccfad6. ± Comparison against base commit f595c9e.

This pull request removes 2 tests.
distributed.cli.tests.test_dask_worker.test_listen_address_ipv6[tcp:..[ ‑ 1]:---nanny]
distributed.cli.tests.test_dask_worker.test_listen_address_ipv6[tcp:..[ ‑ 1]:---no-nanny]

♻️ This comment has been updated with latest results.

@milesgranger milesgranger marked this pull request as ready for review March 8, 2024 11:16
Comment on lines 2772 to 2773
if dd._dask_expr_enabled():
pytest.skip("No split task in dask-expr")
Copy link
Collaborator

@crusaderky crusaderky Mar 8, 2024

Choose a reason for hiding this comment

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

@fjetter @phofl can I have a secondo opinion on this? Not sure I understand why we don't have this task in dask-expr. Are we missing a line in distributed.scheduler.default-task-durations?

Copy link
Collaborator

Choose a reason for hiding this comment

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

There should be a split_task, e.g. the materialisation happens in TaskShuffle in _layer, but I have never look into how this is actually materialized

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same discussion for test_blocklist_shuffle_split

Copy link
Collaborator

Choose a reason for hiding this comment

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

@hendrikmakait and I stumbled upon the root cause, dask-expr doesn't properly respect the context manager. I am preparing a fix at the moment, should be there in a minute

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

pr is merged

Copy link
Collaborator

Choose a reason for hiding this comment

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

@phofl I'm now seeing split-taskshuffle and split-stage. Are they both supposed to have 1us default-duration?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah the naming is confusing and something we probably should fix, but it's ok conceptually

@@ -1011,6 +1011,7 @@ async def test_balance_with_longer_task(c, s, a, b):
assert z.key in b.data


@pytest.mark.xfail(reason="blocked is empty", raises=AssertionError)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Potential blocker.
Discussion on test_scheduler.py::test_default_task_duration_splits above.

@crusaderky
Copy link
Collaborator

crusaderky commented Mar 8, 2024

I'm aiming to merge this PR with these tests still failing, as they are more involved:

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, everyone! I agree with @crusaderky that we should go ahead and merge if nothing else pops up on CI.

@crusaderky crusaderky mentioned this pull request Mar 8, 2024
4 tasks
@phofl phofl merged commit e16a7af into dask:main Mar 8, 2024
12 of 35 checks passed
@milesgranger milesgranger deleted the milesgranger/fix-ci branch March 8, 2024 19:08
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.

5 participants