-
Notifications
You must be signed in to change notification settings - Fork 4
Smarter example selection #18
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: main
Are you sure you want to change the base?
Conversation
3538efc to
38603d0
Compare
9080243 to
bba6687
Compare
bba6687 to
f3ca5e1
Compare
jamadeo
left a comment
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.
Definitely the right way to go for reducing context. I left some questions about response parsing.
@katzdave has some recent additions to trim the context window when you hit the limit, maybe we should invoke this when we hit that?
| "excluded_examples": [], | ||
| } | ||
| ) | ||
| except Exception as e: |
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.
avoid catching Exception, the stack trace should have enough info here
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.
ack
| from .example_selector import select_relevant_examples | ||
|
|
||
| log("[agent] Running example selection analysis...") | ||
| selection_result = await select_relevant_examples(target_files, examples, client) |
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.
should we put it behind a flag?
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 good point. which would be the default tho?
| if "choices" in response and response["choices"]: | ||
| message = response["choices"][0].get("message", {}) | ||
| if isinstance(message, dict): | ||
| content = message.get("content", "") | ||
| else: | ||
| content = str(message) | ||
| else: | ||
| content = str(response) |
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's happening here?
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.
this is different format handling, but now I realize it's redundant.
| deletions: int | ||
|
|
||
|
|
||
| @dataclass |
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.
FWIW I am not a fan of having files called utils. It can just mean anything. maybe we can just call this code_handling and move all code handling here? now it just a mixed bag of things max built.
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.
point taken.
| "excluded_examples": [], | ||
| } | ||
| ) | ||
| except Exception as e: |
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.
come on, don't catch general Exceptions
| if not examples: | ||
| raise FileNotFoundError("No valid example pairs found in examples directory") | ||
|
|
||
| from .example_selector import select_relevant_examples |
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.
same
| from .example_selector import select_relevant_examples | ||
|
|
||
| log("[agent] Running example selection analysis...") | ||
| selection_result = await select_relevant_examples(target_files, examples, client) |
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.
this can throw exceptions, which would stop the whole thing. is that what we want?
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.
if it is not under feature flag - probably yes as there will be no examples.
In case we put it under a flag - probably not
| ] | ||
|
|
||
| response, _ = await client.generate_completion(messages=messages, temperature=0.1) | ||
|
|
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.
openai allows you to specify json as an output format, which is more reliable than asking for it. you can even specify the format.
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.
yeah, but we aren't using claude? or you mean openAi style client?
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.
we are using databricks which provides an openai client independent of the model we choose I think. of course that's a leaky abstraction so it might not work, but we should give it a shot. it's a useful trick in general
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.
gotcha
This PR enables intelligent example selection to minimize the impact on context window