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

Add PipInstall WorkerPlugin #3216

Merged
merged 8 commits into from
Oct 9, 2020
Merged

Conversation

mrocklin
Copy link
Member

@mrocklin mrocklin commented Nov 9, 2019

Examples

>>> from dask.distributed import PipInstall
>>> plugin = PipInstall(packages=["dask", "scikit-learn"])
>>> client.register_worker_plugin(plugin)

Examples
--------

```python
>>> from dask.distributed import PipInstall
>>> plugin = PipInstall(packages=["dask", "scikit-learn"])
>>> client.register_worker_plugin(plugin)
```
@mrocklin
Copy link
Member Author

mrocklin commented Nov 9, 2019

cc @TomAugspurger @rabernat @jhamman

distributed/diagnostics/plugin.py Show resolved Hide resolved
distributed/diagnostics/plugin.py Show resolved Hide resolved
distributed/diagnostics/plugin.py Show resolved Hide resolved

name = "pip"

def __init__(self, packages=[], restart=False):
Copy link
Member

Choose a reason for hiding this comment

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

Possibly would want to accept an argument for options passed to pip. Things like --upgrade, --pre, etc.

So perhaps a pip_options=None that's a list of strings, just like in Popen.

* Add doc warning / note
* Mock the tests
* Added pip_options
@TomAugspurger
Copy link
Member

Pushed a few changes here.

@jcrist do you have any objections to the overall idea of doing this in a worker plugin?

@mrocklin
Copy link
Member Author

mrocklin commented Nov 19, 2019 via email

@kyprifog
Copy link

Whats the status of this effort? I just ran into an issue running dask-gateway with not having s3fs on the workers and I'm trying to find the path of least resistence.

@TomAugspurger
Copy link
Member

I pushed a couple updates to

  • fix merge conflicts
  • add a test for an install failures (e.g. package typo)
  • added the PipInstall plugin to the docs

I think this should be good to go.

@mrocklin
Copy link
Member Author

mrocklin commented Oct 1, 2020

I think that there are likely still issues when you have multiple workers on the same machine pip/conda installing things. However, this may be an uncommon enough case where this is likely to be used that we shouldn't care. I'm not sure. I think that it's totally ok to merge anyway.

@kyprifog
Copy link

kyprifog commented Oct 1, 2020

@mrocklin, to clarify are you saying there are issues with multiple clients trying to issue (possibly contradictory or overlapping) commands to the workers of the same cluster to update packages?

@TomAugspurger I am trying to test this (rudimentarily) by doing this. Am I doing it wrong? I am still getting a package not found s3fs error from the workers:

with Client() as client:
    plugin = PipInstall(packages=["s3fs"], pip_options=["--upgrade"], restart=True)
    client.register_worker_plugin(plugin)
    df = dd.read_csv(files)
    df.count().compute()

>>
dask-worker-4c0dd1f1d56b4050ba4fc4e1e89edd44-vb7cg dask-worker ModuleNotFoundError: No module named 's3fs'

@mrocklin
Copy link
Member Author

mrocklin commented Oct 1, 2020 via email

@TomAugspurger
Copy link
Member

I am still getting a package not found s3fs error from the workers:

I wonder if the installation / restart fails to complete before you start your computation requiring s3fs?

The issue with multiple workers sharing a filesystem is a good one I hadn't considered. I'm OK with just documenting that shortcoming for now.

[ci skip]
@kyprifog
Copy link

kyprifog commented Oct 2, 2020

I am still getting a package not found s3fs error from the workers:

I wonder if the installation / restart fails to complete before you start your computation requiring s3fs?

The issue with multiple workers sharing a filesystem is a good one I hadn't considered. I'm OK with just documenting that shortcoming for now.

I had wondered the same thing but if that were the case running the same operation twice would result in a positive response. That being said, it didn't appear that the workers restarted in kubernetes so maybe I am doing something else wrong. I think maybe there is also a chance something weird with conda is happening, the command that finally worked when run on each worker was:

conda install --yes -c conda-forge s3fs==0.4.2

So i had to use conda and downgrade s3fs (I was getting an error form fsspec about "other_paths" with 0.5.0 and 0.5.1)

For context, I'm trying to avoid forking the helm chart for daskhub which is why I'm spending more time trying to figure out how to do this via plugin.

Its possible the issue is actually related to conda environments.

@kyprifog
Copy link

kyprifog commented Oct 2, 2020

Update: I was able to get my issue resolved by building my own conda install worker plugin.

@TomAugspurger
Copy link
Member

That being said, it didn't appear that the workers restarted in kubernetes

What do you mean by this? I don't believe this would be visible at the kubernetes level (other than via logs), since no pods / processes are going away.

@kyprifog
Copy link

kyprifog commented Oct 6, 2020

Yeah I meant via logs, I was running stern dask and non of the workers logged any changes. On my own plugin, I just ended up doing it synchronously and not restarting the workers bc I couldn't figure out what was going on and it seemed to work. 🤷 Maybe it was actually working, dask-gateway was just expecting conda install, idk.

@TomAugspurger
Copy link
Member

Gotcha, thanks for clarifying.

@TomAugspurger
Copy link
Member

I'm planning to merge this later today.

@mrocklin
Copy link
Member Author

mrocklin commented Oct 9, 2020 via email

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.

3 participants