[Usage Stats] Record usage stats when dashboard disabled #26042

rkooo567 · 2022-06-23T16:24:38Z

Why are these changes needed?

Since usage stats are recorded from the dashboard (which will become API server), it is not collected when the dashboard is not included (include_dashboard=False).

This PR fixes the issues by

change dashboard -> API server (to avoid confusing users that dashboard is still started when include_dashboard=False)
Only load modules that are irrelevant to the dashboard from the API server, so it will have the same impact as no dashboard.

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 :(

dashboard/tests/test_dashboard.py

jjyao · 2022-06-29T23:11:22Z

python/ray/_private/node.py

+ include_dashboard: If true, this will load all modules of the
+ api server to start dashboard. Otherwise, it will only
+ start the modules that are not relevant to the dashboard.
+ raise_on_failure: If true, this will raise an exception
 if we fail to start the dashboard. Otherwise it will print


if we fail to start the api server or dashboard (since now they are different things)?

API server. I will fix it (there's no time dashboard can be started when API server doesn't)

python/ray/_private/node.py

python/ray/_private/services.py

jjyao · 2022-06-29T23:18:06Z

python/ray/_private/services.py

@@ -1488,7 +1499,7 @@ def start_dashboard(
 dashboard_url = ""
 return dashboard_url, process_info
 except Exception as e:
- if require_dashboard:
+ if raise_on_failure:
 raise e from e
 else:
 logger.error(f"Failed to start the dashboard: {e}")


dashboard -> api server? make sure other places are updated consistently

Actually, I will just keep the dashboard in this PR (it is a little random from end users saying API server now when the full refactoring is not done. For example, the process name is dashboard.py). I will address this comment when we change all dashboard -> API server.

jjyao · 2022-06-29T23:28:45Z

dashboard/dashboard.py

+ minimal: bool = False,
+ modules_to_load: Optional[Set[str]] = None,


What if minimal conflicts with modules_to_load?

Could you also remind me why do we need to pass in the --minimal flag instead of figuring out by itself inside dashboard process?

Actually, we figure out the minimal modules inside the dashboard process. --minimal was passed to not start HTTP server (which is only available at ray[default]).

Right now, if it conflicts, it will fail to start a server (I will add a test). It is okay to not handle this well now because this is not exposed to users (and modules_to_load is only used for this use case although this can be used for replicating the API server with different functionalities in the future).

Also in the follow-up PR, I will clean up this code path (rename --minimal to --disable-http-server, and choose minimal modules at the proc start time and use modules_to_load to start them).

rkooo567 · 2022-06-30T17:21:52Z

@jjyao will address your comments soon

dashboard/dashboard.py

dashboard/modules/usage_stats/usage_stats_head.py

jjyao · 2022-07-27T18:47:25Z

Failed tests seems related

//python/ray/tests:test_usage_stats                                     TIMEOUT in 3 out of 3 in 303.0s

jjyao · 2022-07-27T18:58:43Z

Oh, seems we are on the edge. Just need to change it from medium to large size.

jjyao · 2022-07-27T21:16:37Z

Marked the test as large: #27111. Could you pull it?

rkooo567 · 2022-07-28T01:01:48Z

Will do. Do you think we need to cherry pick this?

rkooo567 · 2022-07-28T01:32:55Z

pulled it just now

jjyao · 2022-07-28T04:01:59Z

Do you think we need to cherry pick this?
If it's low risk, I think we should cherry pick it.

rkooo567 · 2022-07-29T06:02:27Z

Let me see if this breaks anything for next 3~4 days. I think this PR is probably not very low risk PR because I have failed CI many times before merging it..

…#26042) Since usage stats are recorded from the dashboard (which will become API server), it is not collected when the dashboard is not included (include_dashboard=False). This PR fixes the issues by change dashboard -> API server (to avoid confusing users that dashboard is still started when include_dashboard=False) Only load modules that are irrelevant to the dashboard from the API server, so it will have the same impact as no dashboard. Signed-off-by: Frank Luan <[email protected]>

…#26042) Since usage stats are recorded from the dashboard (which will become API server), it is not collected when the dashboard is not included (include_dashboard=False). This PR fixes the issues by change dashboard -> API server (to avoid confusing users that dashboard is still started when include_dashboard=False) Only load modules that are irrelevant to the dashboard from the API server, so it will have the same impact as no dashboard. Signed-off-by: Scott Graham <[email protected]>

…#26042) Since usage stats are recorded from the dashboard (which will become API server), it is not collected when the dashboard is not included (include_dashboard=False). This PR fixes the issues by change dashboard -> API server (to avoid confusing users that dashboard is still started when include_dashboard=False) Only load modules that are irrelevant to the dashboard from the API server, so it will have the same impact as no dashboard.

…#26042) Since usage stats are recorded from the dashboard (which will become API server), it is not collected when the dashboard is not included (include_dashboard=False). This PR fixes the issues by change dashboard -> API server (to avoid confusing users that dashboard is still started when include_dashboard=False) Only load modules that are irrelevant to the dashboard from the API server, so it will have the same impact as no dashboard. Signed-off-by: Chong-Li <[email protected]>

…#26042) Since usage stats are recorded from the dashboard (which will become API server), it is not collected when the dashboard is not included (include_dashboard=False). This PR fixes the issues by change dashboard -> API server (to avoid confusing users that dashboard is still started when include_dashboard=False) Only load modules that are irrelevant to the dashboard from the API server, so it will have the same impact as no dashboard. Signed-off-by: Stefan van der Kleij <[email protected]>

rkooo567 added 3 commits June 23, 2022 09:10

Completed.

fd6ddab

Done.

1cc7557

lint

60843e2

rkooo567 requested review from ericl, richardliaw and jjyao as code owners June 23, 2022 16:24

jjyao reviewed Jun 29, 2022

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 Jul 24, 2022

Merge branch 'master' into usage-stats-when-dashboard-disabled

c41802b

rkooo567 assigned jjyao Jul 26, 2022

rkooo567 removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Jul 26, 2022

rkooo567 added 2 commits July 26, 2022 06:23

done.

bfcbc99

Fix a lint issue.

892c7a9

jjyao approved these changes Jul 26, 2022

View reviewed changes

dashboard/dashboard.py Outdated Show resolved Hide resolved

dashboard/modules/usage_stats/usage_stats_head.py Outdated Show resolved Hide resolved

rkooo567 added 6 commits July 27, 2022 01:43

Merge branch 'master' into usage-stats-when-dashboard-disabled

5b92fbc

ADdressed code review + fix test failures.

6c06a4f

Merge branch 'master' into usage-stats-when-dashboard-disabled

5b8232a

Fixed the test failure

19b9cf6

Merge branch 'master' into usage-stats-when-dashboard-disabled

94a0384

Fixed an unit test.

7643fcf

Merge branch 'master' into usage-stats-when-dashboard-disabled

00b07d1

rkooo567 merged commit 16aa102 into ray-project:master Jul 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Usage Stats] Record usage stats when dashboard disabled #26042

[Usage Stats] Record usage stats when dashboard disabled #26042

rkooo567 commented Jun 23, 2022

jjyao Jun 29, 2022

rkooo567 Jul 26, 2022

jjyao Jun 29, 2022

rkooo567 Jul 26, 2022

jjyao Jun 29, 2022

rkooo567 Jul 26, 2022

rkooo567 commented Jun 30, 2022

jjyao commented Jul 27, 2022

jjyao commented Jul 27, 2022

jjyao commented Jul 27, 2022

rkooo567 commented Jul 28, 2022

rkooo567 commented Jul 28, 2022

jjyao commented Jul 28, 2022

rkooo567 commented Jul 29, 2022

		minimal: bool = False,
		modules_to_load: Optional[Set[str]] = None,

[Usage Stats] Record usage stats when dashboard disabled #26042

[Usage Stats] Record usage stats when dashboard disabled #26042

Conversation

rkooo567 commented Jun 23, 2022

Why are these changes needed?

Related issue number

Checks

jjyao Jun 29, 2022

Choose a reason for hiding this comment

rkooo567 Jul 26, 2022

Choose a reason for hiding this comment

jjyao Jun 29, 2022

Choose a reason for hiding this comment

rkooo567 Jul 26, 2022

Choose a reason for hiding this comment

jjyao Jun 29, 2022

Choose a reason for hiding this comment

rkooo567 Jul 26, 2022

Choose a reason for hiding this comment

rkooo567 commented Jun 30, 2022

jjyao commented Jul 27, 2022

jjyao commented Jul 27, 2022

jjyao commented Jul 27, 2022

rkooo567 commented Jul 28, 2022

rkooo567 commented Jul 28, 2022

jjyao commented Jul 28, 2022

rkooo567 commented Jul 29, 2022