Skip to content

Conversation

@jrbourbeau
Copy link
Member

Currently the default name is "pip" which results in

client.register_worker_plugin(PipInstall(packages=["foo"]))
client.register_worker_plugin(PipInstall(packages=["bar"]))

not registering the second plugin. This PR ensures a unique name is generated for PipInstall plugins which are installing different packages

@fjetter
Copy link
Member

fjetter commented Jul 28, 2021

@jrbourbeau sorry I messed up the merge commit. I pushed another one to fix it

@mrocklin
Copy link
Member

I sent in a comment by e-mail yesterday and just got a bounce message. That comment is below:

My experience with this is that often I realize that I wanted to upload a different set of packages (maybe adding one more or changing a version, for example). What I really want in these cases is to replace the previous plugin, not add another one. In this situation I think that we want to keep the uniform name, but use the semantics in the NannyPlugin around repeats. Otherwise I could imagine being surprised that I have ten different pip installs happening each time :)

@jrbourbeau
Copy link
Member Author

In this situation I think that we want to keep the uniform name, but use the semantics in the NannyPlugin around repeats.

I think this difference between how worker and nanny plugins behave will end up causing confusion. I'm curious why worker plugins have the repeat behave they do to begin with. Maybe we can update how worker plugins behave and always overwrite worker plugins with the same name. FWIW we're adding names to scheduler plugins (xref #5120) and I think there would be value in all our plugin systems having consistent behavior around repeats

Also, I just noticed that we always overwrite the worker plugin stored on the scheduler

self.worker_plugins[name] = plugin

which means that new workers which arrive can end up with a different set of worker plugins than older workers

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