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 scheduler_options to pass scheduler-specific parameters #384

Merged
merged 14 commits into from
Mar 23, 2020

Conversation

lesteve
Copy link
Member

@lesteve lesteve commented Mar 4, 2020

fix #355 (regression in 0.7 where scheduler port was ignored), fix #382 (different interfaces on scheduler and workers).

@lesteve lesteve changed the title Add scheduler options Add scheduler options to pass scheduler interface and scheduler port Mar 4, 2020
@lesteve lesteve changed the title Add scheduler options to pass scheduler interface and scheduler port Add scheduler options to pass scheduler interface and scheduler port (amongst other things) Mar 4, 2020
@@ -447,22 +453,47 @@ def __init__(
)
)

if dashboard_address is not None:
Copy link
Member Author

@lesteve lesteve Mar 4, 2020

Choose a reason for hiding this comment

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

Not allowing to pass host and dashboard_address is a breaking change. I guess it feels cleaner to be able to pass all the scheduler options through scheduler_options.

Also if we keep host for example, you can pass different things at different levels, e.g. host + interface in scheduler_options which would make the error hard to make user-friendly (and/or the code a bit ugly).

If you think that's too harsh, I can make it a warning until the 0.8 release!

Copy link
Member

Choose a reason for hiding this comment

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

What feel's weird here is to keep the host and dashboard_address in the constructor kwargs, but thrown an exception when used. Maybe we should add some doc or comment, or simply remove them and catch them through **kwargs argument? Or else we should handle it and log a deprecation warning.

I'm not really familliar with Python best practices in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can see two solutions:

  1. raise an informative error if people are using host or dasboard_address to tell them to pass this through scheduler_options. What I had in mind was to have this user-friendly error in 0.7.1 and remove host and dashboard_address in 0.8.
  2. warn if people are using host or dashboard_address telling them that this will not be supported in 0.8 and then combine them with scheduler_options to do the right thing.

I tend to think 1. is easier because you don't need the combining logic between host dashboard_address and scheduler_options which can be tricky. For example, if you use host and use scheduler_options={'interface': eth0} which one should have the priority and should we raise an error in this case.

Also my feeling is that people ignore warnings until it breaks so that 2. is not that helpful for users in the end.

I'm not really familliar with Python best practices in this case.

It completely depends on the project. For example, in scikit-learn we have a strong backward-compatibility policy: we would have warnings for two major releases before we can break user code. I feel in dask-jobqueue we can afford to be less conservative.

@lesteve
Copy link
Member Author

lesteve commented Mar 6, 2020

Maybe @willirath you could have a quick look if you are around and not too busy with other things?

Copy link
Member

@guillaumeeb guillaumeeb left a comment

Choose a reason for hiding this comment

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

This looks good, just a few interrogations. Do you need this to be tested?

@@ -447,22 +453,47 @@ def __init__(
)
)

if dashboard_address is not None:
Copy link
Member

Choose a reason for hiding this comment

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

What feel's weird here is to keep the host and dashboard_address in the constructor kwargs, but thrown an exception when used. Maybe we should add some doc or comment, or simply remove them and catch them through **kwargs argument? Or else we should handle it and log a deprecation warning.

I'm not really familliar with Python best practices in this case.

# Use the same network interface as the workers if scheduler ip has not
# been set through scheduler_options via 'host' or 'interface'
if "host" not in scheduler_options and "interface" not in scheduler_options:
scheduler_options["interface"] = interface
Copy link
Member

Choose a reason for hiding this comment

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

Should'nt we just put interface in the default_scheduler_options? Won't it be overriden if defined in the scheduler_options?

Or is this because of a problem in distributed if host and interface do not match?

Copy link
Member Author

@lesteve lesteve Mar 8, 2020

Choose a reason for hiding this comment

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

I should probably test this edge case indeed. My line of reasoning is like this (as I tried to explain in the comment but the wording can probably be improved ...): interface parameter applies primarily to workers. It should apply to the scheduler only if the scheduler IP is not already set by either host or interface in the scheduler_options dict.

@@ -202,9 +206,6 @@ def __init__(

if interface:
extra = extra + ["--interface", interface]
kwargs.setdefault("host", get_ip_interface(interface))
Copy link
Member Author

@lesteve lesteve Mar 16, 2020

Choose a reason for hiding this comment

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

After having looked a bit more in details, I am reasonably sure that host in kwargs is not used at all, so this code can be safely removed. In other words this is another manifestation of #386.

This code was introduced in #135. At that time the kwargs were passed to LocalCluster. Nowadays the kwargs are passed to the job_cls. This does not make any sense to pass host=something to the worker arguments where something is computed on the node the scheduler runs (similar comment as #382 (comment)) ...

@lesteve
Copy link
Member Author

lesteve commented Mar 19, 2020

I think this one should be ready to merge, if someone has some spare time to look at it, this would be much appreciated!

If I don't hear back, I plan to merge this one in a few days.

@lesteve
Copy link
Member Author

lesteve commented Mar 23, 2020

OK I am going to merge this one, do feel free to comment on this PR (even after the fact) if you have some improvements to suggest!

@lesteve lesteve changed the title Add scheduler options to pass scheduler interface and scheduler port (amongst other things) Add scheduler_options to pass scheduler-specific parameters Mar 23, 2020
@lesteve lesteve merged commit a85b64f into dask:master Mar 23, 2020
@lesteve lesteve deleted the add-scheduler-options branch March 23, 2020 07:47
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.

Using different interfaces for scheduler and workers Specify a cluster scheduler listener port?
2 participants