CompleteRequestSettings is insufficiently abstract #2735
Assignees
Labels
ai connector
Anything related to AI connectors
kernel
Issues or pull requests impacting the core kernel
Milestone
Comments
|
@shawncal I have been working on a solution for this issue, so I can take this one. |
nacharya1
added
kernel
|
I've added support for Function Calling in my fork this week and it's pretty obvious that the library painted itself into a corner here with ChatRequestSettings, CompleteRequestSettings, and ISKFunction. You are going to be forced to do a big refactor soon and break some stuff. Is there a branch I can watch to follow along? |
|
@douglasware I'll a link to the PR once I have something to share. I expect to have something later this week. |
This was referenced Sep 13, 2023
Merged
4 tasks
|
@douglasware Here's the PR #2829. I still have more testing to do but any feedback is much appreciated. |
|
@douglasware Could you take a look at this comment #2829 (comment) to see does this change make sense |
github-merge-queue bot
pushed a commit
that referenced
this issue
Sep 20, 2023
…se AIRequestSettings) (#2829) ### Motivation and Context Currently the SK core uses a model for LLM request settings which is OpenAI specific. This is a refactor to remove this assumption and to allow SK to be LLM agnostic. Resolves: #2735 ### Description 1. Remove `ChatRequestSettings` and `CompleteRequestSettings` and use new class `AIRequestSettings` instead 1. Create default OpenAI request settings class `OpenAIRequestSetings` which extends `AIRequestSettings`. 1. Fix up planner, unit tests and integration tests 1. Add a new Kernel syntax example showing all options to configure LLM request settings ### Contribution Checklist - [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 😄
SOE-YoungS
pushed a commit
to SOE-YoungS/semantic-kernel
that referenced
this issue
Nov 1, 2023
…se AIRequestSettings) (microsoft#2829) ### Motivation and Context Currently the SK core uses a model for LLM request settings which is OpenAI specific. This is a refactor to remove this assumption and to allow SK to be LLM agnostic. Resolves: microsoft#2735 ### Description 1. Remove `ChatRequestSettings` and `CompleteRequestSettings` and use new class `AIRequestSettings` instead 1. Create default OpenAI request settings class `OpenAIRequestSetings` which extends `AIRequestSettings`. 1. Fix up planner, unit tests and integration tests 1. Add a new Kernel syntax example showing all options to configure LLM request settings ### Contribution Checklist - [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
and others
Note: I could also file this as a bug as many models do not work with SK and the Oobabooga and HuggingFace connectors because of this problem.
When I started using other completion APIs, e.g. Oobabooga, HuggingFace, the design around Microsoft.SemanticKernel.AI.TextCompletion.CompleteRequestSettings gets inconvenient quickly because behind those APIs the parameters vary, and the SK implementation is specifically shaped around OpenAI API.
Seems like this one design element is a big blocker to the vision of a pluggable system.
A simple (but not terribly robust) solution would be to get rid of CompleteRequestSettings altogether and allow the use of a name/value collection instead.
The text was updated successfully, but these errors were encountered: