Skip to content

Remove redundant _color_supported() check.#6363

Merged
sawa3030 merged 6 commits intooptuna:masterfrom
gen740:remove_redundant_no_color_check
Dec 11, 2025
Merged

Remove redundant _color_supported() check.#6363
sawa3030 merged 6 commits intooptuna:masterfrom
gen740:remove_redundant_no_color_check

Conversation

@gen740
Copy link
Copy Markdown
Member

@gen740 gen740 commented Nov 28, 2025

Motivation

The logging module colorlog already supports the NO_COLOR environment variable since https://github.com/borntyping/python-colorlog/releases/tag/v6.0.0-alpha.2. Therefore, checking and switching the Formatter in Optuna code is unnecessary.

Additionally, colorlog supports the FORCE_COLOR option, which forces colored outputs regardless of the environment. This is particularly useful when running Optuna in GitHub CI or other environments that don’t have a tty device but support colored outputs. However, in the current code, when the user provides the FORCE_COLOR environment variable, Optuna ignores it, resulting in colorless output.

Description of the changes

  • Remove the _color_supported() function since the colorlog module itself can handle it.

@gen740 gen740 closed this Nov 28, 2025
@gen740 gen740 reopened this Nov 28, 2025
@gen740
Copy link
Copy Markdown
Member Author

gen740 commented Nov 28, 2025

I previously submitted the pull request #4376 to support non-tty devices with no color. I also introduced the NO_COLOR and isatty logic. However, the correct approach is to specify the stream = sys.stderr to the colorlog.ColoredFormatter.

@gen740 gen740 changed the title Remove redundant NO_COLOR check. Remove redundant _color_supported() check. Nov 28, 2025
@gen740 gen740 force-pushed the remove_redundant_no_color_check branch from f797f5e to bdb146e Compare November 28, 2025 17:01
@c-bata c-bata added the code-fix Change that does not change the behavior, such as code refactoring. label Dec 5, 2025
@c-bata
Copy link
Copy Markdown
Member

c-bata commented Dec 5, 2025

@sawa3030 Could you review this PR?

Copy link
Copy Markdown
Member

@c-bata c-bata left a comment

Choose a reason for hiding this comment

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

LGTM!

@c-bata c-bata removed their assignment Dec 5, 2025
@c-bata c-bata added this to the v4.7.0 milestone Dec 5, 2025
Copy link
Copy Markdown
Collaborator

@sawa3030 sawa3030 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 have tested this change locally and confirmed it works as expected

@sawa3030 sawa3030 merged commit e6c822b into optuna:master Dec 11, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-fix Change that does not change the behavior, such as code refactoring.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants