-
Notifications
You must be signed in to change notification settings - Fork 38
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
🐛 Fix dependency agent back and forth chat for not-so-good models #490
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.
I think this makes sense, @shawn-hurley is more familiar with this agent though.
LGTM
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.
Big +1 on the test that was added!
@@ -88,7 +88,7 @@ def execute_task(self, rcm: RepoContextManager, task: Task) -> TaskResult: | |||
) | |||
pom = os.path.join(os.path.join(rcm.project_root, "pom.xml")) | |||
# Needed to remove ns0: | |||
ET.register_namespace("", "http://maven.apache.org/POM/4.0.0") | |||
# ET.register_namespace("", "http://maven.apache.org/POM/4.0.0") |
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.
Lets just remove this ?
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
Signed-off-by: Pranav Gaikwad <[email protected]>
I am testing this again today with full flow because I was seeing pom.xml issues with white spaces. I have Fabians changes checked out now. |
# We do not believe that we should not continue now we have to continue after running the code that is asked to be run. | ||
# The only exception to this rule, is when we actually update the file, that should be handled by the caller. | ||
# This happens sometimes that the LLM will stop and wait for more information. | ||
# we have to keep the chat going until we get a final answer |
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 have some limit here, say 5-8 round trips? if it is more, then something is not correct, and we need to exit?
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.
I set the limit to 3 but can increase
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.
I still approve, are you ready to take off the do not merge label?
Signed-off-by: Pranav Gaikwad <[email protected]>
Good to merge now |
Will merge first thing monday as I assume most folks are done for the weekend |
I think we can simplify the agent a lot because effectively only one of the tools is actually being used but we describe 4 tools in the prompt. But I didn't want to introduce new issues because of it, so I want to take the time to redo / simplify the agent.