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

Update PR with review comments #174

Merged
merged 15 commits into from
Oct 22, 2024
Merged

Conversation

malhotra5
Copy link
Contributor

@malhotra5 malhotra5 commented Oct 20, 2024

Summary

This PR addresses #27

  • Updated openhands_resolver/resolve_issues.py with flag --issue-type pr
  • Recreates PR locally based on PR identity (number and repo name)
  • Downloads contextual information
    • original linked issues the PR would close
    • review comments
  • merges comment threads into singular string
  • all unresolved comments are passed (together) as the problem statement into custom prompt openhands_resolver/prompts/resolve/basic-followup.jinja
  • same workflow as before is maintained for codebase modifications
  • output.jsonl indicates if all comments were resolved successfully; success indicator were not implemented on a per comment basis

Note: version bumping was not performed on this PR as described in .openhands_instructions

Future work to be done

  1. Currently all the unresolved comments are passed at once for a given PR. Possibly process unresolved comment separately? This may lead to following issues -
    a. Should every resolved comment open a new PR request, or update the current one?
    b. What happens when 3/5 comments are resolved? How can the user specify which code changes to push on a per comment basis?
    c. How are comment dependencies handled? For instance, if two comments address the same part of the codebase, handling them separately may create merge conflicts. Do we expect the user to perform incremental commenting to mitigate this?

previously, the same message thread was stored as separate messages;

they are now combined into one unresolved comment with an indicator for the latest message

all unresolved comments of this structure as passed to code agent
@neubig neubig self-requested a review October 21, 2024 00:37
Copy link
Contributor

@neubig neubig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, this is awesome, thanks so much for doing this! I left a bunch of comments below, but many of them are just part of a single section to make it so issue_type is not null. They can be accepted by going to files and clicking "add suggestion to batch" and then you might need to clean up some stuff I missed. I'm happy to review again when you're done revising.

Regarding the questions:

a. Should every resolved comment open a new PR request, or update the current one?

I think update the current one. It's a little tricky because resolving the comments might mess up the current PR, but maybe that's OK since we can roll back to the previous commit?

b. What happens when 3/5 comments are resolved? How can the user specify which code changes to push on a per comment basis?

Yeah, great question. I'd say that we might want to push anyway and notify the user that we identified 5 comments, but were only able to resolve 3. It'd be awesome if we could actually output that as a structured checklist in markdown like:

I have updated the PR and resolved some of the issues that were cited in the pull request review. Specifically, I identified the following revision requests, and all the ones that I think I successfully resolved are checked off. All the unchecked ones I was not able to resolve, so manual intervention may be required:

- [ ] ...
- [x] ...
- [ ] ...
- [x] ...
- [x] ...

We could even provide this as an in-context example to a prompt in the guess_success() function that generates the PR message.

c. How are comment dependencies handled? For instance, if two comments address the same part of the codebase, handling them separately may create merge conflicts. Do we expect the user to perform incremental commenting to mitigate this?

I think we'll want to try to resolve all comments at the same time, which will prevent this from being an issue.

openhands_resolver/github_issue.py Outdated Show resolved Hide resolved
openhands_resolver/github_issue.py Outdated Show resolved Hide resolved
openhands_resolver/prompts/resolve/basic-followup.jinja Outdated Show resolved Hide resolved
openhands_resolver/prompts/resolve/basic-followup.jinja Outdated Show resolved Hide resolved
openhands_resolver/resolve_issues.py Outdated Show resolved Hide resolved
openhands_resolver/resolve_issues.py Outdated Show resolved Hide resolved
openhands_resolver/resolve_issues.py Outdated Show resolved Hide resolved
openhands_resolver/resolve_issues.py Outdated Show resolved Hide resolved
openhands_resolver/resolve_issues.py Outdated Show resolved Hide resolved
openhands_resolver/resolve_issues.py Outdated Show resolved Hide resolved
malhotra5 and others added 7 commits October 21, 2024 15:01
performing a guess for every comment -> forces one true/false + explanation per comment

for previous iteration llm would frequently provide wrong number of explanations (e.g 3 explanations even though 5 comments were provided)
used yellow for the "-" instead of orange; orange doesn't exist in term color options
@malhotra5
Copy link
Contributor Author

Feedback has been addressed!

Summary of changes

  1. Prevent issue_type from being null
  2. Follow best practice when ordering function parameters
  3. Split large test cases into multiple
  4. Added success indicator per review comment with explanation (initially all comments were evaluated together, but changed this to be one evaluation per comment -> see commit #13 description for reasoning)
  5. Updated output.jsonl (openhands_resolver/resolver_output.py) to contain issue-type and success indicator per comment

Considerations

  1. I haven't implemented updating of the PR in openhands_resolver/send_pull_request.py after openhands resolves comments. I imagine this should be implemented in a separate PR given this one is quite long?
  2. The success checklist (reference attached below) is logged inside process_issue in openhands_resolver/resolve_issue.py; should this be moved to openhands_resolver/visualize_resolver_output.py instead?

Success checklist reference:

I have updated the PR and resolved some of the issues that were cited in the pull request review. Specifically, I identified the following revision requests, and all the ones that I think I successfully resolved are checked off. All the unchecked ones I was not able to resolve, so manual intervention may be required:

- [ ] ...
- [X] ...
- [ ] ...
- [X] ...
- [X] ...

@malhotra5 malhotra5 requested a review from neubig October 21, 2024 22:26
Copy link
Contributor

@neubig neubig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks!

There are lots of "if" statements switching between issue and PR that we might want to refactor a little later to simplify the logic, but overall this is a good change and we should get it in.

I agree that send_pull_request.py could be made in a separate PR.

@neubig neubig enabled auto-merge (squash) October 22, 2024 01:05
@neubig neubig merged commit dbdeb7e into All-Hands-AI:main Oct 22, 2024
2 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.

2 participants