Add minimum dependency testing to CI by charlesbluca · Pull Request #7285 · dask/distributed

charlesbluca · 2022-11-10T05:24:14Z

Continuing @jrbourbeau's work in #4794, this PR adds a new check that runs the test suite against the minimum versions of Distributed's dependencies, which should help us to identify if/when deps need to be updated.

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

…ps-ci

charlesbluca · 2022-11-22T18:19:53Z

distributed/system_monitor.py

            except Exception:
-                # FIXME is this possible?
+                # FIXME occurs when psutil version doesn't have handling for given platform / kernel;
+                # should we explicitly error in this case?
                monitor_disk_io = False  # pragma: nocover


Digging into the failures on distributed/tests/test_system_monitor.py::test_disk_config, it looks like this exception gets raised when psutil doesn't support the given platform/kernel (in our case, 5.6.3 didn't have support for Linux 5.5+).

Given this knowledge, would it make sense to raise a warning here to let the user know that I/O monitoring will be disabled because of this?

Nice detective work! I could definitely see how emitting a warning here could help avoid user confusion. Normally, I'd defer to you as to if you'd like to include that new warning/test here or in a separate PR. But given the diff here is already pretty big, and adding a new warning seems more like an (reasonable) enhancement rather than directly related to the mindeps CI build, how about we tackle it in a separate PR?

fjetter · 2022-11-24T09:47:37Z

I took the liberty of fixing merge conflicts in requirements.txt after merging #7286

@charlesbluca is there anything else left to do?

charlesbluca · 2022-11-30T17:24:18Z

Thanks @fjetter!

is there anything else left to do?

Yeah, there are two remaining tests that seem to be failing consistently with mindeps that I haven't been able to debug:

distributed/deploy/tests/test_spec_cluster.py::test_adaptive_killed_worker
distributed/deploy/tests/test_spec_cluster.py::test_broken_worker

Beyond these failures, it would probably be worthwhile to dig through the files that pytest is currently skipping to see if there are any tests that can be run with minimal dependencies to increase the testing coverage, but considering the diff here is pretty large I'm okay with pushing that off to a follow-up PR.

fjetter · 2022-12-01T10:00:51Z

Yeah, there are two remaining tests that seem to be failing consistently with mindeps that I haven't been able to debug:

I think both test cases have the same root cause. This reminds me of failures I encountered in #7323 I suggest to merge that one first (still WIP) and then see if the issue persists. I don't think this is necessarily related to the mindeps but maybe the dependencies introduced different timings making the race conditions I'm fixing over there more likely

Beyond these failures, it would probably be worthwhile to dig through the files that pytest is currently skipping to see if there are any tests that can be run with minimal dependencies to increase the testing coverage, but considering the diff here is pretty large I'm okay with pushing that off to a follow-up PR.

Yes, that's definitely out of scope

…orker' into mindeps-testing

charlesbluca · 2022-12-05T16:16:18Z

Looks like the current changes in #7323 resolves the failures 🎉 once that's finished and merged, we can revisit this PR to see if there are any remaining tests to resolve

jrbourbeau

Thanks for all your work here @charlesbluca. I'm inclined to temporarily skip/xfail those failing tests in order to get this PR in. We can then un-skip/un-xfail once #7323 is in.

Here's another issue where a mindeps build would have been useful #7416

…ps-testing

…t_kill_worker' into mindeps-testing" This reverts commit 43d4017, reversing changes made to 8e59a2c.

mrocklin · 2023-01-03T16:16:33Z

It looks like this has been going on for a while. @charlesbluca thank you for continuing to track this. Is there anything we can do to get some variant of this in quickly so that you don't have to work to keep this from going stale?

charlesbluca · 2023-01-03T16:25:16Z

Is there anything we can do to get some variant of this in quickly so that you don't have to work to keep this from going stale?

Was going to go with @jrbourbeau's suggestion and just xfail the few remaining tests that should be resolved with #7323

charlesbluca · 2023-01-03T19:35:37Z

distributed/tests/test_nanny.py

+@pytest.mark.xfail(
+    os.environ.get("MINDEPS") == "true",
+    reason="Timeout errors with mindeps environment",
+)


Since it wasn't immediately obvious which package(s) were causing this remaining failure, I opted to xfail when matrix.environment == 'mindeps' in the GHA run; this means that to run mindeps tests locally, one would need to prepend the pytest command with MINDEPS=true.

fjetter · 2023-01-04T12:57:24Z

#7323 might not get finished after all. I will have another final look at it but it proved to be incredibly difficult to converge and a fix might require some significant refactoring that will be out of scope for that PR

If a single/few tests are failing on this (that are not failing elsewhere) we should probably just skip them for the mindep build. #7323 should not block this

charlesbluca · 2023-01-04T18:33:55Z

we should probably just skip them for the mindep build

Yeah it looks like the one remaining failure here is distributed/tests/test_nanny.py::test_nanny_timeout, which I've gone ahead and xfailed when we're running mindeps testing. The remaining failures are unrelated, so this should be ready for review

fjetter

Great work. Thank you @charlesbluca

jrbourbeau

Woo! It's great to see this included -- thanks for all your work here @charlesbluca!

jrbourbeau and others added 30 commits May 3, 2021 13:17

Add minimum dependencies CI build

51ef208

Fix conda solve errors

ef68243

Update minimum psutil

6eec67f

Specify Python version in workflow file

88f3c53

Bump click

e871698

Add testing deps

6d486e0

Make sure distributed is installed

a02538a

Update distributed/comm/tests/test_ws.py

06cbef5

Make sure DISABLE_IPV6 is set properly

c50f54c

Bump minimum Dask version in CI

761c419

More test_ws.py fixup

e6c08a3

Bump minimum version of toolz to avoid related cytoolz seg fault

723f549

Update minimum deps in requirements.txt

130d512

Add fix for test_pickle_empty

3a728ab

Merge branch 'main' of https://github.com/dask/distributed into minde…

4739e58

…ps-ci

Latest dask

5be6f20

Add pip

3b4ff44

Fixup

cd76c21

Merge branch 'main' of https://github.com/dask/distributed into minde…

27d561d

…ps-ci

Add jinja2

1962ddd

Merge branch 'main' of https://github.com/dask/distributed into minde…

f3980ba

…ps-ci

msgpack < 1 compatibility

a18f33d

Possible ssl workaround

56184ab

Add note about openssl version constraint

b3fa6cf

Merge branch 'main' of https://github.com/dask/distributed into minde…

fabd6da

…ps-ci

Merge branch 'main' of https://github.com/dask/distributed into minde…

44fd418

…ps-ci

Review feedback

bf04930

Add pytest-cov to mindeps environment file

6c549d6

Update mindeps file

d9c3929

Merge remote-tracking branch 'origin/main' into mindeps-ci

c03635c

charlesbluca commented Nov 22, 2022

View reviewed changes

Merge branch 'main' into mindeps-testing

8efcb73

fjetter and others added 4 commits December 1, 2022 13:51

Guarantee worker is restarted if Nanny.kill is called

87521ce

WorkerProcess blocks on kill if still starting

85562a0

Merge remote-tracking branch 'origin/main' into mindeps-testing

8e59a2c

Merge remote-tracking branch 'fjetter/ensure_nanny_restart_not_kill_w…

43d4017

…orker' into mindeps-testing

jrbourbeau reviewed Dec 19, 2022

View reviewed changes

jrbourbeau and others added 2 commits December 19, 2022 15:22

Merge branch 'main' of https://github.com/dask/distributed into minde…

509d0f5

…ps-testing

Revert "Merge remote-tracking branch 'fjetter/ensure_nanny_restart_no…

5b2e5f6

…t_kill_worker' into mindeps-testing" This reverts commit 43d4017, reversing changes made to 8e59a2c.

Merge remote-tracking branch 'origin/main' into mindeps-testing

1fe68e1

charlesbluca added 2 commits January 3, 2023 10:15

xfail remaining failing test

fb7adf3

Correct workflow syntax

e279197

charlesbluca commented Jan 3, 2023

View reviewed changes

fjetter approved these changes Jan 5, 2023

View reviewed changes

fjetter merged commit 3619923 into dask:main Jan 5, 2023

crusaderky mentioned this pull request Jan 6, 2023

Drop support for zict <2.1.0 #7456

Merged

jrbourbeau reviewed Jan 9, 2023

View reviewed changes

jrbourbeau mentioned this pull request Jan 9, 2023

Update dependencies after mindeps CI build updates conda-forge/distributed-feedstock#238

Closed

This was referenced Feb 24, 2023

Release 2023.2.1 conda-forge/distributed-feedstock#242

Merged

Release 2023.2.1 conda-forge/dask-feedstock#215

Merged

Release 2023.2.1 dask/community#308

Closed

Add minimum version to optional jinja2 dependency dask/dask#9999

Merged

Uh oh!

Conversation

charlesbluca commented Nov 10, 2022

Uh oh!

charlesbluca Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

jrbourbeau Nov 22, 2022

Choose a reason for hiding this comment

Uh oh!

fjetter commented Nov 24, 2022

Uh oh!

charlesbluca commented Nov 30, 2022

Uh oh!

fjetter commented Dec 1, 2022

Uh oh!

charlesbluca commented Dec 5, 2022

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

mrocklin commented Jan 3, 2023

Uh oh!

charlesbluca commented Jan 3, 2023

Uh oh!

charlesbluca Jan 3, 2023

Choose a reason for hiding this comment

Uh oh!

fjetter commented Jan 4, 2023

Uh oh!

charlesbluca commented Jan 4, 2023

Uh oh!

fjetter left a comment

Choose a reason for hiding this comment

Uh oh!

jrbourbeau left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants