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.

Sign up for GitHub

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

Jump to bottom

Edit: Improve response consistency #1892

Merged
merged 11 commits into from
Nov 28, 2023
Merged

Edit: Improve response consistency #1892

merged 11 commits into from
Nov 28, 2023

Conversation

umpox
Copy link
Contributor

@umpox umpox commented Nov 27, 2023 •
edited

closes #1790

Description

This PR improves edit consistency by:

  • "Putting words into the LLM's mouth". As we're using Claude we can start the transcript with the expected tag so we're more likely to get a valid output.
    • For "Add" intents, we also include the preceding code as part of the injected transcript
  • Using non-HTML related XML tags. Using XML tags to declare different parts of the prompt does show strong improvements (I tried removing them all apart from the main one), but the problem I think is that the LLM can become confused as to if this is HTML code - especially when making JS/TS edits. We now add numbers to the tags, which would be invalid for a HTML tag. This seems to help steer the response a lot.
  • Various small prompt tweaks

Test plan

Create fixups:

  • Edits from selection
  • Adding code from no selection
  • Fixing error diagnostics
  • Doc command. Code action commands to document symbols

@umpox umpox marked this pull request as ready for review November 27, 2023 14:39
@umpox umpox requested review from abeatrix and a team November 27, 2023 14:39
abeatrix
abeatrix requested changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

Tested the document code-action, /edit and /doc commands and everything worked as expected. also tested the Undo and retry code lenses. The Undo code lense worked, but the retry button would remove the code that we want to retry on?

See loom for details: https://www.loom.com/share/13ced7cf376c473cb43b4bb2ccebf4e4?sid=48a31348-97be-4aab-bfc4-cb1593260a0e

@umpox umpox self-assigned this Nov 27, 2023
@umpox
Copy link
Contributor Author

umpox commented Nov 28, 2023

Thanks @abeatrix. The issue I think is because we reuse the selectionRange for the task on retry. If this selectionRange expands then it won't match the original range.

It's a little tricky to get this right:

  • I think we need a copy of the original range task.originalRange
  • That range should never expand or shrink BUT it should shift down if new code is added above the range
  • Use task.originalRange on retry.

It's not related to these changes (I think possibly a bug from the original retry logic), so I'll create a follow up PR for this one.

Looking at it now

abeatrix
abeatrix approved these changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@abeatrix abeatrix left a comment

Choose a reason for hiding this comment

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

Thanks for clarifying. Approved since the located bug is not from this PR.

@umpox umpox merged commit 6dfe516 into main Nov 28, 2023
14 checks passed
@umpox umpox deleted the tr/fixup-consistency branch November 28, 2023 10:55
@umpox umpox mentioned this pull request Nov 28, 2023
Merged
@umpox
Copy link
Contributor Author

umpox commented Nov 28, 2023

@abeatrix Fix is here: #1926!

umpox added a commit that referenced this pull request Nov 30, 2023
@umpox
Edit: Use a tracked originalRange when retrying a task (#1926)
e6753d3 
## Description


https://github.com/sourcegraph/cody/assets/9516420/0f6ac95a-621c-42f2-b42c-b3dd395eaaf3

Follow up on:
#1892 (comment)

We were retrying fixup tasks with the `selectionRange` - this is not
suitable as this range may shrink or expand depending on the code that
the LLM produces.

Instead we should use the `originalRange` - the range that is provided
when the fixup task is created. We still need to track the positions of
the range, as it could shift due to code being added above.

## Test plan

1. Create fixups
2. Press "retry"
3. Check retry range is correct (code is not unnecessarily deleted,
unless caused by the LLM)

<!-- Required. See
https://docs.sourcegraph.com/dev/background-information/testing_principles.
-->
@umpox umpox assigned umpox and unassigned umpox Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abeatrix abeatrix abeatrix approved these changes

Assignees

@umpox umpox

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Edit: Improve response format reliability
2 participants
@umpox @abeatrix