[Core] Resubmit #47871 & #48030 and Fix the Failed Test for Windows #48054

MengjinYan · 2024-10-16T21:17:34Z

Why are these changes needed?

Resubmit [Ray Core] Fix the Paths in the Generated Monitoring Configs to Consider temp-dir #47871 and Fix Style Changes Suggested in #47871 #48030 as they are reverted as release blocker
Fixed the failed test in windows CI test.
- The test test_metrics_folder_and_content failed because of the different file separator between linux and Windows
- The PR fix the issue by using os.path.join to generate the path for validation instead of directly using formatted string

Related issue number

Closes #48037

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
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 added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
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 :(

… Configs to Consider temp-dir Signed-off-by: Mengjin Yan <[email protected]>

Signed-off-by: Mengjin Yan <[email protected]>

aslonnie

is this PR ready for review yet?

aslonnie · 2024-10-17T02:11:06Z

python/ray/dashboard/modules/metrics/grafana_dashboard_provisioning_template.py

@@ -1,10 +1,10 @@
-DASHBOARD_PROVISIONING_TEMPLATE = """
-apiVersion: 1
+# DASHBOARD_PROVISIONING_TEMPLATE = """


why are these lines commented out?

Thanks for pointing out!

This file and python/ray/dashboard/modules/metrics/grafana_datasource_template.py should be deleted as I moved the content to the more general template file: python/ray/dashboard/modules/metrics/templates.py. Something might be wrong when I reapplying the patch from the previous PR #47871. I'll update the PR to remove the files.

MengjinYan · 2024-10-17T04:47:47Z

is this PR ready for review yet?

The original changes are reverted because a windows test failed due to the change. I did the fix and now waiting for all the CI tests to pass. It will be ready for review after the CI tests passed.

…mplate file Signed-off-by: Mengjin Yan <[email protected]>

2. Make the test that specifies the temp-dir parameter doesn't run in Windows. This is because the specified path doesn't apply to Windows Signed-off-by: Mengjin Yan <[email protected]>

rynewang · 2024-10-18T19:56:32Z

This is surprising since I thought Python can treat / as dir separator on Windows as well. But anyway LGTM

MengjinYan · 2024-10-18T20:11:26Z

This is surprising since I thought Python can treat / as dir separator on Windows as well. But anyway LGTM

is this PR ready for review yet?

The original changes are reverted because a windows test failed due to the change. I did the fix and now waiting for all the CI tests to pass. It will be ready for review after the CI tests passed.

@aslonnie The PR passed the Windows test and I made sure the reverted changes are all included in this one so it should be ready to review.

MengjinYan · 2024-10-18T20:18:24Z

This is surprising since I thought Python can treat / as dir separator on Windows as well. But anyway LGTM

Thanks! I think there are 2 things we hit in the Windows test:

The temp path in Windows are different from that on linux. On Windows, it is "C:\Users..." while on Linux, we are using "/tmp/ray", so we cannot assume the temp path to always be "/tmp/ray"
Originally in the test, I used formatted string to generate the path instead of os.path. This cause the mismatch of the separator.

angelinalg

Some style nits if they can be fixed in a separate PR.

angelinalg · 2024-10-18T21:32:28Z

python/ray/dashboard/modules/metrics/install_and_start_prometheus.py

+    # The function assumes the Ray cluster to be monitored by Prometheus uses the
+    # default configuration with "/tmp/ray" as the default root temporary directory.
+    #
+    # This is to support the `ray metrics launch-prometheus` command, when a ray cluster


...when a Ray cluster...

angelinalg · 2024-10-18T21:34:47Z

python/ray/dashboard/modules/metrics/install_and_start_prometheus.py

+    # default configuration with "/tmp/ray" as the default root temporary directory.
+    #
+    # This is to support the `ray metrics launch-prometheus` command, when a ray cluster
+    # is not started yet and we have no way to get a `--temp-dir` anywhere. So we choose


hasn't started yet and the user doesn't have a way to.....

angelinalg · 2024-10-18T21:35:05Z

python/ray/dashboard/modules/metrics/metrics_head.py

-            shutil.rmtree(grafana_config_output_path)
-        os.makedirs(os.path.dirname(grafana_config_output_path), exist_ok=True)
-        shutil.copytree(GRAFANA_CONFIG_INPUT_PATH, grafana_config_output_path)
+        # Create grafana configuration folder


Create Grafana configuration folder.

angelinalg · 2024-10-18T21:35:19Z

python/ray/dashboard/modules/metrics/metrics_head.py

+            shutil.rmtree(self._grafana_config_output_path)
+        os.makedirs(self._grafana_config_output_path, exist_ok=True)
+
+        # Overwrite grafana's configuration file


verwrite Grafana's configuration file.

angelinalg · 2024-10-18T21:35:37Z

python/ray/dashboard/modules/metrics/metrics_head.py

+                GRAFANA_INI_TEMPLATE.format(
+                    grafana_provisioning_folder=grafana_prov_folder_with_latest_session
+                )
+            )

        # Overwrite grafana's dashboard provisioning directory based on env var


Overwrite Grafana's dashboard provisioning directory based on env var.

angelinalg · 2024-10-18T21:37:18Z

python/ray/dashboard/modules/metrics/templates.py

+  # scrape_timeout is set to the global default (10s).
+
+scrape_configs:
+# Scrape from each ray node as defined in the service_discovery.json provided by ray.


Ray node...provided by Ray.
(capitalize Ray)

angelinalg · 2024-10-18T21:37:41Z

python/ray/dashboard/modules/tests/test_metrics_integration.py

+def test_prometheus_config_content():
+    # Test to make sure the content in the hardcoded file
+    # (python/ray/dashboard/modules/metrics/export/prometheus/prometheus.yml) will
+    # always be the same as the template (templates.py) used to generate prometheus


Prometheus
(capitalize Prometheus)

angelinalg · 2024-10-18T21:37:52Z

python/ray/dashboard/modules/tests/test_metrics_integration.py

+    # Test to make sure the content in the hardcoded file
+    # (python/ray/dashboard/modules/metrics/export/prometheus/prometheus.yml) will
+    # always be the same as the template (templates.py) used to generate prometheus
+    # config file when ray startup


when Ray starts up.

MengjinYan added 3 commits October 16, 2024 13:45

Resubmit: #47871 [Ray Core] Fix the Paths in the Generated Monitoring…

1740d4f

… Configs to Consider temp-dir Signed-off-by: Mengjin Yan <[email protected]>

Resubmit: #48030 Fix Style Changes Suggested in #47871

2561ac8

Signed-off-by: Mengjin Yan <[email protected]>

Fix test_metrics_folder_and_content for Windows environment

ad44a11

Signed-off-by: Mengjin Yan <[email protected]>

MengjinYan added the go add ONLY when ready to merge, run all tests label Oct 16, 2024

MengjinYan requested review from maxpumperla, pcmoritz, kevin85421 and a team as code owners October 16, 2024 21:17

Fix lint error

f004350

Signed-off-by: Mengjin Yan <[email protected]>

aslonnie requested review from jjyao and rynewang October 17, 2024 02:10

aslonnie reviewed Oct 17, 2024

View reviewed changes

MengjinYan added 2 commits October 16, 2024 22:08

Remove the template files that are already merged into the general te…

585654b

…mplate file Signed-off-by: Mengjin Yan <[email protected]>

1. Fix the temp path in "test_metrics_folder_and_content"

69974b2

2. Make the test that specifies the temp-dir parameter doesn't run in Windows. This is because the specified path doesn't apply to Windows Signed-off-by: Mengjin Yan <[email protected]>

rynewang enabled auto-merge (squash) October 18, 2024 19:56

rynewang assigned angelinalg Oct 18, 2024

jjyao approved these changes Oct 18, 2024

View reviewed changes

pcmoritz approved these changes Oct 18, 2024

View reviewed changes

rynewang merged commit 75f6047 into master Oct 18, 2024
6 checks passed

rynewang deleted the issue-41648-2 branch October 18, 2024 20:32

angelinalg reviewed Oct 18, 2024

View reviewed changes

MengjinYan mentioned this pull request Oct 18, 2024

[Core] Fix Additional Code Style Suggestions from #48054 #48101

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Resubmit #47871 & #48030 and Fix the Failed Test for Windows #48054

[Core] Resubmit #47871 & #48030 and Fix the Failed Test for Windows #48054

MengjinYan commented Oct 16, 2024 •

edited

Loading

aslonnie left a comment

aslonnie Oct 17, 2024

MengjinYan Oct 17, 2024

MengjinYan commented Oct 17, 2024 •

edited

Loading

rynewang commented Oct 18, 2024

MengjinYan commented Oct 18, 2024

MengjinYan commented Oct 18, 2024

angelinalg left a comment

angelinalg Oct 18, 2024

angelinalg Oct 18, 2024

angelinalg Oct 18, 2024

angelinalg Oct 18, 2024

angelinalg Oct 18, 2024

angelinalg Oct 18, 2024

angelinalg Oct 18, 2024

angelinalg Oct 18, 2024

[Core] Resubmit #47871 & #48030 and Fix the Failed Test for Windows #48054

[Core] Resubmit #47871 & #48030 and Fix the Failed Test for Windows #48054

Conversation

MengjinYan commented Oct 16, 2024 • edited Loading

Why are these changes needed?

Related issue number

Checks

aslonnie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MengjinYan commented Oct 17, 2024 • edited Loading

rynewang commented Oct 18, 2024

MengjinYan commented Oct 18, 2024

MengjinYan commented Oct 18, 2024

angelinalg 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

MengjinYan commented Oct 16, 2024 •

edited

Loading

MengjinYan commented Oct 17, 2024 •

edited

Loading