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

Smart Apply: Use system prompt to encourage code blocks being created #5290

Merged
merged 16 commits into from
Aug 23, 2024

Conversation

umpox
Copy link
Contributor

@umpox umpox commented Aug 22, 2024

Description

This PR improves the smart apply prompt tweak so that:

  • We use the system prompt when available
  • We use the same desired Markdown syntax when we include codebase context, which helps steer the LLM onto the right path.

I have ran an eval on this here: sourcegraph/cody-leaderboard#16

Quite happy with the results so far

Test plan

  • Tested creating code blocks on all major models

@umpox umpox marked this pull request as ready for review August 22, 2024 08:21
@umpox
Copy link
Contributor Author

umpox commented Aug 22, 2024

@abeatrix Seems just as reliable without the weird occasional behaviour in chat... Running evals here sourcegraph/cody-leaderboard#16

test('Test animal implementation name property', () => {
const dog = new Dog()
expect(dog.name).toEqual('Dog')
// src/animal.test.ts
Copy link
Contributor

Choose a reason for hiding this comment

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

This is added as a comment instead of markdown format we provided 👀

(It wasn't doing it before the prompt change though so not a regression but an edge case?)

lib/shared/src/chat/preamble.ts Outdated Show resolved Hide resolved
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.

im seeing a minor regression where it's including the file pointers in the response.

cc @jtibshirani on the response change

agent/src/__snapshots__/custom-commands.test.ts.snap Outdated Show resolved Hide resolved
* produce code blocks that we can associate with existing file paths.
* We want to read these file paths to support applying code directly to files from chat.
*/
const CHAT_PREAMBLE = psDedent`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const CHAT_PREAMBLE = psDedent`
const SMART_APPLY_PREAMBLE = ps`If your answer contains fenced code blocks in Markdown, include the full file path in the code block tag using this structure: \`\`\`$LANGUAGE:$FILEPATH\n\`\`\`.`
const CHAT_PREAMBLE = DEFAULT_PREAMBLE.join(SMART_APPLY_PREAMBLE)

I think we don't need to say that this is an additional rules in system prompt. We can probably update it to something like this? ( Haven't tested it though!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to try anything, will run an eval on this change and we can see!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abeatrix Applied this change and made a couple of more tweaks, looks pretty good!

Eval is here: sourcegraph/cody-leaderboard#16

My comments here are still relevant: sourcegraph/cody-leaderboard#16 (comment)

I think we can go ahead and merge this, we can always tweak later but let's get a patch out today as we may be both OOO next week

Copy link

‼️ Hey @sourcegraph/cody-security, please review this PR carefully as it introduces the usage of an unsafe_ function or abuses PromptString.

@umpox
Copy link
Contributor Author

umpox commented Aug 23, 2024

Have a failing test because the demo instance is replying with

{
  model: 'anthropic/claude-3-opus-20240229',
  speaker: 'assistant',
  error: {
    isChatErrorGuard: 'isChatErrorGuard',
    status: 400,
    traceId: '024f1e65bdeb9024d5dd7ed41efbe4ed',
    message: 'Request to https://demo.sourcegraph.com/.api/completions/stream?client-name=enterpriseclient&client-version=v1 failed with 400 : unsupported chat model "anthropic/claude-3-opus-20240229" (default "anthropic::2023-06-01::claude-3-opus")\n',
    name: 'Error'
  }
}

It's likely because the demo config has changed since that test was last updated. Using demo.sourcegraph.com feels wrong here, so I'm going to skip this test. We should use a non-customer facing instance

@umpox umpox requested review from abeatrix and a team August 23, 2024 14:57
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.

Great! I haven't encountered any prompt leaks anymore, and I don't see any noticeable regression from the test results, so I think this would be a good change to patch the regression we are seeing. Thanks for fixing this!

@umpox umpox merged commit bdb946e into main Aug 23, 2024
19 checks passed
@umpox umpox deleted the tr/use-system-prompt-for-code-blocks branch August 23, 2024 15:37
umpox added a commit that referenced this pull request Aug 23, 2024
…#5290)

## Description

This PR improves the smart apply prompt tweak so that:
- We use the system prompt when available
- We use the same desired Markdown syntax when _we_ include codebase
context, which helps steer the LLM onto the right path.

I have ran an eval on this here:
sourcegraph/cody-leaderboard#16


## Test plan

- [x] Tested creating code blocks on all major models

<!-- Required. See
https://docs-legacy.sourcegraph.com/dev/background-information/testing_principles.
-->

---------

Co-authored-by: Beatrix <[email protected]>
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