-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
feat: Add Rich Progress Bar #8929
Conversation
603ac0f
to
5a3215f
Compare
Codecov Report
@@ Coverage Diff @@
## master #8929 +/- ##
=======================================
- Coverage 93% 88% -5%
=======================================
Files 175 178 +3
Lines 14497 14651 +154
=======================================
- Hits 13444 12890 -554
- Misses 1053 1761 +708 |
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.
this looks great as a first version
for my personal taste it is too colorful, there is no meaning to the colors and I believe a simple dual-color branding would look much cleaner.
also, expect there to be a couple display issues we have to fix in the future.
That's the reason I didn't add colors to the metrics. We could define meaning to the colors and the design with one more iteration. :) We could support dual-color branding with an extra argument to the Callback. The styles and colors are configurable :D |
Agree with Adrian here, maybe we can try a few iterations on colour schemes to see what sticks! I'm hoping we can intuitively add some stats to the bar that would be neat, such as average GPU mem allocation. It might also be worth trying to compact the progress bar a bit to reduce space, but a personal preference :) |
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.
LGTM !
@kaushikb11 on naming, would we rename the existing progress bar as TQDMProgressBar? To clarify there's no code dependency between the rich one and the existing one? |
Yeah, have the following class TQDMProgressBar(ProgressBarBase):
...
# points to the default implementation
ProgressBar = TQDMProgressBar |
@ananthsub @carmocca I can rename the existing one after #8985 Also @kaushikb11 I am wondering if you forgot to add the RichProgressBar to the documentation, i.e. callbacks.rst? |
@@ -84,6 +84,7 @@ def _compare_version(package: str, op, version) -> bool: | |||
_NATIVE_AMP_AVAILABLE = _module_available("torch.cuda.amp") and hasattr(torch.cuda.amp, "autocast") | |||
_OMEGACONF_AVAILABLE = _module_available("omegaconf") | |||
_POPTORCH_AVAILABLE = _module_available("poptorch") | |||
_RICH_AVAILABLE = _module_available("rich") |
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.
@kaushikb11 - is there a minimum version of rich we should specify here that's compatible with the progress bar features?
What does this PR do?
Next PR will be to support rich Model Summary
Fixes #8255
Does your PR introduce any breaking changes? If yes, please list them.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃