### Motivation and Context / Description
<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
1. Why is this change required?
2. What problem does it solve?
3. What scenario does it contribute to?
4. If it fixes an open issue, please link to the issue here.
-->
- The integration tests for Function Calling Stepwise Planner were
removed when we refactored/removed `Planners.Core`. This adds them back
and improves them with more test cases and results validation.
- Also includes a minor fix in sample code (`ChatHistory.AsJson()` no
longer exists)
### Contribution Checklist
<!-- Before submitting this PR, please make sure: -->
- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 😄
### Motivation and Context
<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
1. Why is this change required?
2. What problem does it solve?
3. What scenario does it contribute to?
4. If it fixes an open issue, please link to the issue here.
-->
Addresses multiple issues/regressions in the planner code
### Description
<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
- Remove model ID from prompt YAML, to allow planner to work with any
model ID
- Remove unnecessary `OpenAIChatMessageContent.Name` property - we were
never setting this property, and always just pulling the name value from
the function call metadata.
- Removing this also fixes an issue where we were putting an empty
string for 'name' rather than null, causing an OpenAI model exception.
- Fix incorrect ordering of parameters in example code
### Contribution Checklist
<!-- Before submitting this PR, please make sure: -->
- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 😄
---------
Co-authored-by: Gil LaHaye <[email protected]>
@matthewbolanos, @markwallace-microsoft, entirely up to you whether you
want to merge or close this; fine with me either way, I'm just opening
this to make it easy to decide. Fixes
https://github.com/microsoft/semantic-kernel/issues/4033.
This removes `KernelBuilder` and adds a static `CreateBuilder()` such
that instead of doing:
```C#
Kernel kernel = new KernelBuilder().Whatever().Build();
```
one would now do:
```C#
Kernel kernel = Kernel.CreateBuilder().Whatever().Build();
```
Everything else should be functionally the same.
---------
Co-authored-by: Roger Barreto <[email protected]>
- Replaces WithServices/WithPlugins callback-based methods with
Services/Plugins properties. It makes it less fluent, but it brings it
into line with the current best-practices patterns used by ASP.NET, it
reduces nesting, it avoids confusion about when the callbacks are
invoked, it avoids async-related issues, and it makes the pre-Build
syntax for adding plugins to the builder much closer to the post-Build
syntax for adding plugins to the Kernel.
- Plugins added as services are now automatically resolved as part of
Kernel construction. The Kernel will prefer to use a collection it's
explicitly handed, but if it isn't given one, it'll try to fetch a
collection from the services, and if it can't find that, it'll try to
fetch all IKernelPlugins from the services.
- Renamed With methods to Add methods to improve parity both with
current ASP.NET practices as well as between when doing `builder.AddXx`
and `builder.Services.AddXx`.
- Renamed `AddPluginFromObject<T>` to `AddFromType<T>`.
- Remove `new()` constraint from `AddFromType<T>` and instead use
ActivatorUtilities.CreateInstance. The KernelBuilder.Plugins property
contains the same IServiceCollection as the builder, and so
`AddFromType<T>` has access to it and can use it with CreateInstance to
resolve any plugin object ctor arguments from the service provider. For
example, HttpPlugin has a ctor that takes an HttpClient, so now doing
`builder.Plugins.AddFromType<HttpPlugin>()` will find an HttpClient in
the container and use it automatically when the plugins are built as
part of Build.
- Removed WithLoggerFactory and WithCulture. The latter is basically
never used, and it's much more like the Data property and event handlers
on the Kernel, where it can easily be mutated on the Kernel instance and
doesn't need any builder support. WithLoggerFactory was used a lot, but
mainly because of the patterns employed in the tests, and it's easily
replacable with AddLogging.
- Added IKernelBuilder interface. It's implemented by KernelBuilder and
returned from a new AddKernel extension method on IServiceCollection,
adding a singleton KernelPluginCollection and a transient Kernel to the
container. IKernelBuilder has both the Services and Plugins properties,
and also has Build, but Build throws an exception if it's used on the
instance returned from AddKernel, per feedback that it can lead to folks
accidentally building the container multiple times.
- Changed all of the extensions on KernelBuilder to instead by on
IKernelBuilder.
@davidfowl, @halter73, please ensure this is in line with your feedback.
Thanks!
---------
Co-authored-by: Mark Wallace <[email protected]>
Co-authored-by: SergeyMenshykh <[email protected]>
### Motivation and Context
<!-- Thank you for your contribution to the semantic-kernel repo!
Please help reviewers and future users, providing the following
information:
1. Why is this change required?
2. What problem does it solve?
3. What scenario does it contribute to?
4. If it fixes an open issue, please link to the issue here.
-->
### Description
<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
- Move kernel parameter to `ExecuteAsync` instead of constructor
- Use semantic function for initial plan generation, with prompt and
settings loaded from YAML
- Check estimated token count against max tokens specified for planner
### Contribution Checklist
<!-- Before submitting this PR, please make sure: -->
- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 😄
---------
Co-authored-by: Ben Thomas <[email protected]>
Co-authored-by: Mark Wallace <[email protected]>
### Motivation and Context
Closes#3756
### Description
<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
### Contribution Checklist
<!-- Before submitting this PR, please make sure: -->
- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 😄
Closes https://github.com/microsoft/semantic-kernel/issues/3694
Closes https://github.com/microsoft/semantic-kernel/issues/3693
Closes https://github.com/microsoft/semantic-kernel/issues/3659
Closes https://github.com/microsoft/semantic-kernel/issues/3571
Closes https://github.com/microsoft/semantic-kernel/issues/3191
Closes https://github.com/microsoft/semantic-kernel/issues/2463
(This replaces https://github.com/microsoft/semantic-kernel/pull/3676,
including that as a first commit. That resiliency functionality is no
longer necessary after this change, as developers can simply use
libraries like Microsoft.Extensions.Http.Resilience to achieve the
desired support.)
SK has had a custom IAIServiceProvider interface that's represented a
provider of AI related services. Kernel has then wrapped one of these,
but also wrapped other individual non-AI services, like ILoggerFactory
and HttpClient. This leads to a variety of problems, such as it being
more complicated to construct a Kernel instance, it not integrating as
nicely with a broader dependency injection system, and not being as
future proof because there isn't a good way to flow additional
non-AI-related services through the Kernel to anywhere that might want
access. On the DI front, it's also more difficult to integrate because
all of the KernelBuilder extension methods are specific to
KernelBuilder, and even though they're creating services, the helpers
aren't usable with IServiceCollection and thus with the rest of the DI
system; a developer needs to go directly to the underlying types and
register those directly.
There are also a handful of features that are of little value and in
some cases exposed but not actually implemented, such as setAsDefault
and alsoAsTextCompletion. And other cases where there's something
valuable but missing, such as the ability to create certain kinds of
Azure-related service instances with an existing OpenAIClient.
This PR attempts to address all of these issues and set up SK to evolve
more easily:
- Kernel now wraps an IServiceProvider, and all services (AI and non-AI)
are retrieved from it. This has a variety of benefits, including
future-proofing for wanting to flow more state around, better
integration with other systems (e.g.
Microsoft.Extensions.DependencyInjection), simpler construction, etc.
- KernelBuilder still has dedicated WithXx methods for first-class
features, but they're all layered on top of a new ConfigureServices
method, which lets you add anything to an IServiceCollection. Because
everything is based on IServiceCollection/IServiceProvider, dependencies
are automatically satisfied from the container. For example, for
services that accept an optional HttpClient, if one isn't specified to
the WithXx method, the system will pull one out of the DI container if
it can, which means all existing support for adding configured
HttpClient instances to an IServiceCollection immediately apply to
Kernel{Builder} as well.
- KernelBuilder also exposes a ConfigurePlugins method, largely for
parity with ConfigureServices, as fundmantally a Kernel is just the
combination of a collection of services and a collection of plugins
(plus some additional transient data, event handlers, etc.) This also
integrates with IServiceProvider, with one passed into the callback, so
that if the plugin itself needs access to something like logging, it can
simply get it from the container. These ConfigureServices and
ConfigurePlugins methods can be called any number of times.
- KernelBuilder is now just a simplified, opinionated way to achieve
something you can achieve in two other ways: construct the Kernel
directly, or construct the Kernel via DI. For the latter, you can simply
add Kernel as a DI resource, and it'll automatically be composed out of
what's available. For example, if in ASP.NET you
AddAzureOpenChatCompletion, AddLogging, AddHttp, etc., and then ask for
a Kernel to be injected, that Kernel will be constructed from the
IServiceProvider that contains all of those other resources, and thus
anyone using the Kernel will get that logger, will get that HttpClient,
etc.; the HttpClient will also be used by the Azure OpenAI chat
completion.
@matthewbolanos, @markwallace-microsoft, @SergeyMenshykh, please review
from an SK perspective to let me know if you agree with the direction,
whether there are specific things you'd like to see changed, etc. There
are some straggling things we should still do after this (like add a few
more extension methods for adding plugins to a plugin collection, adding
more tests, revamping XML comments, cleaning up more code, etc.), but
this is a sweeping change and I wanted to get it in.
@eerhardt, @halter73, I'd appreciate it if you could review this from a
DI perspective, e.g. are there variations in the patterns I should be
following, any best-practices I'm violating, whether I'm using singleton
when I should be using transient, etc. etc.
Note that I ran into one issue as part of this: the
Microsoft.Extensions.DependencyInjection support for keyed services
doesn't let you enumerate all services for a particular T, regardless of
key. That's something SK depends on. To workaround that, KernelBuilder
tracks all of the type/keys and injects a dictionary as a service with
that information; then places the Kernel needs it, it can query for that
dictionary and use it. This means that a Kernel constructed with
KernelBuilder will fully support keys, whereas a Kernel constructed with
DI or directly won't work as nicely with named services. My hope is that
M.E.DI can fix this asap (offline conversation) in a way that will allow
SK to upgrade to a patched version, delete its hack, and have those
broader scenarios "just work". In the meantime, though, KernelBuilder
behaves as desired.
I also stopped short of adding Add{Azure}OpenAIXx methods that resolve
the OpenAIClient from the service provider. That'll be an important
thing to do once there's an Aspire component for Azure.AI.OpenAI, but in
the meantime, I didn't want there to be an attractive Add{Azure}OpenAIXx
method that looked like you didn't need to supply the necessary
information even though you do.
---------
Co-authored-by: SergeyMenshykh <[email protected]>
Contributes to https://github.com/microsoft/semantic-kernel/issues/3151
There's currently both Microsoft.SemanticKernel.Planning and
Microsoft.SemanticKernel.Planners. It's confusing for there to be a
distinction between the two, especially since you need the former in
order to do anything meaningful with the latter. This just consolidates
the latter into the former. One less namespace to reason about.
### Motivation and Context
Stepwise planner built on OpenAI function calling
### Description
<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->
### Contribution Checklist
<!-- Before submitting this PR, please make sure: -->
- [x] The code builds clean without any errors or warnings
- [x] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 😄
---------
Co-authored-by: Ben Thomas <[email protected]>
Co-authored-by: Mark Wallace <[email protected]>
Roger Barreto