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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.Net: ADR with proposal for the multiple LLM support use cases #3040

Conversation

markwallace-microsoft
Copy link
Member

@markwallace-microsoft markwallace-microsoft commented Oct 2, 2023

Motivation and Context

  • ADR describing the design for multiple LLM support. This will be expanded in the future to allow additional use cases such as selecting an AI service by LLM model id.
  • The IAIServiceSelector has changed so that it is added to the IKernel instance, see example 62 for an example of a custom IAIServiceSelector implementation.

Contribution Checklist

@shawncal shawncal added the docs and tests Improvements or additions to documentation label Oct 2, 2023
@matthewbolanos matthewbolanos self-requested a review October 2, 2023 14:29
@shawncal shawncal added the .NET Issue or Pull requests regarding .NET code label Oct 4, 2023
@github-actions github-actions bot changed the title ADR with proposal for the multiple LLM support use cases .Net: ADR with proposal for the multiple LLM support use cases Oct 4, 2023
@markwallace-microsoft markwallace-microsoft marked this pull request as ready for review October 4, 2023 19:45
@markwallace-microsoft markwallace-microsoft requested a review from a team as a code owner October 4, 2023 19:45
@markwallace-microsoft markwallace-microsoft force-pushed the users/markwallace/multiple-llm-support-adr branch from 5c1da27 to 8bd8527 Compare October 5, 2023 09:11
@shawncal shawncal added kernel Issues or pull requests impacting the core kernel kernel.core labels Oct 25, 2023
@matthewbolanos
Copy link
Member

Could we also support a scenario where the kernel keys off of "Model ID" instead of "Service ID"? In most cases, the semantic function author doesn't know the names of all the services that the kernel invoker has added, they'll just know the standard model names that are out in the wild (e.g., gpt-4, gpt-3.5-turbo, etc.). Leveraging "Model ID" should likely be the main/predominant scenario.

@markwallace-microsoft
Copy link
Member Author

Could we also support a scenario where the kernel keys off of "Model ID" instead of "Service ID"? In most cases, the semantic function author doesn't know the names of all the services that the kernel invoker has added, they'll just know the standard model names that are out in the wild (e.g., gpt-4, gpt-3.5-turbo, etc.). Leveraging "Model ID" should likely be the main/predominant scenario.

Right now you could make the service if the same as the model id and that would work.

Currently matching specifically on model id is called out of scope because we have no way to store arbitrary attributes for a service. I'll add this as an additional task and will address in a follow up PR.

@SergeyMenshykh
Copy link
Member

Could we also support a scenario where the kernel keys off of "Model ID" instead of "Service ID"? In most cases, the semantic function author doesn't know the names of all the services that the kernel invoker has added, they'll just know the standard model names that are out in the wild (e.g., gpt-4, gpt-3.5-turbo, etc.). Leveraging "Model ID" should likely be the main/predominant scenario.

I'm not sure, but it seems we need both the Service ID and Model ID to cover scenarios in which one connector provides multiple models and when the same model is provided by multiple connectors. Otherwise, the connector will have to be registered many times – one time per model it supports for the former scenario, or it won't be possible to register many connectors supporting the same model for the latter scenario.

@markwallace-microsoft
Copy link
Member Author

Could we also support a scenario where the kernel keys off of "Model ID" instead of "Service ID"? In most cases, the semantic function author doesn't know the names of all the services that the kernel invoker has added, they'll just know the standard model names that are out in the wild (e.g., gpt-4, gpt-3.5-turbo, etc.). Leveraging "Model ID" should likely be the main/predominant scenario.

I'm not sure, but it seems we need both the Service ID and Model ID to cover scenarios in which one connector provides multiple models and when the same model is provided by multiple connectors. Otherwise, the connector will have to be registered many times – one time per model it supports for the former scenario, or it won't be possible to register many connectors supporting the same model for the latter scenario.

So right now we can do this:

IKernel kernel = new KernelBuilder()
    .WithAzureChatCompletionService(serviceId: "azure-gpt-4", deploymentName: "gpt-4", endpoint: endpoint, apiKey: apiKey)
    .WithAzureChatCompletionService(serviceId: "azure-gpt-3.5", deploymentName: "gpt-3.5", endpoint: endpoint, apiKey: apiKey)
    .WithOpenAIChatCompletionService(serviceId: "openai-gpt-4", modelId: "gpt-4", apiKey: openAIApiKey)
    .WithOpenAIChatCompletionService(serviceId: "openai-gpt-3.5", modelId: "gpt-3.5", apiKey: openAIApiKey)
    .Build();

I can write an IAIServiceSelector that picks the right service by service id

In the future we would like to be able to support this:

IKernel kernel = new KernelBuilder()
    .WithAzureChatCompletionService(modelId: "gpt-4", deploymentName: "foo", endpoint: endpoint, apiKey: apiKey)
    .WithAzureChatCompletionService(modelId: "gpt-3.5", deploymentName: "bar", endpoint: endpoint, apiKey: apiKey)
    .WithOpenAIChatCompletionService(modelId: "gpt-4", apiKey: openAIApiKey)
    .WithOpenAIChatCompletionService(modelId: "gpt-3.5", apiKey: openAIApiKey)
    .Build();

We need to solve how to write an IAIServiceSelector that picks the right service by model id and service type

@markwallace-microsoft markwallace-microsoft force-pushed the users/markwallace/multiple-llm-support-adr branch from a01649b to 6416771 Compare October 26, 2023 21:17
@markwallace-microsoft markwallace-microsoft force-pushed the users/markwallace/multiple-llm-support-adr branch from d7bef18 to 368f03e Compare October 27, 2023 17:27
@markwallace-microsoft markwallace-microsoft force-pushed the users/markwallace/multiple-llm-support-adr branch from 64f742a to 305b32c Compare October 27, 2023 17:29
Copy link
Member

@RogerBarreto RogerBarreto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be marked as a Breaking Change after making ServiceProvider internal.

@markwallace-microsoft markwallace-microsoft added the PR: breaking change Pull requests that introduce breaking changes label Oct 27, 2023
@markwallace-microsoft markwallace-microsoft added this pull request to the merge queue Oct 27, 2023
Merged via the queue into microsoft:main with commit cff0170 Oct 27, 2023
19 checks passed
@markwallace-microsoft markwallace-microsoft deleted the users/markwallace/multiple-llm-support-adr branch October 27, 2023 18:09
SOE-YoungS pushed a commit to SOE-YoungS/semantic-kernel that referenced this pull request Nov 1, 2023
…soft#3040)

### Motivation and Context

- ADR describing the design for multiple LLM support. This will be
expanded in the future to allow additional use cases such as selecting
an AI service by LLM model id.
- The `IAIServiceSelector` has changed so that it is added to the
`IKernel` instance, see example 62 for an example of a custom
`IAIServiceSelector` implementation.

### 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 😄
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs and tests Improvements or additions to documentation kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code PR: breaking change Pull requests that introduce breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants