Make Dashboard Port Configurable #8999

mfitton · 2020-06-17T22:24:24Z

Why are these changes needed?

These changes are needed because currently the user cannot specify the port that they wish to start the dashboard on, and it is frequently requested as a feature. In addition, this makes the ray dashboard command work with a remote port that is not the default dashboard port, so that this can be supported in the autoscaler as well.
In addition, I renamed the variables that mention webui as we have started referring to the web ui as the dashboard widely.

Related issue number

Checks

I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/latest/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failure rates at https://ray-travis-tracker.herokuapp.com/.
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested (please justify below)

…name an existing argument from include-webui to include-dashboard as part of more consistent naming, and update the dashboard command to be able to connect to an arbitrary remote port on a cluster

AmplabJenkins · 2020-06-17T22:28:29Z

Can one of the admins verify this patch?

AmplabJenkins · 2020-06-18T01:52:34Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27264/
Test FAILed.

edoakes

LGTM. Please ping when tests pass and you've tested locally

edoakes · 2020-06-18T02:38:21Z

ci/long_running_tests/workloads/serve.py

@@ -31,7 +31,8 @@
    "chmod +x hey_linux_amd64"
 ])

-ray.init(address=cluster.address, include_webui=True, webui_host="0.0.0.0")
+ray.init(
+    address=cluster.address, include_dashboard=True, dashboard_host="0.0.0.0")


Don't need include_dashboard right?

edoakes · 2020-06-18T02:38:25Z

ci/long_running_tests/workloads/serve_failure.py


 ray.init(
    address=cluster.address,
-    include_webui=True,
-    webui_host="0.0.0.0",
+    include_dashboard=True,


Don't need include_dashboard right?

Yeah, you're correct actually. I mindlessly put it there because we had passed include_webui before hehe.

edoakes · 2020-06-18T02:39:32Z

python/ray/scripts/scripts.py

+    " (DEPRECATED: please use --dashboard-host)")
+@click.option(
+    "--include-dashboard",
+    default=None,


Should we be starting it by default? A question for another PR

I am confused. It must be started by default right? Why is i the question for another PR?

We do start it by default already. In retrospect, my comment description here could've been more clear. I will rewrite. The default arg of None starts the dashboard.

Oh, interesting. So you have to set --include-dashboard=False to not start it? I think I was confused by the flag name.

edoakes · 2020-06-18T02:40:53Z

python/ray/scripts/scripts.py

@@ -323,6 +350,20 @@ def start(node_ip_address, redis_address, address, redis_port, port,
        if port is not None and port != redis_port:
            raise ValueError("Cannot specify both --port and --redis-port "
                             "as port is a rename of deprecated redis-port")
+    if include_webui is not None:


If it's not None we should probably either error or set the value for include_dashboard to what include_webui is set to

Similar to what you have for the host flags below

Makes sense.

edoakes · 2020-06-18T02:42:44Z

python/ray/scripts/scripts.py

@@ -443,8 +485,13 @@ def start(node_ip_address, redis_address, address, redis_port, port,
            raise Exception("If --head is not passed in, --redis-max-clients "
                            "must not be provided.")
        if include_webui:
-            raise Exception("If --head is not passed in, the --include-webui "
-                            "flag is not relevant.")
+            raise Exception(


Try not to raise bare Exceptions, we should probably raise ValueError in this case

rkooo567 · 2020-06-18T03:22:37Z

python/ray/scripts/scripts.py

@@ -323,6 +350,20 @@ def start(node_ip_address, redis_address, address, redis_port, port,
        if port is not None and port != redis_port:
            raise ValueError("Cannot specify both --port and --redis-port "
                             "as port is a rename of deprecated redis-port")
+    if include_webui is not None:
+        logger.warn("The --include-webui argument will be deprecated soon. "


Can you write something more specific? For example, --include-webui argument will be deprecated at the version 0.8.7

This makes sense. I'm concerned I'll forget to remove it for the 0.8.7 release though. Do we have a way we normally keep track of deprecations?

No need to specify a release, what you have is fine

rkooo567 · 2020-06-18T03:23:11Z

python/ray/scripts/scripts.py

+
+    dashboard_host_default = "localhost"
+    if webui_host != dashboard_host_default:
+        logger.warn("The --webui-host argument will be deprecated soon. "


rkooo567 · 2020-06-18T03:24:10Z

python/ray/scripts/scripts.py

+    " (DEPRECATED: please use --dashboard-host)")
+@click.option(
+    "--include-dashboard",
+    default=None,


I am confused. It must be started by default right? Why is i the question for another PR?

AmplabJenkins · 2020-06-18T03:49:36Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27265/
Test FAILed.

skbly7 · 2020-06-18T19:11:05Z

python/ray/services.py

@@ -1125,7 +1128,6 @@ def start_dashboard(require_webui,
    Returns:
        ProcessInfo for the process that was started.
    """
-    port = 8265  # Note: list(map(ord, "RAY")) == [82, 65, 89]
    while True:


I guess you would like to maintain old behavior i.e. port += 1 only when port == ray_constants.DEFAULT_DASHBOARD_PORT, or the documentation above needs update to:

(Note that the port number increases if the ~~default~~ port is not available)

Good point, thank you.

AmplabJenkins · 2020-06-18T19:39:38Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27294/
Test FAILed.

…ior if the default port is passed in for the dashboard.

AmplabJenkins · 2020-06-19T00:00:33Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Ray-PRB/27304/
Test FAILed.

Max Fitton added 2 commits June 17, 2020 14:28

Make it possible to set the port that the dashboard should run on, re…

d994aca

…name an existing argument from include-webui to include-dashboard as part of more consistent naming, and update the dashboard command to be able to connect to an arbitrary remote port on a cluster

add back in warning about irrelevant use of include-webui

78a388b

edoakes approved these changes Jun 18, 2020

View reviewed changes

rkooo567 reviewed Jun 18, 2020

View reviewed changes

address pr feedback

54d31ec

skbly7 reviewed Jun 18, 2020

View reviewed changes

Revert exact deprecation date and only do the port incrementing behav…

e993aa6

…ior if the default port is passed in for the dashboard.

edoakes merged commit ad09aa9 into ray-project:master Jun 19, 2020

This was referenced Aug 27, 2020

Upgrade to Ray 0.8.7 modin-project/modin#1958

Closed

FEAT-#1958: pin ray to 0.8.7 modin-project/modin#1966

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make Dashboard Port Configurable #8999

Make Dashboard Port Configurable #8999

mfitton commented Jun 17, 2020

AmplabJenkins commented Jun 17, 2020

AmplabJenkins commented Jun 18, 2020

edoakes left a comment

edoakes Jun 18, 2020

edoakes Jun 18, 2020

mfitton Jun 18, 2020

edoakes Jun 18, 2020

rkooo567 Jun 18, 2020

mfitton Jun 18, 2020

edoakes Jun 18, 2020

edoakes Jun 18, 2020

edoakes Jun 18, 2020

mfitton Jun 18, 2020

edoakes Jun 18, 2020

rkooo567 Jun 18, 2020

mfitton Jun 18, 2020

edoakes Jun 18, 2020

rkooo567 Jun 18, 2020

rkooo567 Jun 18, 2020

AmplabJenkins commented Jun 18, 2020

skbly7 Jun 18, 2020

mfitton Jun 18, 2020

AmplabJenkins commented Jun 18, 2020

AmplabJenkins commented Jun 19, 2020

Make Dashboard Port Configurable #8999

Make Dashboard Port Configurable #8999

Conversation

mfitton commented Jun 17, 2020

Why are these changes needed?

Related issue number

Checks

AmplabJenkins commented Jun 17, 2020

AmplabJenkins commented Jun 18, 2020

edoakes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Jun 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AmplabJenkins commented Jun 18, 2020

AmplabJenkins commented Jun 19, 2020