[Ray Core] Fix the Paths in the Generated Monitoring Configs to Consider temp-dir #47871

MengjinYan · 2024-09-30T21:13:48Z

Why are these changes needed?

Today, when a user specifies the --temp-dir when starting ray, the paths in the default config files generated are still with the default root directory /tmp/ray. The issue is because the default config files are directly copied from the package when ray starts and the paths are all hardcoded.

This PR fix the issue by generating the corresponding config files with the corresponding path at runtime.

Also:

keep using the default path with the hard coded file in the ray metrics launch-prometheus command because the command can be executed with/without ray cluster started. The doc is updated accordingly to suggest the user to use manual mode to start prometheus is --temp-dir will be specified
Updated the tests to:
- add verification of the monitoring config generation for the case of specify temp-dir
- add verification of the paths in the config content

Related issue number

Closes #41648

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

… temp-dir Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

doc/source/cluster/metrics.md

2. fix comment 3. Update the logic in the install_and_start_prometheus to continue leverage the hard coded file, in order to avoid generating a file in user's package or working directory. Tests added accordingly Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

rynewang · 2024-10-04T00:43:35Z

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

-        # in install_and_start_prometheus.py is updated to reload the config file.
-        shutil.copy(PROMETHEUS_CONFIG_INPUT_PATH, prometheus_config_output_path)
+
+        # The code here is slightly different from how the prometheus config is


I am not sure I fully understand this comment section. What about:

# This code generates the Prometheus config based on the custom temporary root path # set by the user at Ray cluster start up (via --temp-dir). In contrast, start_prometheus in # install_and_start_prometheus.py uses a hardcoded Prometheus config at # PROMETHEUS_CONFIG_INPUT_PATH that always uses "/tmp/ray". Other than the root path, # the config file generated here is identical to that hardcoded config file.

rynewang · 2024-10-04T03:08:37Z

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

-    # However, if in the future Ray ever modifies this file at runtime, we'll
-    # need to use the user-friendly location instead, and reload the config
-    # file after it's updated by Ray.
+    # The function assumes, to-be-prometheus-monitored ray cluster uses the default


Nit:

# This function assumes the Ray cluster to be monitored by Prometheus uses the default # configuration with "/tmp/ray" as the 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 to # use a hardcoded default value.

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

rynewang · 2024-10-07T22:22:06Z

@angelinalg please take a look at this - Mengjin fixed an issue about when users specify --temp-dir, the default grafana config no longer works. We added docs for those people about how to config their grafana.

angelinalg

some style nits and a typo. For future reference, please run Vale to get style feedback: https://docs.ray.io/en/latest/ray-contribute/docs.html#how-to-use-vale

angelinalg · 2024-10-15T18:56:45Z

doc/source/cluster/metrics.md

@@ -22,6 +22,12 @@ For a quick demo, you can run Prometheus locally on your machine. Follow the qui

 ### Quickstart: Running Prometheus locally

+```{admonition} Note
+:class: note
+If you need to change the root temporary directory by using "--temp-dir" in your ray 


Suggested change

If you need to change the root temporary directory by using "--temp-dir" in your ray

If you need to change the root temporary directory by using "--temp-dir" in your Ray

angelinalg · 2024-10-15T18:57:01Z

doc/source/cluster/metrics.md

+```{admonition} Note
+:class: note
+If you need to change the root temporary directory by using "--temp-dir" in your ray 
+cluster setup, please follow the [manual steps](#optional-manual-running-prometheus-locally) to setup Prometheus locally.


Suggested change

cluster setup, please follow the [manual steps](#optional-manual-running-prometheus-locally) to setup Prometheus locally.

cluster setup, follow these [manual steps](#optional-manual-running-prometheus-locally) to set up Prometheus locally.

angelinalg · 2024-10-15T18:57:34Z

doc/source/cluster/metrics.md

@@ -76,7 +82,7 @@ tar xvfz prometheus-*.tar.gz
 cd prometheus-*
 ```

-Ray provides a Prometheus config that works out of the box. After running Ray, you can find the config at `/tmp/ray/session_latest/metrics/prometheus/prometheus.yml`.
+Ray provides a Prometheus config that works out of the box. After running Ray, you can find the config at `/tmp/ray/session_latest/metrics/prometheus/prometheus.yml`. If you specify the `--temp-dir={your_temp_path}` when starting the ray cluster, the config file will be at `{yout_temp_path}/session_latest/metrics/prometheus/prometheus.yml`


Suggested change

Ray provides a Prometheus config that works out of the box. After running Ray, you can find the config at `/tmp/ray/session_latest/metrics/prometheus/prometheus.yml`. If you specify the `--temp-dir={your_temp_path}` when starting the ray cluster, the config file will be at `{yout_temp_path}/session_latest/metrics/prometheus/prometheus.yml`

Ray provides a Prometheus config that works out of the box. After running Ray, you can find the config at `/tmp/ray/session_latest/metrics/prometheus/prometheus.yml`. If you specify the `--temp-dir={your_temp_path}` when starting the Ray cluster, the config file is at: `{your_temp_path}/session_latest/metrics/prometheus/prometheus.yml`

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

## Why are these changes needed? This PR fixed the style suggestions in #47871. The original PR got accidentally merged before the suggestions are fixed.  ## Related issue number  ## Checks - [x] 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 :( --------- Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com> Co-authored-by: angelinalg <122562471+angelinalg@users.noreply.github.com>

This reverts commit 35d95a7.

Reverts #48030

…to Consider temp-dir" (#48050) Reverts #47871

… Configs to Consider temp-dir Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

…48054) * Resubmit #47871 and #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 Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

…t#48049) Reverts ray-project#48030

…to Consider temp-dir" (ray-project#48050) Reverts ray-project#47871

…led Test for Windows (ray-project#48054) * Resubmit ray-project#47871 and ray-project#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 Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

…led Test for Windows (ray-project#48054) * Resubmit ray-project#47871 and ray-project#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 Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com> Signed-off-by: mohitjain2504 <mohit.jain@dream11.com>

MengjinYan added 3 commits September 27, 2024 18:19

Fix the path in the default grafana/prometheus config to consider the…

943df20

… temp-dir Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

Remove the merged files

d1381b9

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

fix lint issue

8681c8c

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

MengjinYan requested review from architkulkarni, maxpumperla, pcmoritz, kevin85421 and a team as code owners September 30, 2024 21:13

MengjinYan changed the title ~~Issue-41648: Fix the Paths in the Generated Monitoring Configs to Consider temp-dir~~ [Ray Core] Fix the Paths in the Generated Monitoring Configs to Consider temp-dir Sep 30, 2024

MengjinYan requested a review from rynewang September 30, 2024 21:14

rynewang reviewed Sep 30, 2024

View reviewed changes

doc/source/cluster/metrics.md Outdated Show resolved Hide resolved

doc/source/cluster/metrics.md Outdated Show resolved Hide resolved

MengjinYan added 3 commits September 30, 2024 15:45

1. fix lint

3730c8e

2. fix comment 3. Update the logic in the install_and_start_prometheus to continue leverage the hard coded file, in order to avoid generating a file in user's package or working directory. Tests added accordingly Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

update comment in metrics_head.py

4d2f6d6

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

fix linter error

84c7ae4

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

MengjinYan assigned rynewang Oct 3, 2024

rynewang approved these changes Oct 4, 2024

View reviewed changes

Fix comment

49e8487

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

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

Merge branch 'master' into issue-41648

3bd284d

rynewang enabled auto-merge (squash) October 7, 2024 22:20

rynewang assigned angelinalg Oct 7, 2024

angelinalg approved these changes Oct 15, 2024

View reviewed changes

rynewang merged commit f04ddd6 into master Oct 15, 2024
6 checks passed

rynewang deleted the issue-41648 branch October 15, 2024 18:58

MengjinYan added a commit that referenced this pull request Oct 15, 2024

Fix style changes suggested in #47871

56356de

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

MengjinYan mentioned this pull request Oct 15, 2024

Fix Style Changes Suggested in #47871 #48030

Merged

8 tasks

can-anyscale added a commit that referenced this pull request Oct 16, 2024

Revert "Fix Style Changes Suggested in #47871 (#48030)"

0057654

This reverts commit 35d95a7.

can-anyscale added a commit that referenced this pull request Oct 16, 2024

Revert "Fix Style Changes Suggested in #47871" (#48049)

37e2449

Reverts #48030

can-anyscale mentioned this pull request Oct 16, 2024

Revert "[Ray Core] Fix the Paths in the Generated Monitoring Configs to Consider temp-dir" #48050

Merged

can-anyscale added a commit that referenced this pull request Oct 16, 2024

Revert "[Ray Core] Fix the Paths in the Generated Monitoring Configs …

8bd6a9a

…to Consider temp-dir" (#48050) Reverts #47871

MengjinYan added a commit that referenced this pull request Oct 16, 2024

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

1740d4f

… Configs to Consider temp-dir Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

MengjinYan added a commit that referenced this pull request Oct 16, 2024

Resubmit: #48030 Fix Style Changes Suggested in #47871

2561ac8

Signed-off-by: Mengjin Yan <mengjinyan3@gmail.com>

MengjinYan mentioned this pull request Oct 16, 2024

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

Merged

8 tasks

Jay-ju pushed a commit to Jay-ju/ray that referenced this pull request Nov 5, 2024

Revert "Fix Style Changes Suggested in ray-project#47871" (ray-projec…

90607a9

…t#48049) Reverts ray-project#48030

Jay-ju pushed a commit to Jay-ju/ray that referenced this pull request Nov 5, 2024

Revert "[Ray Core] Fix the Paths in the Generated Monitoring Configs …

47cfb18

…to Consider temp-dir" (ray-project#48050) Reverts ray-project#47871

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Ray Core] Fix the Paths in the Generated Monitoring Configs to Consider temp-dir #47871

[Ray Core] Fix the Paths in the Generated Monitoring Configs to Consider temp-dir #47871

MengjinYan commented Sep 30, 2024 •

edited

Loading

rynewang Oct 4, 2024

rynewang Oct 4, 2024

rynewang commented Oct 7, 2024

angelinalg left a comment

angelinalg Oct 15, 2024

angelinalg Oct 15, 2024

angelinalg Oct 15, 2024

	If you need to change the root temporary directory by using "--temp-dir" in your ray
	If you need to change the root temporary directory by using "--temp-dir" in your Ray

	cluster setup, please follow the [manual steps](#optional-manual-running-prometheus-locally) to setup Prometheus locally.
	cluster setup, follow these [manual steps](#optional-manual-running-prometheus-locally) to set up Prometheus locally.

	Ray provides a Prometheus config that works out of the box. After running Ray, you can find the config at `/tmp/ray/session_latest/metrics/prometheus/prometheus.yml`. If you specify the `--temp-dir={your_temp_path}` when starting the ray cluster, the config file will be at `{yout_temp_path}/session_latest/metrics/prometheus/prometheus.yml`
	Ray provides a Prometheus config that works out of the box. After running Ray, you can find the config at `/tmp/ray/session_latest/metrics/prometheus/prometheus.yml`. If you specify the `--temp-dir={your_temp_path}` when starting the Ray cluster, the config file is at: `{your_temp_path}/session_latest/metrics/prometheus/prometheus.yml`

[Ray Core] Fix the Paths in the Generated Monitoring Configs to Consider temp-dir #47871

[Ray Core] Fix the Paths in the Generated Monitoring Configs to Consider temp-dir #47871

Conversation

MengjinYan commented Sep 30, 2024 • edited Loading

Why are these changes needed?

Related issue number

Checks

rynewang Oct 4, 2024

Choose a reason for hiding this comment

rynewang Oct 4, 2024

Choose a reason for hiding this comment

rynewang commented Oct 7, 2024

angelinalg left a comment

Choose a reason for hiding this comment

angelinalg Oct 15, 2024

Choose a reason for hiding this comment

angelinalg Oct 15, 2024

Choose a reason for hiding this comment

angelinalg Oct 15, 2024

Choose a reason for hiding this comment

MengjinYan commented Sep 30, 2024 •

edited

Loading