-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Improve RichProgreasBar speed performance #21321
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
base: master
Are you sure you want to change the base?
Improve RichProgreasBar speed performance #21321
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21321 +/- ##
=========================================
- Coverage 87% 79% -8%
=========================================
Files 269 266 -3
Lines 23744 23722 -22
=========================================
- Hits 20569 18703 -1866
- Misses 3175 5019 +1844 |
| with self.live._lock: | ||
| self.live.refresh() | ||
| self.refresh_cond = False | ||
| time.sleep(0.005) |
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.
Looks great so far!
One minor question: can we make this configurable? so we/users can easily reduce this even more if required?
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.
What do you think about reusing the original _RefreshThread.refresh_per_second, then exposing it as a configurable argument in the RichProgressBar callback.
One thing to note, this PR would also nullify the current refresh_rate argument, since we are now auto-refreshing the progress with refresh_per_second frequency.
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.
couldn't we use the refresh_rate argument instead then? But yeah generally something like that I had in mind :)
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.
Yes, that works. I believe this is also related to the discussion mentioned here. I agree that we should keep using refresh_rate in this case, as long as we’re okay with updating its description, since it'll now represent the updates per second rather than current update frequency in number of batches?
What does this PR do?
Attempt to fix #13937
Benchmark Results are as below
Key changes:
CustomProgress.refreshnow only calls its superclass method when running in interactive mode. This change is critical for performance since in most cases, we only need tonotifythe refresh thread rather than performing a full refresh.threading.Eventfor custom_RefreshThreadUsing
Eventwould introduce unnecessary locking overhead. Since acquiring and releasing locks are relatively expensive here, this approach didn’t yield any speed improvement.Simply enabling
auto_refreshwould revert Addrefresh_rateto RichProgressBar #10497 and bring back pdb - based debugging with RichProgressBar doesn't show input #10607, which causes pdb unusable when usingRichProgressBar.TBH, I’m not entirely sure if this is the best solution, but I haven’t found another way to improve performance without undoing prior fixes.
Given that we’re now defaulting to rich-based progress bars and model summaries when rich is installed, I think this could still be quite a beneficial improvement — happy to hear thoughts or alternative ideas.
Before submitting
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
📚 Documentation preview 📚: https://pytorch-lightning--21321.org.readthedocs.build/en/21321/