-
Notifications
You must be signed in to change notification settings - Fork 174
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
chore: Build progress bar only on first update #3626
Conversation
CodSpeed Performance ReportMerging #3626 will improve performances by 89.78%Comparing Summary
Benchmarks breakdown
|
caedb22
to
4ba1200
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3626 +/- ##
==========================================
+ Coverage 77.80% 77.90% +0.09%
==========================================
Files 718 720 +2
Lines 88250 88726 +476
==========================================
+ Hits 68666 69122 +456
- Misses 19584 19604 +20
|
daft/runners/progress_bar.py
Outdated
bar_format, initial_message = self.bar_configs[pbar_id] | ||
self.pbars[pbar_id] = self.tqdm_mod( | ||
bar_format=bar_format, | ||
desc=initial_message, |
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.
Seems like initial_message isn't really used? It gets added for a brief moment during the first update, but is superceded shortly after.
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.
nice catch
Theres 2 progress bars for swordfish, a rust based one thats for terminals, and a python one thats for jupyter notebooks (uses tqdm).
This pr makes it so that the tqdm bar is only rendered on first update (the rust based one is already lazy rendered), + increase the refresh rate of both progress bars to 500ms.
native
native.mov
py
py.mov