-
Notifications
You must be signed in to change notification settings - Fork 1.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
Improve the observability and readability of BootstrapFewShot optimizer #1574
base: main
Are you sure you want to change the base?
Improve the observability and readability of BootstrapFewShot optimizer #1574
Conversation
|
||
if success: | ||
bootstrapped[example_idx] = True | ||
if success: |
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.
Did we remove the check below?
if example_idx not in bootstrapped:
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.
Yea it's my bad, I thought this condition is always true, but in fact the same example can go through bootstrap more than once.
Thanks @chenmoneygithub ! Overall, I like the goals of this PR but I don't think we should merge yet. I think some functionality may have changed accidentally(?), see above. We will re-design this optimizer so it's parallel etc so maybe we should jump straight to that. On logging, I'm not sure I like the proposed approach. It makes it really hard to see if the bootstrapping keeps failing or how much progress it's making. Keep in mind that in the general case, bootstrapping can fail 100s of times and may even never succeed. Right now, the proposal here would make the user see zero progress. Maybe the better thing to do is to just bump the progress bar to full after bootstrapping exits :D But as I said, this all will change with parallelism. We should just jump right into that. |
@okhat Thanks for reviewing! Yea this progbar will not be valid if we enable parallelism.
Good point! That's actually an area I am uncertain about - I suspect if our users can understand what "bootstrap failure" means, I couldn't understand what bootstrap means until reading through the source code, which is unlikely to happen for our users. For us seeing bootstrapping failures helps with debugging, but users may find the logging as pure noise and no idea what that means. However, as you mentioned, if bootstrapping keeps failing users will see 0 progress, which is bad as well. Agree we should not merge this PR! I will open a separate one which only contains trivial fixes as in this PR for better readability. |
The PR has two parts:
Before the PR:
After the PR: