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

[RLlib] Clean up logging configuration #35690

Conversation

peytondmurray
Copy link
Contributor

@peytondmurray peytondmurray commented May 23, 2023

Why are these changes needed?

At the suggestion of @aslonnie I've broken up the remaining logging configuration cleanup into a few separate PRs to make it a little easier to merge.

In this PR:

  • Many places where logging.basicConfig was called have been removed, and we now use the existing log config defined in ray._private.log. The existing ray logging configuration already has the log level for the ray.rllib logger set to logging.WARN, so there shouldn't be much difference in logging behavior from the user perspective.
  • Setting the log level on an AlgorithmConfig instance or in AlgorithmConfig.debugging is now deprecated.
  • Removed two instances where the root logger was retrieved but unused.

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

@peytondmurray peytondmurray requested a review from rkooo567 May 23, 2023 23:45
@peytondmurray peytondmurray changed the title [WIP] [RLlib] Clean up logging configuration [RLlib] Clean up logging configuration May 26, 2023
@peytondmurray peytondmurray marked this pull request as ready for review May 26, 2023 20:24
@peytondmurray peytondmurray force-pushed the logging-config-cleanup-rllib branch from ccf72df to 445e5eb Compare June 5, 2023 22:37
@xieus
Copy link

xieus commented Jun 21, 2023

@kouroshHakha @sven1977 Could you pls take a look at this PR?

Copy link
Contributor

@kouroshHakha kouroshHakha left a comment

Choose a reason for hiding this comment

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

@peytondmurray Can you clarify how would someone change the logging level in RLlib on distributed rollout workers then?

@peytondmurray
Copy link
Contributor Author

Can you clarify how would someone change the logging level in RLlib on distributed rollout workers then?

Sure! Part of the goal of the logging refactor effort was to make logging configuration follow standard python logging protocol more closely. The benefit of this is that if you're a user and you want to change the logging level for just yourself, the python docs cover how to do this without needing to have any ray-specific knowledge. But to answer your question, it depends on if you're asking from a user perspective or a ray developer perspective. If you're asking

As a user I want more logging information from RLlib. How do I configure that?

Then you should just use standard python logging approaches to modify the config. For example, by default ray.rllib only logs messages at logging.WARN level or above. If you want more info than that:

import ray
import logging

logging.getLogger('ray.rllib').setLevel(logging.INFO) # Or some other logging level

# rest of your code goes here

This will override the default ray logging configuration. We also have documentation about this if you're interested. If on the other hand your question is

If I'm a Ray developer and I want to change the default RLlib logging level for the entire project, how do I do that?

then you would modify the part of the call to dictConfig in ray/python/ray/_private/log.py that sets the log level.. I hope that explanation makes sense, but please let me know if we need more documentation about this.

@stale
Copy link

stale bot commented Aug 10, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Aug 10, 2023
Signed-off-by: pdmurray <peynmurray@gmail.com>
@peytondmurray peytondmurray force-pushed the logging-config-cleanup-rllib branch from 445e5eb to 6a79dbe Compare August 17, 2023 21:37
@stale stale bot removed the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Aug 17, 2023
@stale
Copy link

stale bot commented Sep 16, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

  • If you'd like to keep this open, just leave any comment, and the stale label will be removed.

@stale stale bot added the stale The issue is stale. It will be closed within 7 days unless there are further conversation label Sep 16, 2023
@stale
Copy link

stale bot commented Oct 15, 2023

Hi again! The issue will be closed because there has been no more activity in the 14 days since the last message.

Please feel free to reopen or open a new issue if you'd still like it to be addressed.

Again, you can always ask for help on our discussion forum or Ray's public slack channel.

Thanks again for opening the issue!

@stale stale bot closed this Oct 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale The issue is stale. It will be closed within 7 days unless there are further conversation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants