Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global logging format changes #32741

Merged

Conversation

peytondmurray
Copy link
Contributor

@peytondmurray peytondmurray commented Feb 22, 2023

Why are these changes needed?

This PR changes how the logging configuration for Ray is set, and changes the format of log messages.

After a discussion with @rkooo567 we've decided to split the logging changes into multiple PRs. This is the first in a series which makes changes to library-level logging code for Ray.

Changes

  • 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.

Related issue number

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

@peytondmurray peytondmurray force-pushed the global-logging-config-format branch 2 times, most recently from 81a3df1 to 3941de4 Compare February 22, 2023 21:18
@peytondmurray
Copy link
Contributor Author

peytondmurray commented Feb 22, 2023

Failing tests

  • Windows build and tests fail due to an issue creating a file - I'm assuming this is some permissions issue on the CI runner:
[Errno 2] No such file or directory: '::test_list_named_actors_namespace.txt'
  • There are also several test suites which fail for some other unrelated issue:
Error: Observed wheel commit () is not expected commit (3941de4b770d0b904ec720ddd42555cc200f9f7d). Aborting.

@peytondmurray peytondmurray force-pushed the global-logging-config-format branch 2 times, most recently from 9e0df01 to c564fdb Compare February 23, 2023 01:13
@peytondmurray peytondmurray changed the title [WIP] Global logging format changes Global logging format changes Feb 23, 2023
@peytondmurray peytondmurray marked this pull request as ready for review February 23, 2023 18:55
@peytondmurray peytondmurray requested a review from a team as a code owner February 23, 2023 18:55
@rkooo567 rkooo567 self-assigned this Feb 25, 2023
@rkooo567
Copy link
Contributor

Please add me to the assignee section!

@peytondmurray
Copy link
Contributor Author

☝️ Sorry about that! I saw your name in the assignee section, I should have added you to the reviewers as well!

@rkooo567 rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Feb 27, 2023
@peytondmurray peytondmurray removed the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Feb 27, 2023
@peytondmurray peytondmurray force-pushed the global-logging-config-format branch 8 times, most recently from faba24c to 487b0a3 Compare March 1, 2023 05:53
@peytondmurray
Copy link
Contributor Author

Rebased onto master to pull in the new test fixes for rllib.

Copy link
Contributor

@rkooo567 rkooo567 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add unit tests? We should make sure

  1. The context is properly printed within each library
  2. make sure the logging_level and logging_format works after this PR

I will also play with this log once I come back from OOO (next Mon)

doc/source/ray-observability/ray-logging.rst Outdated Show resolved Hide resolved
python/ray/log.py Outdated Show resolved Hide resolved
python/ray/_private/test_utils.py Outdated Show resolved Hide resolved
python/ray/log.py Outdated Show resolved Hide resolved
python/ray/log.py Outdated Show resolved Hide resolved
python/ray/log.py Outdated Show resolved Hide resolved
python/ray/log.py Outdated Show resolved Hide resolved
@rkooo567 rkooo567 added the @author-action-required The PR author is responsible for the next step. Remove tag to send back to the reviewer. label Mar 1, 2023
@peytondmurray peytondmurray force-pushed the global-logging-config-format branch 2 times, most recently from 026c746 to 8454bef Compare March 4, 2023 00:24
@peytondmurray peytondmurray force-pushed the global-logging-config-format branch 2 times, most recently from a5cbc5b to 0209f19 Compare March 28, 2023 04:41
@richardliaw richardliaw added Ray 2.5 tests-ok The tagger certifies test failures are unrelated and assumes personal liability. and removed tests-ok The tagger certifies test failures are unrelated and assumes personal liability. labels Mar 29, 2023
@krfricke krfricke removed their assignment Mar 29, 2023
@rkooo567 rkooo567 merged commit dc558d5 into ray-project:master Apr 5, 2023
@peytondmurray peytondmurray deleted the global-logging-config-format branch April 5, 2023 05:38
krfricke added a commit that referenced this pull request Apr 6, 2023
@peytondmurray peytondmurray restored the global-logging-config-format branch April 7, 2023 20:33
rkooo567 pushed a commit that referenced this pull request Apr 7, 2023
Adter manual bisection, I think this PR may be causing the "Documentation" tests to fail.

The failure was previously masked by an actual failing doctest, but after this commit, actor outputs clutter the doctests and lead to mismatches in expected and actual output.

Let's see if reverting fixes these problems.


Reverts #32741
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
This PR changes how the logging configuration for Ray is set, and changes the format of log messages.

After a discussion with @rkooo567 we've decided to split the logging changes into multiple PRs. This is the first in a series which makes changes to library-level logging code for Ray.

Signed-off-by: elliottower <[email protected]>
elliottower pushed a commit to elliottower/ray that referenced this pull request Apr 22, 2023
Adter manual bisection, I think this PR may be causing the "Documentation" tests to fail.

The failure was previously masked by an actual failing doctest, but after this commit, actor outputs clutter the doctests and lead to mismatches in expected and actual output.

Let's see if reverting fixes these problems.

Reverts ray-project#32741

Signed-off-by: elliottower <[email protected]>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
This PR changes how the logging configuration for Ray is set, and changes the format of log messages.

After a discussion with @rkooo567 we've decided to split the logging changes into multiple PRs. This is the first in a series which makes changes to library-level logging code for Ray.

Signed-off-by: Jack He <[email protected]>
ProjectsByJackHe pushed a commit to ProjectsByJackHe/ray that referenced this pull request May 4, 2023
Adter manual bisection, I think this PR may be causing the "Documentation" tests to fail.

The failure was previously masked by an actual failing doctest, but after this commit, actor outputs clutter the doctests and lead to mismatches in expected and actual output.

Let's see if reverting fixes these problems.

Reverts ray-project#32741

Signed-off-by: Jack He <[email protected]>
jjyao pushed a commit that referenced this pull request Apr 30, 2024
#32741 accidentally removes the logging_format parameter from the setup_logging function. This PR overwrites the formatters for all default handlers.

Signed-off-by: kaihsun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants