-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Typo/nit fixes and add tests for BoostrapWithRandomSearch #1502
base: main
Are you sure you want to change the base?
Conversation
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.
Hi @chenmoneygithub , let's leave the commented code / variable names as-is for now as we make decisions on how to integrate this going forward.
The test for RandomSearch is a bit trivial - I would recommend following how the test is setup for BootstrapFewShot
here and adding the validations of multiple optimization program candidates in this test. Let me know if that makes sense
@@ -48,16 +48,10 @@ def __init__( | |||
self.max_num_samples = max_bootstrapped_demos | |||
self.max_errors = max_errors | |||
self.num_candidate_sets = num_candidate_programs | |||
# self.max_num_traces = 1 + int(max_bootstrapped_demos / 2.0 * self.num_candidate_sets) |
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.
comment back in
|
||
scores.append(score) | ||
print(f"Scores so far: {scores}") |
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.
comment back in
@arnavsinghvi11 For a public repo with big user base, there shouldn't be any zombie code. Three reasons:
if something is worth further discussion opening an issue for tracking is a better approach. Re the unit test - the goal of this PR is lint fix, and the test is for sanity check. I am not sure what's the best way to test it out without making actual LLM calls, will work on that in a separate PR. |
Fix a few odds in the BootstrapFewShotWithRandomSearch optimizer:
program2
are confusing.Also added basic unit test case for BootstrapFewShotWithRandomSearch.