Add initial support for variable resolution from user input #108

martin-fleck-at · 2024-07-29T17:08:29Z

What it does

Ensure we always create a variable part even for undefined variables
-- Prompt text will then default to user text (including '#')
Allow adopters to register variable resolution contributions
-- Given a particular variable name, argument and location
Automatically resolve all variable parts in a chat request
-- Ensure parts always provide a matching prompt text

Relates to https://github.com/eclipsesource/osweek-2024/issues/46

How to test

Test that normal chat still works as before
Use variable '#today' in your prompt and try different arguments such as 'inUnixSeconds' or 'inIso8601' combined as '#today:inIso8601' to see different results.

Follow-ups

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

martin-fleck-at · 2024-07-29T17:12:24Z

packages/ai-chat/src/common/chat-util.ts

@@ -26,16 +26,17 @@ export interface ChatMessage {
 export const getMessages = (model: ChatModel, includeResponseInProgress = false): ChatMessage[] =>
    model.getRequests().flatMap(request => {
        const messages: ChatMessage[] = [];
+        const query = request.message.parts.map(part => part.promptText).join('');


- Ensure we always create a variable part even for undefined variables -- Prompt text will then default to user text (including '#') - Allow adopters to register variable resolution contributions -- Given a particular variable name, argument and location - Automatically resolve all variable parts in a chat request -- Ensure parts always provide a matching prompt text Relates to eclipsesource/osweek-2024#46 Co-authored-by: Christian W. Damus <[email protected]>"

- Allow adopters to register resolvers with priority - Ensure we use a dedicated frontend service to init contributions - Push variable service into core -- Generic variable handling for all agents and UI layers -- Chat-specific variable handling only in the chat layer

…bles

eneufeld

Some questions

eneufeld · 2024-07-30T10:09:02Z

packages/ai-chat/src/browser/agent-frontend-module.ts

@@ -40,8 +39,7 @@ export default new ContainerModule(bind => {
    bind(ChatRequestParserImpl).toSelf().inSingletonScope();
    bind(ChatRequestParser).toService(ChatRequestParserImpl);

-    bind(DummyChatVariablesService).toSelf().inSingletonScope();
-    bind(ChatVariablesService).toService(DummyChatVariablesService);
+    bind(AIVariableContribution).to(TodayVariableContribution).inSingletonScope();


why is the todayvariable in chat only?

To demonstrate the chat-specific variables context information (which just happens not to be used). Ideally it will be moved into a sample extension.

let's move to ai-core then

eneufeld · 2024-07-30T10:13:26Z

packages/ai-chat/src/common/chat-request-parser.ts

-        }
-
-        return;
+        return new ChatRequestVariablePart(varRange, name, variableArg);


what happens if the variable cannot be resolved? we still show the chatpart?

What exactly do you mean by 'show'? The part remains unresolved and in the textual part (promptText) it will simply keep the original syntax. On resolution the prompt text is changed. You can also check if a part is resolved.

so we keep the variable in the prompt even if we can't resolve it, something vscode doesn't do, but this can be changed in a follow up

eneufeld · 2024-07-30T10:16:59Z

packages/ai-chat/src/common/chat-variables.ts

-export interface IChatVariableData {
-    id: string;
-    name: string;
+export interface VsCodeChatRequestVariableValueData {


why is it called vsCode... now?

The intention is that this is to support the interface with VSCode chat extensions. Perhaps it could have a more generic name.

We could call it Extension but it is a specific format for the extension API that VS Code expects. For now it is not really used and could be removed as well.

if it is not used I would tend to remove it

eneufeld · 2024-07-30T10:18:56Z

packages/ai-chat/src/node/agent-backend-module.ts

-    bind(ChatVariablesService).toService(DummyChatVariablesService);
+    bind(DefaultAIVariableService).toSelf().inSingletonScope();
+    bindContributionProvider(bind, AIVariableContribution);
+    bind(AIVariableService).toService(DefaultAIVariableService);


why is this registered in the chat module?

Right. This should be registered in core.

packages/ai-core/src/common/variable-service.ts

eneufeld · 2024-07-30T10:27:41Z

packages/ai-core/src/common/variable-service.ts

+    }
+
+    protected initContributions(): void {
+        this.contributionProvider.getContributions().forEach(contribution => contribution.registerVariables(this));


why not retrieve the necessary information from the contribution and register the resolver here directly? we now go to the contribution give it the service and it calls the service to provide data, this is a bit confusing

I think this is the more common Theia way to do this, cf. CommandService and CommandContribution. I think we should not confuse AIVariableResolver with AIVariableContribution. A contribution may register variables and resolvers but it may also unregister a variable or use other parts of the service.

…bles

- Move today variable contribution to core package as it is generic - Remove VS code extension-specific interfaces - Do not register variable service in backend

feat/46-variables

martin-fleck-at requested a review from eneufeld July 29, 2024 17:10

martin-fleck-at commented Jul 29, 2024

View reviewed changes

martin-fleck-at added 2 commits July 30, 2024 10:53

martin-fleck-at force-pushed the feat/46-variables branch from e3a12f8 to d8794bb Compare July 30, 2024 08:55

Merge remote-tracking branch 'origin/feat/ai-chat' into feat/46-varia…

445b772

…bles

martin-fleck-at mentioned this pull request Jul 30, 2024

Resolve variables in prompt templates #116

Merged

1 task

eneufeld reviewed Jul 30, 2024

View reviewed changes

cdamus and others added 2 commits July 30, 2024 13:26

Merge remote-tracking branch 'origin/feat/ai-chat' into feat/46-varia…

0949843

…bles

Integrate PR review

5cb0946

- Move today variable contribution to core package as it is generic - Remove VS code extension-specific interfaces - Do not register variable service in backend

eneufeld approved these changes Jul 30, 2024

View reviewed changes

martin-fleck-at added 2 commits July 30, 2024 14:23

Merge remote-tracking branch 'origin/feat/ai-chat' into

5eea5fe

feat/46-variables

Merge remote-tracking branch 'origin/feat/ai-chat' into

e63b26f

feat/46-variables

martin-fleck-at merged commit ba3bf65 into feat/ai-chat Jul 30, 2024
1 check passed

sdirix deleted the feat/46-variables branch August 12, 2024 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial support for variable resolution from user input #108

Add initial support for variable resolution from user input #108

martin-fleck-at commented Jul 29, 2024 •

edited

Loading

martin-fleck-at Jul 29, 2024

eneufeld left a comment

eneufeld Jul 30, 2024

cdamus Jul 30, 2024

eneufeld Jul 30, 2024

eneufeld Jul 30, 2024

martin-fleck-at Jul 30, 2024

eneufeld Jul 30, 2024

eneufeld Jul 30, 2024

cdamus Jul 30, 2024

martin-fleck-at Jul 30, 2024

eneufeld Jul 30, 2024

eneufeld Jul 30, 2024

cdamus Jul 30, 2024

eneufeld Jul 30, 2024

martin-fleck-at Jul 30, 2024

eneufeld Jul 30, 2024

Add initial support for variable resolution from user input #108

Add initial support for variable resolution from user input #108

Conversation

martin-fleck-at commented Jul 29, 2024 • edited Loading

What it does

How to test

Follow-ups

Review checklist

Reminder for reviewers

Choose a reason for hiding this comment

eneufeld left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martin-fleck-at commented Jul 29, 2024 •

edited

Loading