-
Notifications
You must be signed in to change notification settings - Fork 6k
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
Turn off color logging prefix #45218
Conversation
01d060a
to
646384d
Compare
@@ -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`` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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]>
Signed-off-by: mliu <[email protected]>
50dd7cf
to
df98e7d
Compare
Signed-off-by: mliu <[email protected]>
Signed-off-by: mliu <[email protected]>
Please advise on failing rllib test, appears to be unrelated. |
@jjyao could you get some one from core team to review? @maxliuofficial maybe sync your branch with latest master head. |
There was a problem hiding this 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!
Signed-off-by: mliu <[email protected]>
Signed-off-by: mliu <[email protected]>
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](https://private-user-images.githubusercontent.com/48189521/329164920-7fca5791-06cf-4a26-be11-1c2c31df952f.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk2NDg5MDgsIm5iZiI6MTczOTY0ODYwOCwicGF0aCI6Ii80ODE4OTUyMS8zMjkxNjQ5MjAtN2ZjYTU3OTEtMDZjZi00YTI2LWJlMTEtMWMyYzMxZGY5NTJmLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNTAyMTUlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjUwMjE1VDE5NDMyOFomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWFkNjBkMzZjNjczMDkwMmExZGZkMGIyZjBmNDYzOGJhNjdlZDRjNGYxZDdlZjU5MWNiY2YwMmQyNGRmMWVhMzImWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0In0.Yk4Va4lRGD9rKbRdLhbwRR2ruAq2L7av_X2XECIKlEU)
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
git commit -s
) in this PR.scripts/format.sh
to lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/
under thecorresponding
.rst
file.