Attach supplemental context to InlineCompletions request #463

viktorsaws · 2024-09-20T15:47:44Z

Problem

We don't attach cross file context Q InlineCompletion requests.

Solution

Migrate implementation of code to fetch supplemental context from open files in workspace from VSCode Q extension implementation https://github.com/aws/aws-toolkit-vscode/blob/4c3e64cc1987328ad59e2f879eaefd66dff2192a/packages/core/src/codewhisperer/util/supplementalContext/supplementalContextUtil.ts#L15
Attaching cross file context for test files (UTG context) is out of scope.

Entry point for using supplementalContext changes start in CodewhispererServer.ts. Supplemental context is computed for requested TextDocument, cached in Inline completion Session and attached to generateSuggestions service call. Then, supplemental context fields also added to CodeWhisperer Telemetry events, emitted from the server.

All computation logic and helper functions are ported as is, with only adapting them to work with LSP documents instead of VSCode APIs. Most unit tests also migrated as is, with adding extra tests for coverage.

This PR requires aws/language-server-runtimes#214 to be released in Runtimes.
Diff between original code and new code introduced here can be seen in viktorsaws#5.

TODO

Unit tests

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

ege0zcan · 2024-09-24T13:19:20Z

server/aws-lsp-codewhisperer/src/language-server/telemetry/types.ts

@@ -20,6 +20,10 @@ export interface CodeWhispererServiceInvocationEvent {
    codewhispererGettingStartedTask?: string
    reason?: string
    credentialStartUrl?: string
+    codewhispererSupplementalContextTimeout?: boolean


nit: from the name of this field, it is hard to understand that this is a boolean value showing whether the context collection timed out or not

I agree, but this is a Telemetry object field, as defined by CodeWhisperer service. We don't control the name and expected type here.

ege0zcan · 2024-09-24T13:21:21Z

server/aws-lsp-codewhisperer/src/language-server/utilities/filesystemUtilities.ts

+ * The file distance between A/B/C.java and A/D.java is 1
+ */
+export function getFileDistance(fileA: string, fileB: string): number {
+    // TODO: Test how it works with nested folders with spaces (%20 symbol)


This todo is not coming from the toolkit codebase but something we are introducing, why is that?

this is something I noticed when comparing logs for VSCode Q and Language Server. In Language server, files URIs are encoded and spaces are converted to %20 string. Since this is different from original environment, I wanted to double check that in this case this function works. I added few tests to cover this and will remove this comment,

ege0zcan · 2024-09-24T15:05:28Z

...dewhisperer/src/language-server/utilities/supplementalContextUtil/supplementalContextUtil.ts

+): CodeWhispererSupplementalContext | undefined {
+    const timesBeforeFetching = performance.now()
+
+    const isUtg = isTestFile(document.uri, {


I don't have full context, what does UTG actually stand for?

I think it stands for "unit test generator". I don't know how significant this meaning is, tbh.

ege0zcan · 2024-09-25T11:50:51Z

server/aws-lsp-codewhisperer/src/language-server/codeWhispererServer.ts

@@ -573,6 +605,28 @@ export const CodewhispererServerFactory =

        logging.log('Amazon Q Inline Suggestion server has been initialised')

+        const logSupplementalContext = (supplementalContext: CodeWhispererSupplementalContext | undefined) => {


Isn't this too verbose, i see in vscode toolkit codebase they use debug logging to log this information whereas it seems we log always

hm, yeah you're right. I added it as normal log since we don't have different log levels implemented.

Removed excessive logging in last commit

ege0zcan · 2024-09-25T11:51:18Z

server/aws-lsp-codewhisperer/src/language-server/telemetry/userTriggerDecision.test.ts

@@ -760,15 +763,15 @@ describe('Telemetry', () => {
                    aUserTriggerDecision({
                        codewhispererSessionId: 'cwspr-session-id-0',
                        codewhispererSuggestionState: 'Discard',
-                        codewhispererTimeToFirstRecommendation: 2520,
+                        codewhispererTimeToFirstRecommendation: 1260,


was this an intentional change?

Yes. This is caused by changing nested callback in CodeWhispererServer to async and I suspect it changes the chain of async callbacks resolution. The test emulates several inflight requests, and only this time in test environment is impacted.
In my manual E2E testing with compiled server, times are at same rate as before this change. I didn't want to refactor this test as it too complex :(

…ntext

volodkevych · 2024-09-26T14:45:04Z

server/aws-lsp-codewhisperer/src/language-server/codeWhispererServer.test.ts

+                    maxResults: 5,
+                    supplementalContexts: [
+                        { content: 'sample-content', filePath: '/SampleFile.java' },
+                        { content: 'sample-content', filePath: '/SampleFile.java' },


Why does it send the same file twice?

volodkevych · 2024-09-26T14:46:06Z

server/aws-lsp-codewhisperer/src/language-server/codeWhispererServer.test.ts

@@ -467,6 +474,39 @@ describe('CodeWhisperer Server', () => {
            assert.deepEqual(result, EMPTY_RESULT)
        })

+        describe('Supplemental Context', () => {
+            it('should send supplemental context', async () => {
+                // Open 3 files supporting cross-file context


nit: 2 files?

volodkevych · 2024-09-26T14:46:26Z

server/aws-lsp-codewhisperer/package.json

@@ -21,12 +21,13 @@
        "compile": "tsc --build",
        "postcompile": "npm run copyServiceClient",
        "copyServiceClient": "copyfiles -u 1 --error ./src/client/sigv4/*.json out && copyfiles -u 1 --error ./src/client/token/*.json out",
+        "fix": "npm run fix:prettier",


nit: do we really need this duplication?

volodkevych · 2024-09-26T14:53:08Z

server/aws-lsp-codewhisperer/src/language-server/codeWhispererServer.ts

@@ -191,6 +202,9 @@ const emitAggregatedUserTriggerDecisionTelemetry = (
            : undefined,
        codewhispererTimeToFirstRecommendation: session.timeToFirstRecommendation,
        codewhispererPreviousSuggestionState: session.previousTriggerDecision,
+        codewhispererSupplementalContextTimeout: session.supplementalMetadata?.isProcessTimeout,
+        codewhispererSupplementalContextIsUtg: session.supplementalMetadata?.isUtg,
+        codewhispererSupplementalContextLength: session.supplementalMetadata?.contentsLength,


Is it expected we set 3 props in this metric, but 4 above? The tests are checking for 4 properties, at least.

ege0zcan reviewed Sep 24, 2024

View reviewed changes

viktorsaws force-pushed the crossfile_context_src_only branch from 0e87ba4 to 3c56fe4 Compare September 24, 2024 19:28

viktorsaws mentioned this pull request Sep 24, 2024

Crossfile context - original implementation diff viktorsaws/language-servers#5

Draft

viktorsaws marked this pull request as ready for review September 24, 2024 20:15

viktorsaws requested review from a team as code owners September 24, 2024 20:15

ege0zcan reviewed Sep 25, 2024

View reviewed changes

ege0zcan approved these changes Sep 25, 2024

View reviewed changes

viktorsaws added 19 commits September 26, 2024 12:16

Initial port of supplemental context helpers

893cb6a

Port crossFileContextUtil and fetchSupplementalContextForSrc

4f7208a

Start of porting utg context

0009185

Finish porting utg context

4d1f37b

Remove UTG extraction function

2420132

Attach crossFile context to request

2609bc9

Delete sample files

fde3eef

Remove todos

881fb12

Add missing types

0849811

Fix failing tests; convert fetchSupplementalContext to sync function

9dc067b

Move new utils to utilities directory

dcc1335

Port unit tests part 1

c005959

Add unit tests for crossFileContextUtil

f4462c6

Switch to async workspace.getAllTextDocuments and fetchSupplementalCo…

ed4311f

…ntext

Add tests for supplementalContextUtil.ts

ecedc2c

Add test for supplemental context to codeWhispererServer.test.ts

afb5370

Extra tests for filesystem

bc4a90c

Update README

dadc583

Fix failing test compilation

335a895

viktorsaws force-pushed the crossfile_context_src_only branch from ae70cec to 335a895 Compare September 26, 2024 10:16

Remove excessive logging for supplemental context

04e9bd7

viktorsaws merged commit 813978f into aws:main Sep 26, 2024
5 checks passed

volodkevych reviewed Sep 26, 2024

View reviewed changes

viktorsaws mentioned this pull request Oct 9, 2024

Release @aws/lsp-codewhisperer:v0.0.15 #486

Merged

francescoopiccoli mentioned this pull request Jan 3, 2025

feat: pass supplemental contexts only for token client #697

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Attach supplemental context to InlineCompletions request #463

Attach supplemental context to InlineCompletions request #463

viktorsaws commented Sep 20, 2024 •

edited

Loading

ege0zcan Sep 24, 2024 •

edited

Loading

viktorsaws Sep 24, 2024

ege0zcan Sep 24, 2024

viktorsaws Sep 24, 2024

ege0zcan Sep 24, 2024

viktorsaws Sep 24, 2024

ege0zcan Sep 25, 2024

viktorsaws Sep 25, 2024

viktorsaws Sep 26, 2024

ege0zcan Sep 25, 2024

viktorsaws Sep 25, 2024

volodkevych Sep 26, 2024

volodkevych Sep 26, 2024

volodkevych Sep 26, 2024

volodkevych Sep 26, 2024

		@@ -573,6 +605,28 @@ export const CodewhispererServerFactory =

		logging.log('Amazon Q Inline Suggestion server has been initialised')

		const logSupplementalContext = (supplementalContext: CodeWhispererSupplementalContext \| undefined) => {

Attach supplemental context to InlineCompletions request #463

Attach supplemental context to InlineCompletions request #463

Conversation

viktorsaws commented Sep 20, 2024 • edited Loading

Problem

Solution

TODO

License

ege0zcan Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viktorsaws commented Sep 20, 2024 •

edited

Loading

ege0zcan Sep 24, 2024 •

edited

Loading