Skip to content

Conversation

ashikshafi08
Copy link

@ashikshafi08 ashikshafi08 commented Apr 30, 2023

What does this PR do?

Implemented RichProgressBarTheme with a new feature to detect the terminal background color and adjust the styles based on it. The terminal background color is detected using the Console class from the rich library. If the terminal supports truecolor, the TERMINAL_BG_COLOR environment variable is checked, otherwise, it is assumed to be a dark theme.

This implementation allows the RichProgressBar to be visible by default in both light and dark themes without requiring users to modify the theme.

Changes Made

  • Use the first 16 standard colors combined with ANSI styles in RichProgressBarTheme to ensure that the RichProgressBar is visible by default in both light and dark themes.
  • Modify the RichProgressBarTheme class to include a method detect_terminal_bg() that detects the background color of the terminal and sets the default theme colors accordingly.

Note: This is my first pull request in a big code base, if I did something wrong please do let me know thats why I made a draft pull request instead of the actual one. Thanks for your time!.

Fixes #17118

Before submitting
  • Was this discussed/agreed via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:

Reviewer checklist
  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

@github-actions github-actions bot added the pl Generic label for PyTorch Lightning package label Apr 30, 2023
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

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

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.

@stale
Copy link

stale bot commented May 21, 2023

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://lightning.ai/docs/pytorch/latest/generated/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Discord. Thank you for your contributions.

@stale stale bot added the won't fix This will not be worked on label May 21, 2023
@stale
Copy link

stale bot commented Jun 18, 2023

This pull request is going to be closed. Please feel free to reopen it or create a new one based on top of the 'master' branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pl Generic label for PyTorch Lightning package won't fix This will not be worked on

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make RichProgressBar visible for both light and dark theme by default

2 participants