Skip to content

Fix RichProgressBar visibility #17532

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.

Sign up for GitHub

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

Closed
wants to merge 11 commits into from
Closed

Fix RichProgressBar visibility #17532

Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f5232fc
updated the RichProgressBarTheme class for different terminals
ashikshafi08 Apr 30, 2023
d1f86d7
updated the RichProgressBarTheme class for different terminals
ashikshafi08 Apr 30, 2023
f67827c
updated the RichProgressBarTheme class for different terminals
ashikshafi08 Apr 30, 2023
87005de
updated the RichProgressBarTheme class for different terminals
ashikshafi08 Apr 30, 2023
a980032
updated the RichProgressBarTheme class for different terminals
ashikshafi08 Apr 30, 2023
4833118
updated the RichProgressBarTheme class for different terminals
ashikshafi08 Apr 30, 2023
5785548
updated the RichProgressBarTheme class for different terminals
ashikshafi08 Apr 30, 2023
056300a
updated the RichProgressBarTheme class for different terminals
ashikshafi08 Apr 30, 2023
a89bf5d
updated the RichProgressBarTheme class for different terminals
ashikshafi08 Apr 30, 2023
02f35ca
Merge branch 'Lightning-AI:master' into master
ashikshafi08 Apr 30, 2023
e64c3b5
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Apr 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 37 additions & 8 deletions src/lightning/pytorch/callbacks/progress/rich_progress.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,14 +193,43 @@ class RichProgressBarTheme:
https://rich.readthedocs.io/en/stable/style.html
"""

description: Union[str, Style] = "white"
progress_bar: Union[str, Style] = "#6206E0"
progress_bar_finished: Union[str, Style] = "#6206E0"
progress_bar_pulse: Union[str, Style] = "#6206E0"
batch_progress: Union[str, Style] = "white"
time: Union[str, Style] = "grey54"
processing_speed: Union[str, Style] = "grey70"
metrics: Union[str, Style] = "white"
@staticmethod
def detect_terminal_bg() -> str:
console = Console()
if console.color_system == "truecolor":
# Check if the terminal background color environment variable is set
bg_color = os.environ.get("TERMINAL_BG_COLOR", "dark").lower()
Copy link
Contributor

@tshu-w tshu-w Apr 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I want to know if this environment variable is set by the Terminal. My color system is 256 and I don't have this environment variable.

My idea is whether it's possible to have one solution that can adapt to two backgrounds, for example, not setting the colors of default_description and default_batch_progress, will they be consistent with the default font color in different backgrounds, i.e., black in light theme and white in dark theme?

Copy link
Author

@ashikshafi08 ashikshafi08 Apr 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

But it may not be perfect in all cases, right? Especially if the user has a custom terminal theme with non-standard colors.

Also, what if we create a dictionary to store the default colors for each supported color system, so it makes it easy to add or modify the color settings accordingly?

Something like this,


from dataclasses import dataclass
from typing import Union
from rich.console import Console
from rich.style import Style


@dataclass
class RichProgressBarTheme:

    @staticmethod
    def detect_color_system() -> str:
        console = Console()
        return console.color_system

    color_system = detect_color_system.__func__()

    # Default colors for each color system
    default_colors = {
        "truecolor": {
            "progress_bar": "bright_blue",
            "time": "bright_cyan",
            "processing_speed": "bright_yellow",
        },
        "256": {
            "progress_bar": "color51",
            "time": "color45",
            "processing_speed": "color227",
        },
        "default": {
            "progress_bar": "blue",
            "time": "cyan",
            "processing_speed": "yellow",
        },
    }

    # Apply specific colors based on the detected color system
    colors = default_colors.get(color_system, default_colors["default"])

    description: Union[str, Style] = "default"
    progress_bar: Union[str, Style] = colors["progress_bar"]
    progress_bar_finished: Union[str, Style] = colors["progress_bar"]
    progress_bar_pulse: Union[str, Style] = colors["progress_bar"]
    batch_progress: Union[str, Style] = "default"
    time: Union[str, Style] = colors["time"]
    processing_speed: Union[str, Style] = colors["processing_speed"]
    metrics: Union[str, Style] = "default"

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, what I mean is to use the default style for description and batch_progress, which can avoid the problem of being invisible in light themes as mentioned in issue #17532.

But it may not be perfect in all cases, right? Especially if the user has a custom terminal theme with non-standard colors.

As I mentioned in #17532, from my perspective, the first 16 standard colors of most themes are carefully selected and suitable for the theme. So my idea is to only use them.

Try to use the first 16 standard colors combined with ansi styles in RichProgressBarTheme, which are usually defined by the terminal theme.

Copy link
Author

Choose a reason for hiding this comment

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

So the above code works?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so :), although I haven't tested it.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks!
I will update the code and run some test cases, then make a PR again.

return bg_color
else:
# If the terminal does not support truecolor, assume it is dark theme
return "dark"

terminal_bg = detect_terminal_bg.__func__()

if terminal_bg == "light":
default_description = "black"
default_progress_bar = "blue"
default_batch_progress = "black"
default_time = "grey20"
default_processing_speed = "grey40"
default_metrics = "black"

else:
default_description = "bright_white"
default_progress_bar = "bright_blue"
default_batch_progress = "bright_white"
default_time = "bright_cyan"
default_processing_speed = "bright_yellow"
default_metrics = "bright_white"

description: Union[str, Style] = default_description
progress_bar: Union[str, Style] = default_progress_bar
progress_bar_finished: Union[str, Style] = default_progress_bar
progress_bar_pulse: Union[str, Style] = default_progress_bar
batch_progress: Union[str, Style] = default_batch_progress
time: Union[str, Style] = default_time
processing_speed: Union[str, Style] = default_processing_speed
metrics: Union[str, Style] = default_metrics


class RichProgressBar(ProgressBar):
Expand Down