Allow OpenAI to be used with Terminal Chat #17540

PankajBhojwani · 2024-07-09T22:04:12Z

Summary of the Pull Request

Implements OpenAILLMProvider, which is an implementation of ILMProvider that uses OpenAI
Implements an AIConfig on the settings model side, to allow the user to specify which AI provider to use as their current active one (in the case that they have configured more than one LMProvider)

Note to reviewers:
The OpenAI implementation is largely the same as the Azure OpenAI one. The more "new" change in this PR is the AIConfig struct on the settings model side that allows the user to specify which provider is the active one, as well as the logic in TerminalPage for how we update the current active provider based on settings changes

Validation Steps Performed

Able to set OpenAI as the active provider
OpenAI works in Terminal Chat

PR Checklist

Closes #xxx
Tests added/passed
Documentation updated
- If checked, please file a pull request on our docs repo and link it here: #xxx
Schema updated (if necessary)

src/cascadia/QueryExtension/Microsoft.Terminal.Query.Extension.vcxproj

@@ -56,6 +56,9 @@
    <ClInclude Include="AzureLLMProvider.h">
      <DependentUpon>AzureLLMProvider.idl</DependentUpon>
    </ClInclude>
+    <ClInclude Include="OpenAILLMProvider.h">


src/cascadia/TerminalSettingsEditor/AISettingsViewModel.cpp

+        _NotifyChanges(L"IsOpenAIKeySet");
+    }
+
+    bool AISettingsViewModel::AzureOpenAIIsActive()


src/cascadia/QueryExtension/OpenAILLMProvider.cpp

@@ -0,0 +1,129 @@
+// Copyright (c) Microsoft Corporation.


src/cascadia/QueryExtension/OpenAILLMProvider.h

@@ -0,0 +1,48 @@
+// Copyright (c) Microsoft Corporation.


src/cascadia/QueryExtension/OpenAILLMProvider.idl

@@ -0,0 +1,17 @@
+// Copyright (c) Microsoft Corporation.


src/cascadia/QueryExtension/Microsoft.Terminal.Query.Extension.vcxproj

@@ -96,6 +102,9 @@
    <Midl Include="AzureLLMProvider.idl">
      <SubType>Code</SubType>
    </Midl>
+    <Midl Include="OpenAILLMProvider.idl">


src/cascadia/TerminalSettingsEditor/AISettings.xaml

+                                </Button>
+                                <Button Click="SetAzureOpenAIActive_Click"
+                                        HorizontalAlignment="Center"
+                                        Visibility="{x:Bind mtu:Converters.InvertedBooleanToVisibility(ViewModel.AzureOpenAIIsActive), Mode=OneWay}">


src/cascadia/TerminalSettingsEditor/AISettings.xaml

+                                </Button>
+                                <Button Click="SetOpenAIActive_Click"
+                                        HorizontalAlignment="Center"
+                                        Visibility="{x:Bind mtu:Converters.InvertedBooleanToVisibility(ViewModel.OpenAIIsActive), Mode=OneWay}">


src/cascadia/TerminalSettingsEditor/AISettingsViewModel.cpp

+    void AISettingsViewModel::SetAzureOpenAIActive()
+    {
+        _Settings.GlobalSettings().AIInfo().ActiveProvider(Model::LLMProvider::AzureOpenAI);
+        _NotifyChanges(L"AzureOpenAIIsActive");


src/cascadia/TerminalSettingsEditor/AISettingsViewModel.cpp

+    {
+        _Settings.GlobalSettings().AIInfo().ActiveProvider(Model::LLMProvider::AzureOpenAI);
+        _NotifyChanges(L"AzureOpenAIIsActive");
+        _NotifyChanges(L"OpenAIIsActive");


src/cascadia/QueryExtension/OpenAILLMProvider.cpp

PankajBhojwani · 2024-07-09T23:54:17Z

src/cascadia/TerminalSettingsModel/AIConfig.cpp

+    const auto val{ _getActiveProviderImpl() };
+    if (val)
+    {
+        // an active provider was explicitly set, return that
+        return *val;
+    }


This does mean that you can set an active provider even if you have not configured that provider - in this case when you try to use the Chat the provider will return the appropriate error. For example, the OpenAI provider will still make the http request and we get an error message back that no api key was provided.

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp

src/cascadia/TerminalSettingsModel/AIConfig.cpp

src/cascadia/QueryExtension/OpenAILLMProvider.cpp

src/cascadia/TerminalSettingsEditor/AISettingsViewModel.idl

src/cascadia/TerminalSettingsEditor/AISettings.xaml

…m/microsoft/terminal into dev/pabhoj/featurellm_openai

carlos-zamora

Awesome! Nice work!

src/cascadia/TerminalSettingsEditor/AISettingsViewModel.cpp

adrastogi · 2024-07-16T20:37:14Z

src/cascadia/TerminalSettingsEditor/AISettings.xaml

+                                <RichTextBlock TextWrapping="WrapWholeWords"
+                                               MaxWidth="900">
+                                    <Paragraph>
+                                        <Run x:Name="AISettings_OpenAIDescriptionPart1" /><Hyperlink NavigateUri="https://go.microsoft.com/fwlink/?linkid=2251839"


For the OpenAI case, would we want to / need to link out to their terms?

I believe @nguyen-dows said he'll hunt down the ToU once we get closer to release!

src/cascadia/TerminalSettingsEditor/AISettings.xaml

src/cascadia/QueryExtension/OpenAILLMProvider.cpp

…nto dev/pabhoj/llm_provider_interface

lhecker

Biggest concern: The strong-this issue. Otherwise, it seems fine - just some smaller issues.

lhecker · 2024-10-16T18:38:01Z

src/cascadia/QueryExtension/OpenAILLMProvider.cpp

+
+    void OpenAILLMProvider::SetContext(const Extension::IContext context)
+    {
+        _context = context;


This is where we'd ideally take the argument by-copy and then move it into place. This lets the caller move the instance into the argument which you then move into the member.
(A very minor nit.)

Oh and: Moving only works if the argument is not const. If it's const it'll not fail to compile, because... C++ reasons. Instead, it'll silently just create a copy. Bu With high enough compiler warnings it'll warn you about it though, but idk if it's enabled for this project.

lhecker · 2024-10-16T18:39:35Z

src/cascadia/QueryExtension/OpenAILLMProvider.cpp

+        hstring message{};
+
+        // Make sure we are on the background thread for the http request
+        co_await winrt::resume_background();


This also needs the strong-this pointer for safety. You can use the weak/strong handling that we do elsewhere or hold onto a strong pointer the entire time. That's pretty much entirely up to you (the latter is probably better though).

lhecker · 2024-10-16T18:39:42Z

src/cascadia/QueryExtension/OpenAILLMProvider.cpp

+        // Send the request
+        try
+        {
+            const auto response = _httpClient.SendRequestAsync(request).get();


lhecker · 2024-10-16T18:40:44Z

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -129,6 +129,13 @@ namespace winrt::TerminalApp::implementation
            p.SetActionMap(_settings.ActionMap());
        }

+        // If the active LLMProvider changed, make sure we reinitialize the provider
+        const auto newProviderType = _settings.GlobalSettings().AIInfo().ActiveProvider();
+        if (_lmProvider && (newProviderType != _currentProvider))


Why does this depend on the existence of _lmProvider?

lhecker · 2024-10-16T18:43:57Z

src/cascadia/TerminalSettingsEditor/AISettings.cpp

+        if (!OpenAIKeyInputBox().Password().empty())
+        {
+            _ViewModel.OpenAIKey(OpenAIKeyInputBox().Password());
+            OpenAIKeyInputBox().Password(L"");


nit: Avoid calling getters repeatedly and instead assign them to variables. Aids in debugging and avoids unknown side-effects if the getter is non-trivial (e.g. computes the return value, or worse, fetches it from a database, etc.; the PasswordVault getter is a good example).

lhecker · 2024-10-16T18:46:06Z

src/cascadia/TerminalSettingsModel/AIConfig.cpp

+    PasswordVault vault;
+    PasswordCredential cred;
+    // Retrieve throws an exception if there are no credentials stored under the given resource so we wrap it in a try-catch block
+    try
+    {
+        cred = vault.Retrieve(PasswordVaultResourceName, credential);
+    }
+    catch (...)
+    {
+        return L"";
+    }
+    return cred.Password();


This seems expensive. Why not cache it on first load? Should the error be logged to aid in debugging?

PankajBhojwani added 15 commits June 6, 2024 14:40

got an idl

b44216b

works

e78e4d0

allow

ac83d76

conflict

4b944e9

use id here too

ef406ee

have terminal page initialize the llmprovider

4fb4ca4

format

e5afbae

consts

32b3d68

works on palette side

1700a92

open ai in settings, llmprovider enum

08dd951

aiconfig struct

1934b30

move more things to ai info

96a489a

active provider buttons

8e560e2

rename to lmprovider

f9e9326

spelling conflict

4133239

github-advanced-security bot found potential problems Jul 9, 2024

View reviewed changes

PankajBhojwani added 2 commits July 9, 2024 15:33

conflicts

e545ca2

spell

dfad8d9

github-advanced-security bot found potential problems Jul 9, 2024

View reviewed changes

src/cascadia/QueryExtension/OpenAILLMProvider.cpp Fixed Show fixed Hide fixed

This comment has been minimized.

Sign in to view

array of accepted models

5139b88

PankajBhojwani commented Jul 9, 2024

View reviewed changes

cleanup this comment

dfbdc75

PankajBhojwani marked this pull request as ready for review July 9, 2024 23:54

This comment has been minimized.

Sign in to view

carlos-zamora reviewed Jul 12, 2024

View reviewed changes

first round of comments

1f305ab

This comment has been minimized.

Sign in to view

combine setter/getter

242b964

This comment has been minimized.

Sign in to view

Merge branch 'dev/pabhoj/llm_provider_interface' of https://github.co…

0094db0

…m/microsoft/terminal into dev/pabhoj/featurellm_openai

This comment has been minimized.

Sign in to view

carlos-zamora approved these changes Jul 15, 2024

View reviewed changes

src/cascadia/TerminalSettingsEditor/AISettingsViewModel.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalSettingsEditor/AISettingsViewModel.cpp Outdated Show resolved Hide resolved

auth related functions

6479c47

adrastogi reviewed Jul 16, 2024

View reviewed changes

src/cascadia/TerminalSettingsEditor/AISettings.xaml Outdated Show resolved Hide resolved

adrastogi reviewed Jul 16, 2024

View reviewed changes

src/cascadia/QueryExtension/OpenAILLMProvider.cpp Show resolved Hide resolved

ChrisYin mentioned this pull request Jul 17, 2024

[Scenario]: Terminal Chat #15000

Open

PankajBhojwani added 5 commits July 17, 2024 13:26

dispatcher

d392aab

domain check

8faf8b4

hot reload works now

dd6b46d

conflict and updates to openAI provider

4f81775

password box

46ab508

This comment has been minimized.

Sign in to view

logic for updating providers based on settings changes

ac1b4a3

This comment has been minimized.

Sign in to view

PankajBhojwani added 5 commits July 19, 2024 13:03

use static strings for strings used more than once

fc8e36d

Merge branch 'feature/llm' of https://github.com/microsoft/terminal i…

fdee6c2

…nto dev/pabhoj/llm_provider_interface

merge

fff97a4

don't use &

45ce94d

update with base changes

c89a306

zadjii-msft added this to the Terminal v1.23 milestone Aug 22, 2024

Base automatically changed from dev/pabhoj/llm_provider_interface to feature/llm September 6, 2024 03:01

PankajBhojwani added 3 commits September 9, 2024 13:59

conflict

de88842

missed this

edf08a9

newline...

830e655

PankajBhojwani mentioned this pull request Oct 11, 2024

Github Copilot ship list #18035

Open

6 tasks

carlos-zamora approved these changes Oct 14, 2024

View reviewed changes

lhecker reviewed Oct 16, 2024

View reviewed changes

conflict

16d3035

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow OpenAI to be used with Terminal Chat #17540

Allow OpenAI to be used with Terminal Chat #17540

PankajBhojwani commented Jul 9, 2024 •

edited

Loading

This comment has been minimized.

PankajBhojwani Jul 9, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

carlos-zamora left a comment

adrastogi Jul 16, 2024

PankajBhojwani Jul 16, 2024

This comment has been minimized.

This comment has been minimized.

lhecker left a comment

lhecker Oct 16, 2024

lhecker Oct 16, 2024

lhecker Oct 16, 2024

lhecker Oct 16, 2024

lhecker Oct 16, 2024

lhecker Oct 16, 2024 •

edited

Loading

lhecker Oct 16, 2024

Allow OpenAI to be used with Terminal Chat #17540

Are you sure you want to change the base?

Allow OpenAI to be used with Terminal Chat #17540

Conversation

PankajBhojwani commented Jul 9, 2024 • edited Loading

Summary of the Pull Request

Validation Steps Performed

PR Checklist

This comment has been minimized.

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

lhecker 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

lhecker Oct 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PankajBhojwani commented Jul 9, 2024 •

edited

Loading

lhecker Oct 16, 2024 •

edited

Loading