Update logging config #30863

peytondmurray · 2022-12-02T19:42:41Z

Why are these changes needed?

This PR simplifies some logging configuration in accordance with extended discussions with @xwjiang2010 and @rkooo567. Changes:

Avoid an unnecessary binding the 'filelock' logger to a variable in python/ray/autoscaler/_private/local/node_provider.py to discourage use within ray
Made a note in logservicer.py (mostly for myself) that the logging configuration there is not to be messed with because it runs in a separate thread
Log to a module logger release/ray_release/logger.py, rllib/examples/sumo_env_local.py, and rllib/examples/tune/framework.py, python/ray/_private/runtime_env/_clonevirtualenv.py
Remove custom logging configuration in rllib and experimental/raysort/main.py; we will use the global ray logging configuration here moving forward
Logging configuration in rllib can no longer be set from AlgorithmConfig; use standard logging configuration routes going forward
Added an explanation of logging configuration in kuberay

Anything that runs in a separate process was left unmodified, as per the discussion here: #30005.

Related issue number

Partly addresses #30005.

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

rkooo567

For rllib changes, I think we need rllib folks approval. cc @sven1977 who's the best person who can take a look at this PR?

python/ray/tests/kuberay/utils.py

python/ray/autoscaler/_private/kuberay/run_autoscaler.py

DmitriGekhtman · 2022-12-09T22:34:32Z

python/ray/autoscaler/_private/kuberay/run_autoscaler.py

+    # The logging configuration here is for the sidecar container, but we need the
+    # logs to go to the same place as the head node logs because the autoscaler
+    # is allowed to communicate with another Ray process (the log monitor) via logs
+    # in that directory. However, the Ray head container sets up the log directory.


Could you split this sentence up?

Definitely. I've split it up and reworded it - what do you think now?

Looks good, thanks!

rkooo567

LGTM though we still need approval from the rllib. I will ping one of guys there

rkooo567 · 2022-12-13T07:39:37Z

can you also merge the latest master?

rkooo567 · 2022-12-13T07:39:52Z

cc @gjoliver to take a look

sven1977

Thanks for this cleanup @peytondmurray ! One more thing for RLlib. Since we don't use the AlgorithmConfig.log_level setting anymore, we should properly deprecate it.

In rllib/algorithms/algorithm_config.py, can you:

self.log_level = "WARN" -> self.log_level = DERPECATED_VALUE

...
def debugging()
...
        log_level: Optional[str] = NotProvided, -> log_level=DEPRECATED_VALUE,

...
    # and further down in that same method:
    if log_level != DERECATED_VALUE:
        deprecation_warning(old="config.log_level", help="[describe here, how RLlib users should set their log levels, instead]", error=False)

Thanks so much!

peytondmurray · 2022-12-15T05:54:24Z

Thanks @sven1977, I just pushed your requested changes. If the tests pass 🤞 I think we'll be good to merge.

rkooo567 · 2022-12-15T10:58:13Z

@sven1977 we need your approval for the merge! Thank you!

Signed-off-by: pdmurray <peynmurray@gmail.com>

rkooo567 · 2022-12-19T09:20:45Z

cc @gjoliver @sven1977

sven1977

LGTM. Thanks for this PR @peytondmurray !

This reverts commit 608276b.

This reverts commit 608276b. Looks like this breaks the backward compatibility of rllib (it is supposed to print warn first, but it prints the info log).

Signed-off-by: tmynn <hovhannes.tamoyan@gmail.com>

Attempts to consolidate logging configuration by introducing reasonable defaults in ray/log.py. This new logging configuration is done once in ray/__init__.py at the top of the module. Subsequent calls to the configuration are ignored. A logger for ray.rllib is configured at the WARN level, to address Revert "Simplify logging configuration. (#30863)" #31858. With this change, Revert "Simplify logging configuration. (#30863)" #31858 can be reverted, again simplifying and consolidating logging configuration. Modified test_output.py::test_logger_config to test only the logger config, not launch a ray cluster. The test was failing intermittently, I think due to a race condition between the launch of the cluster and the reading of the subprocess's stdout, and anyway it wasn't necessary to call ray.init here to check that logging was configured correctly. Modified python/ray/tune/tests/test_commands.py::test_ls_with_cfg to test the underlying data, not what gets printed to stdout (which has changed with the new logging system). Modified a logging message in ray.tune.automl.search_policy.AutoMLSearcher.on_trial_complete, which in certain cases emits a logging message which tries to format a NoneType into a %f during log message formatting. This was a previously-undetected bug which showed up because the default log level is now INFO. This fixes a test that was failing in test_automl_searcher.py::AutoMLSearcherTest.

…ay-project#34182) Attempts to consolidate logging configuration by introducing reasonable defaults in ray/log.py. This new logging configuration is done once in ray/__init__.py at the top of the module. Subsequent calls to the configuration are ignored. A logger for ray.rllib is configured at the WARN level, to address Revert "Simplify logging configuration. (ray-project#30863)" ray-project#31858. With this change, Revert "Simplify logging configuration. (ray-project#30863)" ray-project#31858 can be reverted, again simplifying and consolidating logging configuration. Modified test_output.py::test_logger_config to test only the logger config, not launch a ray cluster. The test was failing intermittently, I think due to a race condition between the launch of the cluster and the reading of the subprocess's stdout, and anyway it wasn't necessary to call ray.init here to check that logging was configured correctly. Modified python/ray/tune/tests/test_commands.py::test_ls_with_cfg to test the underlying data, not what gets printed to stdout (which has changed with the new logging system). Modified a logging message in ray.tune.automl.search_policy.AutoMLSearcher.on_trial_complete, which in certain cases emits a logging message which tries to format a NoneType into a %f during log message formatting. This was a previously-undetected bug which showed up because the default log level is now INFO. This fixes a test that was failing in test_automl_searcher.py::AutoMLSearcherTest.

…ay-project#34182) Attempts to consolidate logging configuration by introducing reasonable defaults in ray/log.py. This new logging configuration is done once in ray/__init__.py at the top of the module. Subsequent calls to the configuration are ignored. A logger for ray.rllib is configured at the WARN level, to address Revert "Simplify logging configuration. (ray-project#30863)" ray-project#31858. With this change, Revert "Simplify logging configuration. (ray-project#30863)" ray-project#31858 can be reverted, again simplifying and consolidating logging configuration. Modified test_output.py::test_logger_config to test only the logger config, not launch a ray cluster. The test was failing intermittently, I think due to a race condition between the launch of the cluster and the reading of the subprocess's stdout, and anyway it wasn't necessary to call ray.init here to check that logging was configured correctly. Modified python/ray/tune/tests/test_commands.py::test_ls_with_cfg to test the underlying data, not what gets printed to stdout (which has changed with the new logging system). Modified a logging message in ray.tune.automl.search_policy.AutoMLSearcher.on_trial_complete, which in certain cases emits a logging message which tries to format a NoneType into a %f during log message formatting. This was a previously-undetected bug which showed up because the default log level is now INFO. This fixes a test that was failing in test_automl_searcher.py::AutoMLSearcherTest. Signed-off-by: e428265 <arvind.chandramouli@lmco.com>

peytondmurray force-pushed the update-logging-config branch 2 times, most recently from 0ccb25f to 307167a Compare December 5, 2022 18:42

peytondmurray changed the title ~~[WIP] Update logging config~~ Update logging config Dec 5, 2022

peytondmurray marked this pull request as ready for review December 5, 2022 18:46

peytondmurray requested review from krfricke, rkooo567, simon-mo, sven1977, gjoliver, avnishn, ArturNiederfahrenhorst, smorad, maxpumperla, kouroshHakha, wuisawesome, DmitriGekhtman and ericl as code owners December 5, 2022 18:46

rkooo567 self-assigned this Dec 5, 2022

peytondmurray requested a review from xwjiang2010 December 5, 2022 23:29

rkooo567 reviewed Dec 9, 2022

View reviewed changes

python/ray/tests/kuberay/utils.py Show resolved Hide resolved

python/ray/autoscaler/_private/kuberay/run_autoscaler.py Show resolved Hide resolved

peytondmurray force-pushed the update-logging-config branch from 307167a to d529f4f Compare December 9, 2022 22:22

DmitriGekhtman reviewed Dec 9, 2022

View reviewed changes

peytondmurray force-pushed the update-logging-config branch from d529f4f to 41794cf Compare December 13, 2022 01:55

DmitriGekhtman approved these changes Dec 13, 2022

View reviewed changes

rkooo567 approved these changes Dec 13, 2022

View reviewed changes

sven1977 reviewed Dec 14, 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 Dec 15, 2022

peytondmurray force-pushed the update-logging-config branch from 41794cf to 075f04c Compare December 15, 2022 05:53

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

Simplify logging configuration

930d6d1

Signed-off-by: pdmurray <peynmurray@gmail.com>

peytondmurray force-pushed the update-logging-config branch from 075f04c to 930d6d1 Compare December 15, 2022 17:00

sven1977 approved these changes Dec 19, 2022

View reviewed changes

sven1977 merged commit 608276b into ray-project:master Dec 19, 2022

peytondmurray deleted the update-logging-config branch December 19, 2022 23:21

rickyyx mentioned this pull request Dec 23, 2022

[core][nightly] many_actors regression #31284

Closed

AmeerHajAli pushed a commit that referenced this pull request Jan 12, 2023

Simplify logging configuration. (#30863)

efe275e

rkooo567 added a commit to rkooo567/ray that referenced this pull request Jan 23, 2023

Revert "Simplify logging configuration. (ray-project#30863)"

a93a05a

This reverts commit 608276b.

rkooo567 added a commit that referenced this pull request Jan 23, 2023

Revert "Simplify logging configuration. (#30863)" (#31858)

0c69020

This reverts commit 608276b. Looks like this breaks the backward compatibility of rllib (it is supposed to print warn first, but it prints the info log).

rkooo567 added a commit that referenced this pull request Jan 24, 2023

Revert "Simplify logging configuration. (#30863)" (#31858)

fbdaa23

This reverts commit 608276b. Looks like this breaks the backward compatibility of rllib (it is supposed to print warn first, but it prints the info log).

tamohannes pushed a commit to ju2ez/ray that referenced this pull request Jan 25, 2023

Simplify logging configuration. (ray-project#30863)

ed0aec2

Signed-off-by: tmynn <hovhannes.tamoyan@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update logging config #30863

Update logging config #30863

peytondmurray commented Dec 2, 2022 •

edited

Loading

rkooo567 left a comment

DmitriGekhtman Dec 9, 2022

peytondmurray Dec 13, 2022

DmitriGekhtman Dec 13, 2022

rkooo567 left a comment

rkooo567 commented Dec 13, 2022

rkooo567 commented Dec 13, 2022

sven1977 left a comment

peytondmurray commented Dec 15, 2022

rkooo567 commented Dec 15, 2022

rkooo567 commented Dec 19, 2022

sven1977 left a comment

Update logging config #30863

Update logging config #30863

Conversation

peytondmurray commented Dec 2, 2022 • edited Loading

Why are these changes needed?

Related issue number

Checks

rkooo567 left a comment

Choose a reason for hiding this comment

DmitriGekhtman Dec 9, 2022

Choose a reason for hiding this comment

peytondmurray Dec 13, 2022

Choose a reason for hiding this comment

DmitriGekhtman Dec 13, 2022

Choose a reason for hiding this comment

rkooo567 left a comment

Choose a reason for hiding this comment

rkooo567 commented Dec 13, 2022

rkooo567 commented Dec 13, 2022

sven1977 left a comment

Choose a reason for hiding this comment

peytondmurray commented Dec 15, 2022

rkooo567 commented Dec 15, 2022

rkooo567 commented Dec 19, 2022

sven1977 left a comment

Choose a reason for hiding this comment

peytondmurray commented Dec 2, 2022 •

edited

Loading