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

Turn off color logging prefix #45218

Merged

Conversation

maxliuofficial
Copy link
Contributor

@maxliuofficial maxliuofficial commented May 9, 2024

Why are these changes needed?

This pr adds optionality to turn off color logging from actors. This is useful in the case where the logs are outputted to a location that does not support ansi codes, which leads to messy logs.

Similarly to multi color logging, this option uses the RAY_COLOR_PREFIX env var, this time when set to 0. Slightly differently, setting to 0 disables all color logging, even the special cases of raylet / autoscaler. This seems like the more correct approach given the use case.

I also updated the relevant docs (lmk if there are any other relevant docs to this). Here is a screenshot of the local build.
Screenshot 2024-05-09 at 12 36 34 AM

I didn't see any existing corresponding test file for this file, so I created one, and added some tests for this change. Please advise if there is a better place these tests should go, thanks!

Related issue number

Resolves #6855.

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

@maxliuofficial maxliuofficial requested a review from a team as a code owner May 9, 2024 07:37
@maxliuofficial maxliuofficial force-pushed the mliu/turn-off-color-logging-prefix branch from 01d060a to 646384d Compare May 9, 2024 07:38
@@ -95,8 +95,10 @@ The resulting output follows:

### Coloring Actor log prefixes
By default, Ray prints Actor log prefixes in light blue.
Activate multi-color prefixes by setting the environment variable ``RAY_COLOR_PREFIX=1``.
This indexes into an array of colors modulo the PID of each process.
Turn color logging off by setting the environment variable ``RAY_COLOR_PREFIX=0``
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe RAY_LOG_COLOR or something? I think it is better to make it clear that this is related to logs.

and if I would want to disable color, I would want to also disable it for the log line, not only the prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe RAY_LOG_COLOR or something? I think it is better to make it clear that this is related to logs.

I was aiming for consistency with the existing usage of RAY_COLOR_PREFIX=1 which turns on multi color. It seemed like reusing the same env var made sense. Unless the suggestion is to change both, in which case it might be necessary to have a deprecation period for users that already are setting the existing env var somewhere? Lmk ur thoughts, I am open to changing this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and if I would want to disable color, I would want to also disable it for the log line, not only the prefix.

Looks like the color wrapping only applies for the prefix based on this code. Lmk if I missed something somewhere else.

Signed-off-by: mliu <[email protected]>
Signed-off-by: mliu <[email protected]>
Signed-off-by: mliu <[email protected]>
Signed-off-by: mliu <[email protected]>
@maxliuofficial maxliuofficial force-pushed the mliu/turn-off-color-logging-prefix branch from 50dd7cf to df98e7d Compare May 11, 2024 07:33
@maxliuofficial
Copy link
Contributor Author

Please advise on failing rllib test, appears to be unrelated.

@aslonnie aslonnie requested a review from jjyao May 13, 2024 03:06
@aslonnie
Copy link
Collaborator

@jjyao could you get some one from core team to review?

@maxliuofficial maybe sync your branch with latest master head.

@jjyao jjyao self-assigned this May 13, 2024
Copy link
Contributor

@jjyao jjyao left a comment

Choose a reason for hiding this comment

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

lg, thanks for the contribution!

python/ray/_private/worker.py Outdated Show resolved Hide resolved
python/ray/tests/test_worker.py Outdated Show resolved Hide resolved
@jjyao jjyao enabled auto-merge (squash) May 22, 2024 16:32
@github-actions github-actions bot added the go Trigger full test run on premerge label May 22, 2024
@jjyao jjyao merged commit cedc2d4 into ray-project:master May 22, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
go Trigger full test run on premerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Needs flags to enable/disable printing colored log messages
4 participants