Refactor `PackageInstall` into a generic `InstallPlugin` by hendrikmakait · Pull Request #8343 · dask/distributed

hendrikmakait · 2023-11-14T16:58:00Z

~~Blocked by #8342~~

Refactors PackageInstall into a generic InstallPlugin that takes ~~an Installer instance~~ a Callable as input; also introduces a NannyPlugin that is used if workers should be restarted.

This could potentially be refactored into a generic RunPlugin that executes a callable on the cluster. For this, we should probably make the locking mechanism configurable. Right now, we use a host-based lock, I can imagine that a cluster-wide one and lockless mode would also be useful.

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

github-actions · 2023-11-14T18:25:47Z

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 13h 6m 37s ⏱️ + 48m 39s
  3 932 tests -   2   3 806 ✔️ - 10   117 💤 ±0   9 ❌ +  8
49 462 runs - 26 47 037 ✔️ - 37 2 413 💤 +1 12 ❌ +10

For more details on these failures, see this check.

Results for commit ebb4988. ± Comparison against base commit eeec6ce.

This pull request removes 10 and adds 8 tests. Note that renamed tests count towards both.

distributed.diagnostics.tests.test_packageinstall ‑ test_conda_install
distributed.diagnostics.tests.test_packageinstall ‑ test_conda_install_fails_on_returncode
distributed.diagnostics.tests.test_packageinstall ‑ test_conda_install_fails_when_conda_not_found
distributed.diagnostics.tests.test_packageinstall ‑ test_conda_install_fails_when_conda_raises
distributed.diagnostics.tests.test_packageinstall ‑ test_package_install_failing_does_not_restart_on_nanny
distributed.diagnostics.tests.test_packageinstall ‑ test_package_install_installs_once_when_reregistered
distributed.diagnostics.tests.test_packageinstall ‑ test_package_install_installs_once_with_multiple_workers
distributed.diagnostics.tests.test_packageinstall ‑ test_package_install_restarts_on_nanny
distributed.diagnostics.tests.test_packageinstall ‑ test_pip_install
distributed.diagnostics.tests.test_packageinstall ‑ test_pip_install_fails

distributed.diagnostics.tests.test_install_plugin ‑ test_conda_install
distributed.diagnostics.tests.test_install_plugin ‑ test_conda_install_fails_on_returncode
distributed.diagnostics.tests.test_install_plugin ‑ test_conda_install_fails_when_conda_not_found
distributed.diagnostics.tests.test_install_plugin ‑ test_conda_install_fails_when_conda_raises
distributed.diagnostics.tests.test_install_plugin ‑ test_package_install_failing_does_not_restart_on_nanny
distributed.diagnostics.tests.test_install_plugin ‑ test_package_install_restarts_on_nanny
distributed.diagnostics.tests.test_install_plugin ‑ test_pip_install
distributed.diagnostics.tests.test_install_plugin ‑ test_pip_install_fails

♻️ This comment has been updated with latest results.

hendrikmakait · 2023-11-15T06:11:58Z

                loop=worker.loop,
            )
        ):
-            if not await self._is_installed(worker):


I have dropped all of these elaborate checks in favor of the requirement for install_fn() to be idempotent. This makes the plugin a lot simpler and this logic would still not have caught some issues like flaky installs, etc.

hendrikmakait · 2023-11-15T07:27:19Z

    PipInstall
    """

+    idempotent = True


hendrikmakait · 2023-11-15T08:20:09Z

    Environ,
+    InstallPlugin,
    NannyPlugin,
-    PackageInstall,


We may want to consider a deprecation cycle here, but PackageInstall was never truly public since _PackageInstaller was never public.

I think that's fine

hendrikmakait · 2023-11-15T09:55:45Z

Update: I've refactored this so that InstallPlugin takes a Callable as input instead of the customer Installer.

fjetter · 2023-11-15T13:10:42Z

+            await Semaphore(
+                max_leases=1,
+                name=socket.gethostname(),
+                register=True,
+                scheduler_rpc=worker.scheduler,
+                loop=worker.loop,
+            )


unrelated to the change in this PR but reviewing this again I wonder if a lock file wouldn't be much simpler.
We're typically using locket for this, e.g.

distributed/distributed/diskutils.py

Line 161 in e98dcb1

return locket.lock_file(self._global_lock_path, **kwargs)

Good point, I hadn't thought of this. This would also solve the issue that using a Semaphore from the scheduler isn't straightforward (if not even impossible).

I'd leave this to a follow-up PR though.

fjetter

high level, LGTM

One functional question aobut the semaphore but this is unrelated and could be broken out in a follow up

fjetter · 2023-11-17T11:26:31Z

    Environ,
+    InstallPlugin,
    NannyPlugin,
-    PackageInstall,


I think that's fine

hendrikmakait added 2 commits November 14, 2023 17:56

Use nanny plugin if workers should be restarted

04ae012

Adjust tests and re-add logging

1dce52b

Refactor to generic InstallPlugin

f095aa6

hendrikmakait changed the title ~~Use NannyPlugin when restarting workers for PackageInstall plugin~~ Refactor PackageInstall into a generic InstallPlugin Nov 15, 2023

Moved tests

7ff7f99

hendrikmakait changed the title ~~Refactor PackageInstall into a generic InstallPlugin~~ Refactor PackageInstall into a generic SetupPlugin Nov 15, 2023

hendrikmakait changed the title ~~Refactor PackageInstall into a generic SetupPlugin~~ Refactor PackageInstall into a generic InstallPlugin Nov 15, 2023

hendrikmakait added 2 commits November 15, 2023 08:26

Idempotency

0b05290

Callable

6517f52

hendrikmakait commented Nov 15, 2023

View reviewed changes

Comment

b8c7ac1

fjetter reviewed Nov 15, 2023

View reviewed changes

Merge branch 'main' into packageinstall-restarts-from-nannyplugin

cda8917

hendrikmakait marked this pull request as ready for review November 15, 2023 15:05

hendrikmakait requested a review from jacobtomlinson as a code owner November 15, 2023 15:05

Adjust after merge

ebb4988

hendrikmakait requested a review from fjetter November 16, 2023 13:19

hendrikmakait mentioned this pull request Nov 16, 2023

Support environment variables in PipInstall plugin #8357

Merged

2 tasks

fjetter approved these changes Nov 17, 2023

View reviewed changes

Comment thread distributed/__init__.py

Environ,

InstallPlugin,

NannyPlugin,

PackageInstall,

fjetter Nov 17, 2023

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

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

I think that's fine

hendrikmakait merged commit 412a0a9 into dask:main Nov 17, 2023

hendrikmakait deleted the packageinstall-restarts-from-nannyplugin branch November 17, 2023 11:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Refactor `PackageInstall` into a generic `InstallPlugin`#8343

Refactor `PackageInstall` into a generic `InstallPlugin`#8343
hendrikmakait merged 9 commits into
dask:mainfrom
hendrikmakait:packageinstall-restarts-from-nannyplugin

hendrikmakait commented Nov 14, 2023 •

edited

Loading

Uh oh!

github-actions Bot commented Nov 14, 2023 •

edited

Loading

Uh oh!

hendrikmakait Nov 15, 2023 •

edited

Loading

Uh oh!

hendrikmakait Nov 15, 2023

Uh oh!

hendrikmakait Nov 15, 2023

Uh oh!

fjetter Nov 17, 2023

Uh oh!

hendrikmakait commented Nov 15, 2023

Uh oh!

fjetter Nov 15, 2023

Uh oh!

hendrikmakait Nov 15, 2023

Uh oh!

hendrikmakait Nov 15, 2023

Uh oh!

fjetter left a comment

Uh oh!

fjetter Nov 17, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

hendrikmakait commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

hendrikmakait Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hendrikmakait Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

hendrikmakait Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

fjetter Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

hendrikmakait commented Nov 15, 2023

Uh oh!

fjetter Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

hendrikmakait Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

hendrikmakait Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

fjetter left a comment

Choose a reason for hiding this comment

Uh oh!

fjetter Nov 17, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hendrikmakait commented Nov 14, 2023 •

edited

Loading

github-actions Bot commented Nov 14, 2023 •

edited

Loading

hendrikmakait Nov 15, 2023 •

edited

Loading