[Dashboard] Turn on New Dashboard by Default by mfitton · Pull Request #11321 · ray-project/ray

mfitton · 2020-10-09T22:24:05Z

Why are these changes needed?

This PR turns on the new dashboard backend by default.

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/master/.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

…t. This will likely fail and I will need to remove some testing associated with the old dashboard.

…cs. Test code that has been removed has been added to the dashboard sub tests

… tests

… dashboard does not run the reporter process on its head node

…t it is still failing for some reason

…tirely sure why the new_dashboard dir fails.

…test suites, cross platform. I removed it from test-wheels because it seemed to inexplicably hit import errors where it does not in other tests. I think it should have no impact removing from here.

mfitton · 2020-10-13T00:58:02Z

Looks like it passes all tests it should at this point.

fyrestone · 2020-10-13T08:36:59Z

dashboard/agent.py

        type=int,
        help="The port to expose metrics through Prometheus.")
+    parser.add_argument(
+        "--metrics-agent-port",


I think it's better to call it --dashboard-agent-port, because many agent grpc services will share this port. For example, reporter agent, job agent, ...

Good point, thank you

fyrestone · 2020-10-13T08:38:05Z

dashboard/agent.py

        self.server = aiogrpc.server(options=(("grpc.so_reuseport", 0), ))
-        self.grpc_port = self.server.add_insecure_port("[::]:0")
+        self.grpc_port = self.server.add_insecure_port(
+            f"[::]:{self.metrics_agent_port}")


Also, it's better to rename it to dashboard_agent_port.

fyrestone · 2020-10-13T08:43:56Z

dashboard/modules/reporter/reporter_agent.py

 import psutil

 logger = logging.getLogger(__name__)
+routes = dashboard_utils.ClassMethodRouteTable


routes is for RESTful APIs, but i can't find any routes usages in reporter_agent.

Good catch, it shouldn't be there, thanks.

So bizarre, I must have pushed these changes to a different branch or something, I remember addressing these this morning.

fyrestone · 2020-10-13T08:49:42Z

dashboard/modules/reporter/reporter_agent.py

-    async def ReportMetrics(self, request, context):
-        # NOTE: Exceptions are not propagated properly
-        # when we don't catch them here.
+    async def ReportOCMetrics(self, request, context):


Can you add a comment for this method? It's hard to get what OC means.

Sure, I will.

ping on adding a comment. what's OC?

It is OpenCensus

fyrestone · 2020-10-13T08:54:30Z

python/ray/node.py

-            stdout_file, stderr_file = self.get_log_file_handles(
-                "dashboard", unique=True)
+        stdout_file, stderr_file = self.get_log_file_handles(
+            "dashboard", unique=True)


Dashboard use a file logger by default (the dashboard.log), it's not necessary to redirect the stdout and stderr.

I switched it back to the old behavior because a bunch of tests broke in other parts of the codebase. Plus, in this case error and non-error logs go to different places, which I think is desirable.

@rkooo567 @edoakes do either of you have opinions here?

stdout redirection can neither rotate log files nor fragment log files. It is not good for long running cluster. Three log files may confuse users. (dashboard.out, dashboard.err, dashboard.log)

Hmm having a separate .err is useful though. Is there any way we can have .out (rotated) and .err?

Another way to have a seperate .err with rotate feature (not tested):

stdout_file, stderr_file = None, None

Two RotatingFileHandler instances,

One level is WARNING, filename is dashboard.err.

The other level is INFO, filename is dashboard.out, --logging-level only affects this one.

The drawback is, the log with level >= WARNING will be printed twice, it will be slower than one dashboard.log.

I see. I am okay with mixed logs because I agree we should make all logs rotational. @mfitton Can you just revert back to the original (@fyrestone)'s implementation?

…ng the dashboard.

…of building the old dashboard.

mfitton · 2020-10-13T18:00:47Z

Found an issue, which was that the wheels being built were not including the compiled new_dashboard. I switched the dir in the build-wheels shell scripts to rectify.

edoakes · 2020-10-13T20:40:45Z

python/ray/_private/services.py

-        dashboard_dir = "dashboard"
-        logdir = None
-
+    dashboard_dir = "new_dashboard"


I assume we'll remove the "new" prefix in a future PR

Yeah, that'll be done when we remove the old dashboard, which we decided not to do just yet in case reverting is needed.

edoakes · 2020-10-13T20:41:16Z

dashboard/modules/reporter/reporter_agent.py

-    async def ReportMetrics(self, request, context):
-        # NOTE: Exceptions are not propagated properly
-        # when we don't catch them here.
+    async def ReportOCMetrics(self, request, context):


ping on adding a comment. what's OC?

edoakes · 2020-10-13T20:41:36Z

dashboard/modules/reporter/reporter_agent.py

 import psutil

 logger = logging.getLogger(__name__)
+routes = dashboard_utils.ClassMethodRouteTable


rkooo567 · 2020-10-14T02:47:23Z

dashboard/agent.py


        agent = DashboardAgent(
            args.redis_address,
+            args.dashboard_agent_port,


rkooo567 · 2020-10-14T02:47:56Z

dashboard/modules/logical_view/test_logical_view_head.py

+    assert wait_until_server_available(webui_url)
+    webui_url = format_web_url(webui_url)
+
+    def actor_killed(PID):


Why is the argument uppercase?

I copied it over. Happy to change.

rkooo567 · 2020-10-14T02:49:10Z

dashboard/modules/logical_view/test_logical_view_head.py

+        actor = actor_groups["Actor"]["entries"][0]
+        return actor
+
+    def kill_actor(actor):


Suggested change

def kill_actor(actor):

def kill_actor_using_dashboard(actor):

rkooo567 · 2020-10-14T02:50:11Z

dashboard/modules/logical_view/test_logical_view_head.py

+        except (KeyError, AssertionError) as e:
+            last_exc = e
+            time.sleep(.1)
+    assert last_exc is None


Suggested change

assert last_exc is None

assert last_exc is None, (f"last exception: {last_exc}")

If I assert last_exc is None and last_exc is not None, it already prints an assertion error that says what last_exc was.

oh but if last_exc is not None, this assertion will fail, and we'd like to print a better message right?

rkooo567 · 2020-10-14T02:50:26Z

dashboard/modules/reporter/reporter_agent.py

-    async def ReportMetrics(self, request, context):
-        # NOTE: Exceptions are not propagated properly
-        # when we don't catch them here.
+    async def ReportOCMetrics(self, request, context):


It is OpenCensus

rkooo567 · 2020-10-14T02:51:19Z

python/build-wheel-manylinux1.sh


 # Build the dashboard so its static assets can be included in the wheel.
-pushd python/ray/dashboard/client
+pushd python/ray/new_dashboard/client


Add TODO here to revert it back to dashboard when we delete the old dashboard?

rkooo567 · 2020-10-14T02:51:57Z

python/ray/_private/services.py

+        dashboard_dependencies_present = False
        warning_message = (
            "Failed to start the dashboard. The dashboard requires Python 3 "
            "as well as 'pip install aiohttp grpcio'.")


Is this warning message still valid?

rkooo567 · 2020-10-14T02:52:19Z

python/ray/_private/services.py

-        "--raylet-name={}".format(raylet_name),
-        "--temp-dir={}".format(temp_dir),
+        f"--redis-address={redis_address}",
+        f"--metrics-export-port={metrics_export_port}",


Do you mind changing this name to dashboard_agent_port?

They're different in this case, right? The agent port is where other processes send their metrics whereas the metrics export port is where prometheus actually scrapes the states from.

Yeah I meant to change metrics_agenet_port. My bad that I was writing a comment in the wrong line.

Since metrics agent is just the dashboard agent now

rkooo567 · 2020-10-14T02:53:55Z

python/ray/node.py

-            stdout_file, stderr_file = self.get_log_file_handles(
-                "dashboard", unique=True)
+        stdout_file, stderr_file = self.get_log_file_handles(
+            "dashboard", unique=True)


Hmm having a separate .err is useful though. Is there any way we can have .out (rotated) and .err?

rkooo567 · 2020-10-14T02:54:48Z

python/ray/tests/test_webui.py

-import pytest
-import requests
-
-import ray


We probably need to delete this from BUILD.bazel. Same for test_metrics. Also, this test is run as a sanity check for the wheel builds. I think we can do the same thing for the new dashboard.

I removed both of them from BUILD.bazel
As far as running it as a sanity check for the wheels, that makes sense. I was having trouble running a test inside the dashboard directory in this case. There was an import error importing things from conftest. Given how long the cycle time is in trying to fix something like that over CI, I decided it probably wasn't worth the effort. What do you think?

mfitton · 2020-10-16T22:34:11Z

@fyrestone
I'm hitting errors in several tests when making the change you recommended of not redirecting stdout and stderr for the dashboard process.

Somehow as an effect, tests that rely on call_ray_start in conftest.py hang indefinitely on this line:

out = ray.utils.decode(subprocess.check_output(command_args, stderr=subprocess.STDOUT))

Ray starts up correctly (I can access the dashboard and see the processes running on my laptop and even connect and spawn actors manually); however, somehow the output never gets successfully read. Do you know how the two could be related? I must be missing something. I'd really appreciate your help digging into this, as I've spent a couple hours on it, and this is the reason I made the change in the first place.

I also noticed we use a private function _run_app instead of the exposed run_app in aiohttp. Why do we do this?

web_server = aiohttp.web._run_app(
            app, host=self.http_host, port=self.http_port)

fyrestone · 2020-10-19T03:16:53Z

http_port

Let me find out why the check_output hangs.
About the _run_app.
- The run_app not only creates the http server coroutine but also waits the coroutine finished, it means that the run_app will block on the http server exit.
- The _run_app just create a http server coroutine, so we can run the http server in our event loop.
There is a way to avoid using private _run_app, just like the agent http server creation,
- Create a aiohttp.web.Application()
- Add routes to app.
- Create a aiohttp.web.AppRunner on app
- Setup runner
- Create a aiohttp.web.TCPSite on runner
- Start site
- ...
The _run_app is a shortcut of above steps. Dashboard agent use above steps is to enable CORS. So, I choose the shortcut _run_app for dashboard head.

fyrestone · 2020-10-19T08:19:13Z

@mfitton About hanging on check_output. The child (dashboard) inherit fds from parent (ray.scripts.scripts). The dashboard process is long running, it prints stdout / stderr to the parent's fd. So, the check_output can't get the EOF until dashboard exit.

The process tree is,

check_output, waiting for the EOF of stdout from [1]
- [1] ray start ... (ray.scripts.scripts), become a zombie because it's stdout / stderr is not closed.
  - [2] dashboard.py, inherits stdout from [1]

A solution is to redirect the stdout & stderr to devnull: antgroup@59e3250

Ref: https://docs.python.org/3/library/subprocess.html?highlight=popen#subprocess.Popen

With the default settings of None, no redirection will occur; the child’s file handles will be inherited from the parent.

mfitton · 2020-10-19T15:02:21Z

@fyrestone many thanks for your work digging into that. I'll cherry pick the commit onto this branch.

This reverts commit f500292.

#11502)" This reverts commit cdca5af.

Max Fitton added 18 commits October 5, 2020 15:46

Remove the switch for the new dashboard backend to make it the defaul…

c0a61db

…t. This will likely fail and I will need to remove some testing associated with the old dashboard.

Remove dead tests and dead code from tests that live on in test_metri…

58a8596

…cs. Test code that has been removed has been added to the dashboard sub tests

Remove test_webui as that is now covered by the new dashboard backend…

9490768

… tests

remove test webui and account for new output in test_output

9a312f2

add change corresponding to kai's change in PR ray-project#11232

cec9f50

Merge branch 'master' into turn-on-new-dashboard

0f49f29

fix broken logical view test

f018d19

make the old reporter start even with the new dashboard since the new…

19696fe

… dashboard does not run the reporter process on its head node

fix some tests by changing the output redirection for new dashboard.

03dba04

lint

f7109c3

Trying to fix the grpc call for stats aggregation in the reporter, bu…

6f08d3c

…t it is still failing for some reason

Fix test_metrics_agent for the new dashboard

413413c

small change to trigger new tests

55bccf5

Merge branch 'turn-on-new-dashboard' into turn-on-new-dashboard-smh

fd71a98

Fix test_output and lint. Should be passing everything in CI now.

2729797

Update the wheels web_ui test to one of the new dashboard tests

aa03bba

try using the dashboard dir rather than the new_dashboard dir. Not en…

71a7e9d

…tirely sure why the new_dashboard dir fails.

The test that used to be run in test_webui now runs in all the other …

fcae0da

…test suites, cross platform. I removed it from test-wheels because it seemed to inexplicably hit import errors where it does not in other tests. I think it should have no impact removing from here.

mfitton changed the title ~~[WIP] New Dashboard~~ [Dashboard] Turn on New Dashboard by Default Oct 13, 2020

mfitton requested review from fyrestone and rkooo567 October 13, 2020 00:56

fyrestone reviewed Oct 13, 2020

View reviewed changes

Change name of metrics_agent_port to dashboard_agent_port when starti…

7845524

…ng the dashboard.

mfitton assigned rkooo567 Oct 13, 2020

Update wheels so that they build the new dashboard correctly instead …

ed96226

…of building the old dashboard.

Make setup-dev sym link the new_dashboard as well as the old one

ba114f7

zhe-thoughts mentioned this pull request Oct 13, 2020

[New Dashboard] Turn it on by default. #10824

Closed

zhe-thoughts linked an issue Oct 13, 2020 that may be closed by this pull request

[New Dashboard] Turn it on by default. #10824

Closed

edoakes approved these changes Oct 13, 2020

View reviewed changes

Comment for OCMetrics and remove unnecessary routes

701d966

rkooo567 reviewed Oct 14, 2020

View reviewed changes

rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 14, 2020

Max Fitton added 2 commits October 15, 2020 18:48

Change comment style because it was not appearing on remote...

fe689d9

Address Sang pr comments

180840d

Redirect dashboard stdout / stderr to devnull to avoid hang

af75260

mfitton force-pushed the turn-on-new-dashboard-smh branch from 4dcd921 to af75260 Compare October 19, 2020 15:05

mfitton removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Oct 19, 2020

edoakes merged commit f500292 into ray-project:master Oct 19, 2020

mfitton deleted the turn-on-new-dashboard-smh branch October 19, 2020 20:59

mfitton added a commit that referenced this pull request Oct 20, 2020

Revert "[Dashboard] Turn on New Dashboard by Default (#11321)"

e155179

This reverts commit f500292.

edoakes pushed a commit that referenced this pull request Oct 20, 2020

Revert "[Dashboard] Turn on New Dashboard by Default (#11321)" (#11502)

cdca5af

This reverts commit f500292.

mfitton added a commit that referenced this pull request Oct 20, 2020

Revert "Revert "[Dashboard] Turn on New Dashboard by Default (#11321)" (

85f4e3d

#11502)" This reverts commit cdca5af.

fyrestone mentioned this pull request Oct 22, 2020

[Dashboard] Optimize dashboard datacenter #11391

Merged

6 tasks

	def kill_actor(actor):
	def kill_actor_using_dashboard(actor):

	assert last_exc is None
	assert last_exc is None, (f"last exception: {last_exc}")

Conversation

mfitton commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are these changes needed?

Related issue number

Checks

Uh oh!

mfitton commented Oct 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fyrestone Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfitton commented Oct 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mfitton commented Oct 9, 2020 •

edited

Loading

fyrestone Oct 14, 2020 •

edited

Loading

mfitton commented Oct 16, 2020 •

edited

Loading

fyrestone commented Oct 19, 2020 •

edited

Loading