.Net: Feat: adds support for Microsoft Manifest in dotnet and other fixes. #9436

baywet · 2024-10-25T19:40:11Z

Motivation and Context

Adds support for Microsoft Plugins Manifest to semantic kernel in dotnet.
Fixes performance bottleneck for API Manifest loading.
Fixes broken integration tests for API manifest loading.
Adds an OpenAPI operation id normalization visitor to replace . by _ so function names are valid.
Fixes performance bottleneck in OpenAPI operation loading.
Fixes experimental ID for API manifest from SKEXP0043 to SKEXP0040 after the renumbering
Upgrades OAI.net and ApiManifest references.

Description

Contribution Checklist

The code builds clean without any errors or warnings
The PR follows the SK Contribution Guidelines and the pre-submission formatting script raises no violations
All unit tests pass, and I have added new tests where possible
I didn't break anyone 😄

On the unit tests: I'd like guidance on where to add unit tests for:

API Manifest loading (wasn't done at the time)
Microsoft Manifest loading.
Operation Id Normalization Visitor

Why so many files?

./kiota: all generated files, it contains kiota workspace configuration, which comes with copies of the OAS descriptions, etc… You don’t need to review it manually. It is useful because it allows us to automatically generate/refresh the integration tests plugins. If you feel like it's adding too much noise, we can remove those, we'll loose the ability to refresh the plugins definitions. https://aka.ms/kiota
Samples/concepts: restored the api manifest for astronomy API to fix the API manifest integration test. Added sample Microsoft Manifest for the new integration tests. Those are automatically generated via kiota and can be automatically refreshed later.
Src/Functions: Microsoft manifests implementation, API manifest fixes.

How to run the local tests

Create the following JSON file D:\github\semantic-kernel\dotnet\samples\Concepts\bin\Debug\net8.0\appsettings.Development.json (for some reason given how the project is setup this file is not being copied automatically. I didn't to touch any of the project setup out of fear of breaking other things)

{
  "MSGraph": {
    "ClientId": "clientId",
    "TenantId": "tenantId",
    "Scopes": [
      "Calendars.Read",
      "Contacts.Read",
      "Files.Read.All",
      "Mail.Read",
      "User.Read"
    ],
    "RedirectUri": "http://localhost"
  }
}

Replace the clientId and TenantId by your own value.
To create the application registration, go to https://aad.portal.azure.com, create a new application registration, new public client (add the redirect URI). In API access, add the listed Microsoft Graph delegated scopes. Grant consent after adding the scopes. During the first run, your browser will open to get the token.

File paths and copies

Like for the development settings, the project is NOT copying the sample plugin files for some reason. This is why the loading of the files in the integration tests looks at source files directly with ../../../ path segments. Happy to review that upon guidance.

License for Astrology plugins

The description is under the Apache license. I added the plugin (API and Microsoft) to restore the integration test for the former and mirror the setup to the latter. In the case of API plugins, we're only referring to it, so having a plugin is fine. In case of the Microsoft plugin, we have a full copy under the kiota configuration directory, and a sliced down version (derived work) in the example plugin. The value of this API is that it allows us to test scenarios outside of Microsoft Graph. But if the license is a challenge, we can remove those before merging. @RogerBarreto to provide more context on why those were deleted at the first place in #6005

Why so many commits?

Incremental work during the implementation, plus regular merges from main to make sure everything was current and we wouldn't end up with conflicts, etc... Happy to rebase and squash once the initial reviews are through.

Signed-off-by: Vincent Biret <[email protected]>

fix: loading of location OAS descriptions for Microsoft manifests Signed-off-by: Vincent Biret <[email protected]>

Signed-off-by: Vincent Biret <[email protected]>

…t manifests Signed-off-by: Vincent Biret <[email protected]>

Signed-off-by: Vincent Biret <[email protected]>

zengin · 2024-10-25T20:10:14Z

.kiota/apimanifest.json

+      }
+    },
+    "Astronomy": {
+      "apiDescriptionUrl": "https://raw.githubusercontent.com/zengin/openapi-directory/zengin/nasa/APIs/nasa.gov/apod/1.0.0/openapi.yaml",


I think we should not take a dependency on my account here. Is there any chance this file can be moved to a Microsoft owned location? I think I needed a copy for adding auth details.

Sure, we can point it somewhere else. Do you remember where you took it from?

SergeyMenshykh · 2024-10-30T10:37:57Z

.kiota/apimanifest.json

@@ -0,0 +1,79 @@
+{
+  "applicationName": "application",


Consider moving all the documents from the .kiota folder closer to the code that uses them. If those are required for plugin generation, please move them to the plugins folder and add a README describing how to generate those plugins from the documents.

SergeyMenshykh · 2024-10-30T10:50:30Z

dotnet/samples/Concepts/Concepts.csproj

-    <ProjectReference Include="..\..\src\Connectors\Connectors.Memory.Sqlite\Connectors.Memory.Sqlite.csproj" />
-    <ProjectReference Include="..\..\src\Connectors\Connectors.Memory.Weaviate\Connectors.Memory.Weaviate.csproj" />
+    <ProjectReference
+      Include="..\..\src\Connectors\Connectors.HuggingFace\Connectors.HuggingFace.csproj" />


Please review the changes in this file and others, and roll back any unnecessary ones, such as those related to formatting that move the Include attribute to a new line.

SergeyMenshykh · 2024-10-30T11:16:49Z

dotnet/Directory.Packages.props

+    <PackageVersion Include="Microsoft.Extensions.Configuration.EnvironmentVariables"
+      Version="8.0.0" />


Revert unnecessary changes.

SergeyMenshykh · 2024-10-30T11:34:28Z

dotnet/samples/Concepts/Plugins/MicrosoftManifestBasedPlugins.cs

+        #endregion
+    ];
+    [Theory, MemberData(nameof(s_parameters))]
+    public async Task RunSampleWithPlannerAsync(string pluginToTest, string functionToTest, KernelArguments? arguments, params string[] pluginsToLoad)


This sample, like the ApiManifest one, doesn’t use Planner as far as I can tell. I suggest renaming them to avoid the Planner keyword, like RunMicrosoftManifestPlugin and RunApiManifestPlugin.

SergeyMenshykh · 2024-10-30T11:49:17Z

dotnet/samples/Concepts/Plugins/MicrosoftManifestBasedPlugins.cs

+        ["ContactsPlugin", "me_ListContacts", new KernelArguments() { { "_count", "true" } }, "ContactsPlugin", "MessagesPlugin"],
+        ["CalendarPlugin", "me_calendar_ListEvents", new KernelArguments() { { "_top", "1" } }, "CalendarPlugin", "MessagesPlugin"],
+
+        #region Multiple API dependencies (multiple auth requirements) scenario within the same plugin


Is the region really needed here? Won't an XML comment suffice?

SergeyMenshykh · 2024-10-30T12:59:49Z

dotnet/src/Functions/Functions.OpenApi/DocumentLoader.cs

+        var response = await LoadDocumentResponseFromUriAsync(uri, logger, httpClient, authCallback, userAgent, cancellationToken).ConfigureAwait(false);
+        return await response.Content.ReadAsStreamAndTranslateExceptionAsync().ConfigureAwait(false);


The response is not disposed here; consider wrapping the response and the stream into HttpResponseStream the same way it's done in the RestApiOperationRunner.

SergeyMenshykh · 2024-10-30T13:01:43Z

dotnet/src/Functions/Functions.OpenApi/DocumentLoader.cs

@@ -58,10 +77,34 @@ internal static async Task<string> LoadDocumentFromFilePathAsync(
 #endif
            ).ConfigureAwait(false);
    }
+    private static void CheckIfFileExists(string filePath)


Please add new line between the methods and move the private to the bottom to keep internal methods as a group above.

SergeyMenshykh · 2024-10-30T13:34:08Z

dotnet/src/Functions/Functions.OpenApi.Extensions/Extensions/ApiManifestKernelExtensions.cs

I would also consider moving these kernel extensions and the MicrosoftManifestKernelExtensions ones to the Microsoft.SemanticKernel namespace so that they can be easily discovered by consumers on kernel when the extension package is installed.

SergeyMenshykh · 2024-10-30T13:46:09Z

...t/src/Functions/Functions.OpenApi.Extensions/Extensions/MicrosoftManifestPluginParameters.cs

+/// <summary>
+/// API manifest plugin parameters.
+/// </summary>
+public class MicrosoftManifestPluginParameters


The XML comment is not relevant to the class + consider sealing class.

SergeyMenshykh · 2024-10-30T13:54:32Z

...t/src/Functions/Functions.OpenApi.Extensions/Extensions/MicrosoftManifestPluginParameters.cs

+    /// <param name="httpClient">Http client to be used in plugin initialization phase.</param>
+    /// <param name="userAgent">User agent to be used in plugin initialization phase.</param>
+    /// <param name="functionExecutionParameters">A map of function execution parameters.</param>
+    public MicrosoftManifestPluginParameters(


Working on SK for a long period of time, we've noticed that the number of constructor parameters for the parameter/metadata/options classes tends to grow over time, and it is quite difficult to add new ones and keep existing in logical order without introducing breaking changes. So, my advice would be to remove the constructor and use props only. Adding a new property would be just one line without the necessity of changing the constructor and worrying about potential breaking changes.

SergeyMenshykh · 2024-10-31T13:13:34Z

dotnet/src/Functions/Functions.OpenApi.Extensions/Extensions/ApiManifestKernelExtensions.cs

Does MicrosoftManifest supersede the ApiManifest? If that is the case, should the ApiManifest functionality be obsoleted or removed?

zengin and others added 30 commits June 18, 2024 16:10

import implementation for TypeB

ee3312b

update the manifest package and address compiler warnings

8f9bd38

address compiler warnings

987c3bc

isolate runtimes in error handling

b93e6ac

Merge branch 'main' into chore/zengin-manifest

b1320fe

chore: upgrades plugins library version

177e126

chore: updates api manifest dependency

99ac5cc

Signed-off-by: Vincent Biret <[email protected]>

Merge branch 'main' into chore/zengin-manifest

0a4fedd

chore: formatting

f2ebca2

chore: upgrades manifest lib to latest version

e33690a

Merge branch 'main' into chore/zengin-manifest

6d462a8

ci: adds unit test infrastructure for microsoft manifest plugins

a379579

Signed-off-by: Vincent Biret <[email protected]>

ci: adds sample microsoft plugins

c77eb72

Signed-off-by: Vincent Biret <[email protected]>

chore: adds kiota configuration for sample microsoft plugins

20435c6

Signed-off-by: Vincent Biret <[email protected]>

fix: path to microsoft manifest plugin

8dd2906

Signed-off-by: Vincent Biret <[email protected]>

fix: adds missing copy of microsoft manifests

11d2391

Signed-off-by: Vincent Biret <[email protected]>

docs: adds link to microsoft manifest sample

2c7cc8c

Signed-off-by: Vincent Biret <[email protected]>

fix: lookup path for the api and microsoft plugins

d47a812

Signed-off-by: Vincent Biret <[email protected]>

fix: loading of local microsoft manifest file

c98cbcc

fix: loading of location OAS descriptions for Microsoft manifests Signed-off-by: Vincent Biret <[email protected]>

fix: stream instead of string to load microsoft plugins

529dc0d

Signed-off-by: Vincent Biret <[email protected]>

fix: function name for microsoft plugins test data

d98ba29

Signed-off-by: Vincent Biret <[email protected]>

fix: authentication for microsoft plugins

d435b51

Signed-off-by: Vincent Biret <[email protected]>

Merge branch 'main' into chore/zengin-manifest

65543ed

fix: restores wrongly deleted plugin in microsoft#6005

7fa549c

feat: adds astronomy plugin to kiota workspace configuration

814a45c

Signed-off-by: Vincent Biret <[email protected]>

feat: adds astronomy plugin definition

a6e9529

Signed-off-by: Vincent Biret <[email protected]>

fix: nasa astronomy simple test

927723d

Signed-off-by: Vincent Biret <[email protected]>

feat: adds support for multiple microsoft plugins dependencies

882ed3b

Signed-off-by: Vincent Biret <[email protected]>

docs: adds experimental entry for Microsoft Manifests

3e0c0c3

Signed-off-by: Vincent Biret <[email protected]>

fix: aligns experimental code with new numbering for api and microsof…

3471e31

…t manifests Signed-off-by: Vincent Biret <[email protected]>

baywet added 7 commits October 25, 2024 12:22

chore: skip loading the json document ourselves

2b7d722

Signed-off-by: Vincent Biret <[email protected]>

fix: o(n) lookup to o(1) for operation ids to exclude

b2c2ee4

Signed-off-by: Vincent Biret <[email protected]>

fix: disposing response too early

0707600

Signed-off-by: Vincent Biret <[email protected]>

fix: multiple performance issues in API manifest loading

3b2c3c2

Signed-off-by: Vincent Biret <[email protected]>

fix: normalization operation id to avoid . in function names

5fdd657

Signed-off-by: Vincent Biret <[email protected]>

chore: linting

8871dcf

fix; netstandard2.0 build

93eb850

Signed-off-by: Vincent Biret <[email protected]>

baywet requested a review from a team as a code owner October 25, 2024 19:40

markwallace-microsoft added .NET Issue or Pull requests regarding .NET code documentation labels Oct 25, 2024

github-actions bot changed the title ~~Feat: adds support for Microsoft Manifest in dotnet and other fixes.~~ .Net: Feat: adds support for Microsoft Manifest in dotnet and other fixes. Oct 25, 2024

baywet temporarily deployed to integration October 25, 2024 19:40 — with GitHub Actions Inactive

Merge branch 'main' into chore/zengin-manifest

0fe5fa6

baywet temporarily deployed to integration October 25, 2024 19:56 — with GitHub Actions Inactive

zengin reviewed Oct 25, 2024

View reviewed changes

SergeyMenshykh reviewed Oct 30, 2024

View reviewed changes

SergeyMenshykh reviewed Oct 31, 2024

View reviewed changes

SergeyMenshykh mentioned this pull request Oct 31, 2024

.Net: Support for Type B plugins #6614

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

.Net: Feat: adds support for Microsoft Manifest in dotnet and other fixes. #9436

.Net: Feat: adds support for Microsoft Manifest in dotnet and other fixes. #9436

baywet commented Oct 25, 2024 •

edited

Loading

zengin Oct 25, 2024

baywet Oct 25, 2024

SergeyMenshykh Oct 30, 2024

SergeyMenshykh Oct 30, 2024

SergeyMenshykh Oct 30, 2024

SergeyMenshykh Oct 30, 2024

SergeyMenshykh Oct 30, 2024

SergeyMenshykh Oct 30, 2024

SergeyMenshykh Oct 30, 2024

SergeyMenshykh Oct 30, 2024

SergeyMenshykh Oct 30, 2024

SergeyMenshykh Oct 30, 2024

SergeyMenshykh Oct 31, 2024

		<PackageVersion Include="Microsoft.Extensions.Configuration.EnvironmentVariables"
		Version="8.0.0" />

		var response = await LoadDocumentResponseFromUriAsync(uri, logger, httpClient, authCallback, userAgent, cancellationToken).ConfigureAwait(false);
		return await response.Content.ReadAsStreamAndTranslateExceptionAsync().ConfigureAwait(false);

.Net: Feat: adds support for Microsoft Manifest in dotnet and other fixes. #9436

Are you sure you want to change the base?

.Net: Feat: adds support for Microsoft Manifest in dotnet and other fixes. #9436

Conversation

baywet commented Oct 25, 2024 • edited Loading

Motivation and Context

Description

Contribution Checklist

Why so many files?

How to run the local tests

File paths and copies

License for Astrology plugins

Why so many commits?

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

baywet commented Oct 25, 2024 •

edited

Loading