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

Handle dask-worker command args and `extra` kwargs as a list #160

Merged
merged 8 commits into from Sep 25, 2018

Conversation

Projects
None yet
2 participants
@guillaumeeb
Copy link
Member

guillaumeeb commented Sep 24, 2018

Fixes #156

@willirath

This comment has been minimized.

Copy link
Contributor

willirath commented Sep 24, 2018

Just a comment that is beyond the scope of this PR: Keeping track of all the CLI args to dask-worker in a list_of_all_args (see the recommended way of passing args to subprocess) and eventually do " ".join(list_of_all_args) would be a general way of solving this.

@guillaumeeb

This comment has been minimized.

Copy link
Member Author

guillaumeeb commented Sep 24, 2018

@willirath this would probably be cleaner yes, but should we change the extra constructor kwargs to be also a list then?

@willirath

This comment has been minimized.

Copy link
Contributor

willirath commented Sep 24, 2018

but should we change the extra constructor kwargs to be also a list then?

My personal feeling says: Yes, passing CLI args as a sequence is standard any way. But I'm not sure if that's representative.

guillaumeeb added some commits Sep 24, 2018

@guillaumeeb

This comment has been minimized.

Copy link
Member Author

guillaumeeb commented Sep 24, 2018

Okay, so I chose to follow your suggestion @willirath.

@guillaumeeb guillaumeeb changed the title Add a default space before extra options in dask command line Handle dask-worker command args and `extra` kwargs as a list Sep 24, 2018

guillaumeeb added some commits Sep 24, 2018

@guillaumeeb

This comment has been minimized.

Copy link
Member Author

guillaumeeb commented Sep 25, 2018

Travis was also having a difficult monday apprently.

@guillaumeeb

This comment has been minimized.

Copy link
Member Author

guillaumeeb commented Sep 25, 2018

@willirath, since you've made the comment about using a list of args, a review would be welcome!

@willirath
Copy link
Contributor

willirath left a comment

I think, the default config also has to be adapted:

@willirath
Copy link
Contributor

willirath left a comment

And the interface is stil set as a string:

extra += ' --interface %s ' % interface

This should be

extra += [' --interface', interface]

Edit: Typo in second list item.

@guillaumeeb

This comment has been minimized.

Copy link
Member Author

guillaumeeb commented Sep 25, 2018

Few, thanks for that @willirath, no real testing from my part before your comments... Glad you saw those mistakes.

@willirath
Copy link
Contributor

willirath left a comment

Looks good!

@guillaumeeb

This comment has been minimized.

Copy link
Member Author

guillaumeeb commented Sep 25, 2018

Thanks @willirath, feel free to take a look at other PR/issues if you find the time. It could be nice to have another core maintainer.

@guillaumeeb guillaumeeb merged commit 4ec41e5 into dask:master Sep 25, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@willirath

This comment has been minimized.

Copy link
Contributor

willirath commented Sep 27, 2018

Thanks @willirath, feel free to take a look at other PR/issues if you find the time. It could be nice to have another core maintainer.

Sounds good. I'm not sure I'll have much time for continuous work. But it sure looks like dask clusters and dask-jobqueue will play a increasingly role in my and my group's routine work. So count me in!

@guillaumeeb

This comment has been minimized.

Copy link
Member Author

guillaumeeb commented Sep 27, 2018

Nice !

We're all on the same situation about time 🙂 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment