OpenAPI skills MVP #188

SergeyMenshykh · 2023-03-28T19:27:35Z

Motivation and Context

The change lay foundation for OpenApi skills support by introducing a set of classes for importing, parsing OpenApi documents and sending HTTP requests to the resources described in them.

Description

The PR adds the following classes/methods:

ImportOpenApiFromDirectoryExtension – discovers OpenAPI document in a folder, creates SKFunction per REST operation in the document, and registers the functions in the Kernel.
RegisterOpenApiSkill - reates SKFunction per REST operation in the document, and registers the functions in the Kernel.
OpenApiDocumentParser – iterates over the collection of paths in OpenAPI document and creates an instance of RestOperation model class for each of them.
RestOperationRunner – accepts an instance of the RestOperation class and collection of arguments. Gets all required components (url, body, headers, …) for HTTP request and sends the request.
A few other model, exception, and helper classes.

Contribution Checklist

The code builds clean without any errors or warnings
The PR follows SK Contribution Guidelines (https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
The code follows the .NET coding conventions (https://learn.microsoft.com/dotnet/csharp/fundamentals/coding-style/coding-conventions) verified with dotnet format
All unit tests pass, and I have added new tests where possible
I didn't break anyone 😄

shawncal · 2023-03-28T19:50:24Z

dotnet/src/SemanticKernel.Skills/Skills.OpenAPI/Skills/LocalOpenApiSkillsProvider.cs

+/// <summary>
+/// OpenApi skill provider.
+/// </summary>
+public static class LocalOpenApiSkillsProvider


Why is the skill/class named "Local*"? Couldn't this just be an OpenApiSkillProvider with LoadFromResource, LoadFromFilePath, LoadFromUrl...?
...or even better, just include these three types in the Kernel extensions.

shawncal · 2023-03-28T20:01:28Z

dotnet/src/SemanticKernel.Skills/Skills.OpenAPI/Model/RestOperation.cs

+/// <summary>
+/// The Rest operation.
+/// </summary>
+internal class RestOperation


Use of the term "Rest" across the PR is giving me a weird feeling. I'm not sure why; it's not invalid. Maybe it's because it's "Rest" and not "REST".
I think, before merging to main, we should have a quick naming conversation and apply it everywhere. Other options: "RestApiOperation", "WebApiOperation"

umeshma · 2023-03-28T20:06:51Z

dotnet/src/SemanticKernel.Skills/Skills.OpenAPI/Model/RestParameterType.cs

Parameter types are already part of the Microsoft.OpenApi SDK. All defined here.
Microsoft.OpenApi.Models.ParameterLocation

We will evaluate this change in a (near) future change.

shawncal · 2023-03-28T20:09:08Z

dotnet/src/SemanticKernel.Skills/Skills.OpenAPI/Extensions/KernelOpenApiExtensions.cs

+/// </summary>
+public static class KernelOpenApiExtensions
+{
+    public static IDictionary<string, ISKFunction> RegisterOpenApiSkill(this IKernel kernel, Stream documentStream, string skillName)


We'll want a overload of this that loads directly from a json url, and one from a local file json path (maybe these are the same... both string paths)

For this review into the feature branch, I am okay leaving this to just the local and adding URL loading in a future change.

umeshma · 2023-03-28T20:14:13Z

dotnet/src/SemanticKernel.Skills/Skills.OpenAPI/Model/RestParameter.cs

There is already a OpenApiParameter object.
Microsoft.OpenApi.Models.OpenApiParameter

@SergeyMenshykh - Umesh has a good point. Though, to keep other work moving forward, I'm happy to bring this into the feature branch as-is if we look at replacing these models with Microsoft.OpenApi before merging to main. Unless you have other reasons to roll our own.

We will evaluate this change in a (near) future change.

umeshma · 2023-03-28T20:15:34Z

dotnet/src/SemanticKernel.Skills/Skills.OpenAPI/Model/RestOperation.cs

There is already a OpenApiOperation object.
Microsoft.OpenApi.Models.OpenApiOperation

We will evaluate this change in a (near) future change.

umeshma · 2023-03-28T20:19:11Z

dotnet/src/SemanticKernel.Skills/Skills.OpenAPI/OpenApi/OpenApiDocumentParser.cs

Why duplicate all the parameter information into internal objects?
I can see why you may want a REST operation, but why duplicate all the parameters and options? Just hold onto all the metadata types already defined in Microsoft.OpenAPI - keep a reference to them.

We will evaluate this change in a (near) future change.

dotnet/src/SemanticKernel.Skills/Skills.OpenAPI/Skills/LocalOpenApiSkillsProvider.cs

samples/dotnet/kernel-syntax-examples/Example16_OpenApiSkill.cs

umeshma · 2023-03-28T20:20:38Z

dotnet/src/SemanticKernel.Skills/Skills.OpenAPI/Rest/RestOperationRunner.cs

Where will retry etc. get applied for the HttpClient?

Good callout. This is going into a feature branch for iteration and we'll be taking a look at robustness before merging into main.

adrianwyatt · 2023-03-28T19:56:14Z

dotnet/src/SemanticKernel.Skills/Skills.OpenAPI/Authentication/BearerTokenHandler.cs

+        //Example of auth routing - adding auth info to the Http request message based on host name.
+        var host = requestMessage.RequestUri.Host;
+
+        if (host.Contains("dev-tests.vault.azure.net", StringComparison.InvariantCultureIgnoreCase))


What is this address? Could you add a quick comment if the intention is to have this replaced.

adrianwyatt · 2023-03-28T19:56:54Z

dotnet/src/SemanticKernel.Skills/Skills.OpenAPI/Authentication/BearerTokenHandler.cs

+
+        if (host.Contains("dev-tests.vault.azure.net", StringComparison.InvariantCultureIgnoreCase))
+        {
+            requestMessage.Headers.Authorization = new AuthenticationHeaderValue("Bearer", "<token>");


Please add an all caps comment to never check a token into any branch of git, local or otherwise.

dotnet/src/SemanticKernel.Skills/Skills.OpenAPI/Authentication/IAuthenticationHandler.cs

adrianwyatt · 2023-03-28T20:01:59Z

dotnet/src/SemanticKernel.Skills/Skills.OpenAPI/Extensions/KernelOpenApiExtensions.cs

+            catch (Exception ex) when (!ex.IsCriticalException())
+            {
+                //Logging the exception and keep registering other Rest functions
+                kernel.Log.LogWarning(ex, "Something went wrong while rendering the Rest function. Function: {0}.{1}. Error: {2}", skillName, operation.Id, ex.Message);


Why not throw?

adrianwyatt · 2023-03-28T20:34:57Z

samples/dotnet/kernel-syntax-examples/Example16_OpenApiSkill.cs

+        //Use OpenApi skill from Skills.OpenAPI package
+        kernel.RegisterOpenApiSkill(LocalOpenApiSkillsProvider.LoadFroResource(LocalOpenApiSkillsProvider.AzureKeyVault), LocalOpenApiSkillsProvider.AzureKeyVault);
+
+        var plan = await kernel.RunAsync("Load 'test-secret' from Azure KeyValut available at https://dev-tests.vault.azure.net using GetSecret function.", planner["CreatePlan"]);


In the future I would recommend not using the planner for this particular example, so as to keep the concept count limited to what we are trying to show.

shawncal · 2023-03-29T03:19:48Z

@SergeyMenshykh and @adrianwyatt Now that this is in the feature branch (which is great!) I've un-resolved all the unresolved comments so that they can be addressed in a pre-merge PR. Please make sure to note them (or they'll likely all come up again when merging to main).

### Motivation and Context The change lay foundation for OpenApi skills support by introducing a set of classes for importing, parsing OpenApi documents and sending HTTP requests to the resources described in them. ### Description The PR adds the following classes/methods: - ImportOpenApiFromDirectoryExtension – discovers OpenAPI document in a folder, creates SKFunction per REST operation in the document, and registers the functions in the Kernel. - RegisterOpenApiSkill - reates SKFunction per REST operation in the document, and registers the functions in the Kernel. - OpenApiDocumentParser – iterates over the collection of paths in OpenAPI document and creates an instance of RestOperation model class for each of them. - RestOperationRunner – accepts an instance of the RestOperation class and collection of arguments. Gets all required components (url, body, headers, …) for HTTP request and sends the request. - A few other model, exception, and helper classes.

SergeyMenshykh added 2 commits March 28, 2023 19:44

OpenAPI skills MVP

d12fe8a

private regions

1132b82

SergeyMenshykh requested review from shawncal, adrianwyatt and gitri-ms March 28, 2023 19:28

shawncal and others added 2 commits March 28, 2023 12:29

Merge branch 'feature-openapi' into mvp-openapi-skills

49e2c94

Merge with latest

b608200

SergeyMenshykh added the PR: ready for review All feedback addressed, ready for reviews label Mar 28, 2023

shawncal reviewed Mar 28, 2023

View reviewed changes

umeshma reviewed Mar 28, 2023

View reviewed changes

shawncal reviewed Mar 28, 2023

View reviewed changes

umeshma reviewed Mar 28, 2023

View reviewed changes

shawncal reviewed Mar 28, 2023

View reviewed changes

dotnet/src/SemanticKernel.Skills/Skills.OpenAPI/Skills/LocalOpenApiSkillsProvider.cs Outdated Show resolved Hide resolved

shawncal reviewed Mar 28, 2023

View reviewed changes

samples/dotnet/kernel-syntax-examples/Example16_OpenApiSkill.cs Outdated Show resolved Hide resolved

umeshma reviewed Mar 28, 2023

View reviewed changes

Fix typo in PR: LoadFroResource

1540068

adrianwyatt added PR: feedback to address Waiting for PR owner to address comments/questions and removed PR: ready for review All feedback addressed, ready for reviews labels Mar 28, 2023

adrianwyatt requested changes Mar 28, 2023

View reviewed changes

SergeyMenshykh added 2 commits March 28, 2023 22:12

Addressing comments and merge

b432b5f

Addressing comments

7e134e8

SergeyMenshykh added the PR: ready for review All feedback addressed, ready for reviews label Mar 28, 2023

adrianwyatt approved these changes Mar 28, 2023

View reviewed changes

adrianwyatt merged commit 813e59f into microsoft:feature-openapi Mar 28, 2023

Bryan-Roe added a commit to BMR-Cloud-Dev/semantic that referenced this pull request Jun 28, 2024

Bump milvus from 2.3.5 to 2.3.8 in /python (microsoft#188)

ed8649f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenAPI skills MVP #188

OpenAPI skills MVP #188

SergeyMenshykh commented Mar 28, 2023

shawncal Mar 28, 2023

shawncal Mar 28, 2023

umeshma Mar 28, 2023 •

edited

Loading

adrianwyatt Mar 28, 2023

shawncal Mar 28, 2023

adrianwyatt Mar 28, 2023

umeshma Mar 28, 2023

adrianwyatt Mar 28, 2023 •

edited

Loading

adrianwyatt Mar 28, 2023

umeshma Mar 28, 2023

adrianwyatt Mar 28, 2023

umeshma Mar 28, 2023

adrianwyatt Mar 28, 2023

umeshma Mar 28, 2023

adrianwyatt Mar 28, 2023

adrianwyatt Mar 28, 2023

adrianwyatt Mar 28, 2023

adrianwyatt Mar 28, 2023

adrianwyatt Mar 28, 2023

shawncal commented Mar 29, 2023

OpenAPI skills MVP #188

OpenAPI skills MVP #188

Conversation

SergeyMenshykh commented Mar 28, 2023

Motivation and Context

Description

Contribution Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umeshma Mar 28, 2023 • 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

adrianwyatt Mar 28, 2023 • 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

shawncal commented Mar 29, 2023

umeshma Mar 28, 2023 •

edited

Loading

adrianwyatt Mar 28, 2023 •

edited

Loading