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

make slash command required in command configs #841

Merged
merged 27 commits into from
Sep 3, 2023

Conversation

taras-yemets
Copy link
Contributor

@taras-yemets taras-yemets commented Aug 28, 2023

Next iteration on #631
Follow-up on #798
Design

  1. Changes commands format in default-prompts.json and .vscode/cody.json:
    • slash command now is required and acts as a key in commands map, slashCommand field on a command is removed
    • command name previously used as a key is now used as a command description.
  2. Updates the custom command creation process making the slash command input required and going first before the description and prompt inputs.
  3. If spotted user or workspace Cody config with commands in "old" format, transforms them to the new one, saves updated config file and informs user about it.
Screen.Recording.2023-09-01.at.16.28.04.mov

Test plan

CI passes.

@taras-yemets taras-yemets changed the title make slash command reqired make slash command required in command configs Aug 30, 2023
Comment on lines 111 to 118
for (const key in prompts) {
if (Object.prototype.hasOwnProperty.call(prompts, key)) {
const prompt = prompts[key]
prompt.name = key
prompt.type = type
// replace any '/' from the start
const slashCommand = prompt.slashCommand?.replace(/^\//, '')
if (slashCommand?.length) {
prompt.slashCommand = `/${slashCommand}`
}
prompt.slashCommand = key.startsWith('/') ? key : '/' + key
this.myPromptsMap.set(key, prompt)
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open question: how to ensure these changes are backward compatible (users with the previous format of cody.json with command names as keys, users with recipes)?
cc: @abeatrix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A possible solution could be to apply new logic if the description field exists in a command. This would mean that slash command is used as a key.
If the description field is missing, store the command only if the slashCommand is defined.
However, this will lead to partial use of cody.json content (in "old" format commands only with slash commands will be stored). Maybe we should rewrite (make slash command a key) the cody.json content when we spot the old format? WDYT?

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.

Left comments in line regarding migrating current file into next format

}
this.myPromptsMap.set(key, prompt)
// ensure there is only one leading forward slash
prompt.slashCommand = key.replace(/^\/+/, '').replace(/^/, '/')
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we should also update the save method to handle backward compatibility?

I think we should also extract the first words from all the current keys and turn them into the prompt.slashCommand option here to auto-migrate the current commands without the "slashCommand" settings for our users?

Maybe we can add a CTA to let them know how they can change the command keyword afterward. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out! Updated the PR. Now we transform "old" format commands to the new format, update the corresponding config files, and inform the user about the change.
This PR converts names to dash-case (not the first word in the command as it's not guaranteed to be unique) and uses them as slash commands.

@taras-yemets taras-yemets requested review from toolmantim, abeatrix and a team September 1, 2023 13:30
@taras-yemets taras-yemets marked this pull request as ready for review September 1, 2023 13:35
@taras-yemets
Copy link
Contributor Author

@toolmantim, except for the overall implementation, I'm looking for your feedback on:

  • the updated custom command creation flow;
  • "old" format configs transformation and notification shown after.

@abeatrix
Copy link
Contributor

abeatrix commented Sep 1, 2023

tried to run the branch but i'm getting:
image

Looks like issue with sentry?

@taras-yemets
Copy link
Contributor Author

taras-yemets commented Sep 1, 2023

Looks like issue with sentry?

@abeatrix, it seems so. But it doesn't seem to be caused by te current PR changes.

Copy link
Contributor

@toolmantim toolmantim left a comment

Choose a reason for hiding this comment

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

Works great! I gave it a thorough test locally, and popped on some commits to update a bunch of the UI labels.

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.

Works beautifully! Thanks @taras-yemets!

I have a PR opened that would be affected by the changes in this PR. Since my changes are closer to the main branch as it is using the old format, can you review my PR and see if we can merge mine before yours? Or would it cause other conflicts that would not matter which one gets merged first?

Here is the PR: #907

cbart and others added 3 commits September 3, 2023 22:19
Part of https://github.com/sourcegraph/sourcegraph/issues/56182

Here's how to get this running:

1. Azure keys
* https://portal.azure.com > Azure OpenAI > sourcegraph-test-oai > Keys
and Endpoint
   * Copy the key and `export AZURE_API_KEY="<paste the key here>"`
1. `export SRC_ENDPOINT=https://sourcegraph.sourcegraph.com`
1. `export SRC_ACCESS_TOKEN="<s2 access token goes here>"`
1. Run like so: `pnpm run start --label="Beam search"
--output="$HOME/test.results" --provider=azure`
   * single beam search test runs faster than total # test cases
* output file actually allows you to drill into the LLM interaction
details

## Test plan

Manually verified:

### success scenario with Azure OpenAI and

```
➜  e2e git:(cb/s/56182-azure) ✗ pnpm run start --label="Beam search" --output="$HOME/azure.results" --provider=azure

> @sourcegraph/cody-e2e@0.0.1 start /Users/cbart/cody/e2e
> pnpm run --silent build && node dist/e2e "--label=Beam search" "--output=/Users/cbart/azure.results" "--provider=azure"

Run 1 of 1

Testing (1/1): Beam search (github.com/huggingface/transformers)
Incorrect answers (LLM judge): 0/2 (0%)
Incorrect or partial answers (LLM judge): 1/2 (50%)
Missing facts: 5/9 (56%)
Hallucinated entities: 9/9 (100%)

Run 1 of 1 summary:
Incorrect answers (LLM judge): 0/2 (0%)
Incorrect or partial answers (LLM judge): 1/2 (50%)
Missing facts: 5/9 (56%)
Hallucinated entities: 9/9 (100%)
```

### fast fail when not enough env vars provided

```
➜  e2e git:(cb/s/56182-azure) unset AZURE_DEPLOYMENT_ID
➜  e2e git:(cb/s/56182-azure) ✗ pnpm run start --label="Beam search" --output="$HOME/azure.results" --provider=azure

> @sourcegraph/cody-e2e@0.0.1 start /Users/cbart/cody/e2e
> pnpm run --silent build && node dist/e2e "--label=Beam search" "--output=/Users/cbart/azure.results" "--provider=azure"

Missing required environment variables: AZURE_DEPLOYMENT_ID
1. https://portal.azure.com > Azure OpenAI > sourcegraph-test-oai > Keys and Endpoint
2. Go to Model Deployments > Manage Deployments. Find the deployment name and:
   export AZURE_DEPLOYMENT_ID="<paste deployment name here>"
 ELIFECYCLE  Command failed with exit code 1.
```
@taras-yemets taras-yemets merged commit 519145a into main Sep 3, 2023
6 of 7 checks passed
@taras-yemets taras-yemets deleted the taras-yemets/make-slash-command-required branch September 3, 2023 20:36
@taras-yemets
Copy link
Contributor Author

I have a PR opened that would be affected by the changes in this PR. Since my changes are closer to the main branch as it is using the old format, can you review my PR and see if we can merge mine before yours? Or would it cause other conflicts that would not matter which one gets merged first?
Here is the PR: #907

@abeatrix There is an ongoing discussion about whether the referenced PR will be included in the upcoming release, so I'm this one to have a bit more time to test it on main. I'm happy to help with resolving merge conflicts!

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.

4 participants