Skip to content

Disable colored log when NO_COLOR env or not tty.#4376

Merged
keisuke-umezawa merged 4 commits intooptuna:masterfrom
gen740:no_colored
Feb 2, 2023
Merged

Disable colored log when NO_COLOR env or not tty.#4376
keisuke-umezawa merged 4 commits intooptuna:masterfrom
gen740:no_colored

Conversation

@gen740
Copy link
Copy Markdown
Member

@gen740 gen740 commented Jan 31, 2023

Motivation

When piping optuna's log output to the file, escape sequences are also captured.

Given the following code.

python3 -c "import optuna;study = optuna.create_study()" &> log.txt 

log.txt would be like

�[32m[I 2023-01-31 10:25:25,449]�[0m A new study created in memory with name: no-name-7c296450-89b2-45d3-9f6c-c363edf688f3�[0m

Many python tools including pip would disable coloring when stdout is not a tty, so it is more natural to do this in optuna.

And also natural to disable coloring when NO_COLOR environment variable is set, (ref. https://no-color.org).

Description of the changes

  • Created a function _color_supported to check the environment variable and tty state.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 31, 2023

Codecov Report

Merging #4376 (a3f21ec) into master (ead2e13) will decrease coverage by 0.04%.
The diff coverage is 75.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master    #4376      +/-   ##
==========================================
- Coverage   90.43%   90.40%   -0.04%     
==========================================
  Files         172      172              
  Lines       13660    13668       +8     
==========================================
+ Hits        12354    12357       +3     
- Misses       1306     1311       +5     
Impacted Files Coverage Δ
optuna/logging.py 95.65% <75.00%> (-2.71%) ⬇️
optuna/samplers/nsgaii/_crossover.py 89.70% <0.00%> (-1.48%) ⬇️
optuna/integration/botorch.py 96.73% <0.00%> (-0.82%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@HideakiImamura
Copy link
Copy Markdown
Member

@Alnusjaponica @keisuke-umezawa Could you review this PR?

@Alnusjaponica
Copy link
Copy Markdown
Contributor

LGTM

@Alnusjaponica Alnusjaponica removed their assignment Feb 2, 2023
@gen740
Copy link
Copy Markdown
Member Author

gen740 commented Feb 2, 2023

Do I need to add the test?
It's hard to test. And test/test_logger.py doesn't test a private function (that starts with _).

Copy link
Copy Markdown
Member

@keisuke-umezawa keisuke-umezawa left a comment

Choose a reason for hiding this comment

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

LGTM! I confirmed it worked in my local env.

@keisuke-umezawa keisuke-umezawa merged commit ef1779e into optuna:master Feb 2, 2023
@keisuke-umezawa keisuke-umezawa added the CI Continuous integration. label Feb 2, 2023
@keisuke-umezawa keisuke-umezawa added this to the v3.2.0 milestone Feb 2, 2023
@contramundum53
Copy link
Copy Markdown
Member

This breaks compatibility with old colorlog. #4403

gen740 added a commit to gen740/optuna that referenced this pull request Feb 8, 2023
gen740 added a commit to gen740/optuna that referenced this pull request Feb 8, 2023
gen740 added a commit to gen740/optuna that referenced this pull request Feb 8, 2023
@gen740 gen740 deleted the no_colored branch February 21, 2023 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI Continuous integration.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants