Hitesh/add diff strategies logging #6189
Conversation
beyang
changed the title
| throw new Error(`Unknown diff strategy identifier: ${identifier}`) | ||
| } | ||
| } | ||
| import type { AutocompleteContextSnippetMetadataFields } from '../../../../../../../lib/shared/src/completions/types' | ||
|
|
||
| export interface RecentEditsRetrieverDiffStrategy { |
There was a problem hiding this comment.
Can you add a docstring describing the interface contract and what role this plays in the overall process of integrating recent low-level edits into a more coherent history of recent edits? My rough understanding is that this takes a low-level or line-by-line diff and outputs a set of larger edits that serve as better context for LLM consumption, but how that's done and what other components are involved is a bit unclear. Also, what does the metadata method return and how is that expected to be useful in the aforementioned pipeline?
There was a problem hiding this comment.
Sure @beyang
Added the documentation for the interface. Also added a comment stating what we consider as a logical change.
To make sure the data is correct, I sampled the diff examples from different strategies which I manually analysed. Also adding a link here for reference.
…e metadata from retreivers
hitesh-1997
force-pushed
the
hitesh/add-diff-stratagies-logging
branch
from
251ffc7
to
717cb79
Compare
lib/shared/src/completions/types.ts
Outdated
| */ | ||
| recentEditsRetrieverDiffStrategy?: string | ||
| diffStrategyMetadata?: AutocompleteContextSnippetMetadataFields |
There was a problem hiding this comment.
Does it make sense to name the type in the same way?
| diffStrategyMetadata?: AutocompleteContextSnippetMetadataFields | |
| diffStrategyMetadata?: AutocompleteDiffStrategyMetadata |
There was a problem hiding this comment.
Thanks for noticing,
Since This field is intended to capture additional metadata from the context retrievers and not limited by diff retriever. Changed the name to retrieverMetadata
| @@ -1,48 +1,34 @@ | |||
| import type { PromptString } from '@sourcegraph/cody-shared' | |||
| import type * as vscode from 'vscode' | |||
| import { AutoeditWithShortTermDiffStrategy } from './auotedit-short-term-diff' | |||
| import { UnifiedDiffStrategy } from './unified-diff' | |||
| import type { AutocompleteContextSnippetMetadataFields } from '../../../../../../../lib/shared/src/completions/types' | |||
There was a problem hiding this comment.
We should use @sourcegraph/cody-shared to import from lib/shared. AutocompleteContextSnippetMetadataFields import should be added to the existing cody-shared import statement.
To make AutocompleteContextSnippetMetadataFields available to the outside world, it should be added to lib/shared/src/index.ts.
There was a problem hiding this comment.
Thanks for noticing this,
I use this extension which automatically add imports for the variables I copy, guess it doesn't consider this.
Fixed at all the places.
| @@ -1,4 +1,5 @@ | |||
| import { PromptString } from '@sourcegraph/cody-shared' | |||
| import type { AutocompleteContextSnippetMetadataFields } from '../../../../../../../lib/shared/src/completions/types' | |||
There was a problem hiding this comment.
Let's update all the other AutocompleteContextSnippetMetadataFields imports in the same way.
| diffStrategyIdentifier: RecentEditsRetrieverDiffStrategyIdentifier.UnifiedDiff, | ||
| diffStrategyList: [new UnifiedDiffStrategy({ addLineNumbers: false })], |
There was a problem hiding this comment.
We don't need to address this now, but should we remove supercompletion-related code from the codebase? @hitesh-1997 @beyang. We can always check it back in, but we won't need to maintain it.
There was a problem hiding this comment.
That should be okay I think with me !!
## Context Add diff strategies for diff calculation Build on top of PR: #6189 ## Test plan Added CI tests
change the interface schema for diff calculation to enable logging the metadata from retreivers
Build on top of PR: #6188
Test plan