Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Import gRPC skills #730

Merged
merged 2 commits into from
May 1, 2023

Conversation

SergeyMenshykh
Copy link
Member

Motivation and Context

This change adds functionality required for gRPC skills import to Kernel allowing SK SDK client code to register and use gRPC skills in SK SDK.

Description

This change adds a few ImportGrpc* Kernel extension methods that can import gRPC skills from a folder or a file. Additionality, GrpcOperationRunner is changed to accept gRPC request message as JSON and return the gRPC method result as JSON as well.

Contribution Checklist

@SergeyMenshykh SergeyMenshykh added the PR: ready for review All feedback addressed, ready for reviews label Apr 28, 2023
@cchighman
Copy link
Member

I'm new to SK and just learning its architecture and looking for learning opportunities to question my asumptions.

I was curious if it might make sense to separate this into a skill and a connector. For example, the OperationRunner would represent a skill that's leveraging a gRPC connector. By packaging the entire piece into a skill, I'm wondering if that might make the overall use less extensible or confusing to users who are setting connection properties for a skill or trying to connect to gRPC and not finding a connector.

@cchighman
Copy link
Member

As an example, consider dotnet/src/SemanticKernel/Connectors/WebApi/Rest/RestApiOperationRunner.cs which is a REST connector and has its "OperationRunner" as a connector. My impression as a user would be to expect that other connector protocols would follow suit.

The OpenAI skill, for example, makes use of the REST OperationRunner as a connector in /dotnet/src/Skills/Skills.OpenAPI/Extensions/KernelOpenApiExtensions.cs.

@shawncal
Copy link
Member

As an example, consider dotnet/src/SemanticKernel/Connectors/WebApi/Rest/RestApiOperationRunner.cs which is a REST connector and has its "OperationRunner" as a connector. My impression as a user would be to expect that other connector protocols would follow suit.

The OpenAI skill, for example, makes use of the REST OperationRunner as a connector in /dotnet/src/Skills/Skills.OpenAPI/Extensions/KernelOpenApiExtensions.cs.

@cchighman Good observation. If I understand, you're referring more to the the packaging (nuget) and namespacing than the actual architecture. Reason for the different approach:

  • we figured RestApi was important and useful enough to bring it into the SemanticKernel Core. This is easy because it only uses official dotnet libraries (HttpClient, etc) that are already included in the Core.
  • with GRPC, we rely on other libraries that cannot be packaged/imported into Core, so it has to be in another (optional) package. The best fit for that would be in a Skills.* package, but the effect is that everything in there ends up being a "Skill.something" to match the package name, so it introduces an inconsistency with the RestAPI version.

Thoughts for @SergeyMenshykh

  • let's get this in as-is, to enable users that have been asking for it, and encourage the community to start piling on additional support.
  • let's review naming and packaging this week, consider splitting it into two packages (Skills. and Connectors. as @cchighman suggests) or consider a bigger change that puts both under a neutral top level namespace - i.e. MS.SK.Adapters.GRPCSkill MS.SK.Adapters.GRPCRunner) and make the same change to RestAPI.

Sound good to you @cchighman? Thanks for the great observation!

@shawncal
Copy link
Member

shawncal commented May 1, 2023

Merge to feature-grpc branch

@shawncal shawncal merged commit a02707a into microsoft:feature-grpc May 1, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: ready for review All feedback addressed, ready for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants