-
Notifications
You must be signed in to change notification settings - Fork 14.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
Add progress bar + runner fixes #10348
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
f2c106a
to
2b892dd
Compare
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.
Overall looks good, some comments
from threading import Lock | ||
|
||
|
||
class ProgressBar: |
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.
Btw we have tqdm as a transitive dependency already courtesy of openai sdk
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.
Oh that's even nicer. I almost never run into scenarios where people don't have tqdm so i'll just use that, just didn't want to add another requirement
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.
Hm actually just gonna keep the print method since it works and pbar kept not erasing (i can swap out later if we want)
if verbose: | ||
try: | ||
agg_feedback = results.get_aggregate_feedback() | ||
print("\n Eval quantiles:") |
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.
Why use print for some things and logger for others?
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.
Hm mainlyformatting? default logging configs have a prefix that makes it less pretty to view. The error messaging with logging seems right for actually reporting errors (especially since i then don't have to handle threading)
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.
Can switch at some point if we want
a3daa60
to
7f28c56
Compare
Closes LS-902