Skip to content
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

Merged
merged 4 commits into from
Nov 18, 2024

Conversation

pranavgaikwad
Copy link
Contributor

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.

Copy link
Contributor

@fabianvf fabianvf left a 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

Copy link
Contributor

@shawn-hurley shawn-hurley left a 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")
Copy link
Contributor

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]>
@pranavgaikwad
Copy link
Contributor Author

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
Copy link
Contributor

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?

Copy link
Contributor Author

@pranavgaikwad pranavgaikwad Nov 15, 2024

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

Copy link
Contributor

@shawn-hurley shawn-hurley left a 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]>
@pranavgaikwad
Copy link
Contributor Author

Good to merge now

@shawn-hurley
Copy link
Contributor

Will merge first thing monday as I assume most folks are done for the weekend

@shawn-hurley shawn-hurley merged commit 24a77ee into konveyor:main Nov 18, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants