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 adding chat completion and settings to Oobabooga connector #2229

Closed
wants to merge 64 commits into from

Conversation

jsboige
Copy link
Contributor

@jsboige jsboige commented Jul 31, 2023

Motivation and Context

Resolves: #2583

a Oobabooga text completion connector was recently added to SK. Oobabooga API also supports a rich Chat API compatible with SK's chat completion model

Description

This PR completes the recently added Oobabooga connector with a complementary chat completion connector and exposes Oobabooga settings both for text and chat completion.

Contribution Checklist

@jsboige jsboige requested a review from a team as a code owner July 31, 2023 10:20
@shawncal shawncal added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel labels Jul 31, 2023
@jsboige
Copy link
Contributor Author

jsboige commented Aug 25, 2023

@dmytrostruk Thanks for your review.
I believe I addressed all of your comments and concerns.
From my commits since yesterday, the first address your comments individually, but then 2 issues involved a major refactoring with major changes:

  • My initial PR included an initial effort at mutualizing some code between chat and text completion. I had also noted that I could have gone further as you did, but that was a bit tricky since the code was similar but making use of a series of distinct classes for request, response, results etc. Accordingly, that involved starting making use of generics to support Text/Chat with all those differences.

  • Then the major refactoring pushed that further to get rid of the many ctor parameters by moving a lot of the logic into the Settings classes, which used to inherit from the Oobabooga's parameters, and now hold them as an inner property, additionally to the parameters that used to be within the completion classes.
    Accordingly, there is a several step hierarchy through generics for settings as well as for completion.

Now I suppose that the settings where most of the parameters and some of the logic were moved might invite to some more refactoring later on if we want clean serializable settings as was initially planned, but I believe this is a good start, and it has the additional benefit to fix an issue I had initially overlooked, that the completion themselves are supposed to be scoped and short-lived, whereas all the unit stress tests involved a single completion hammered with many calls.
By moving the concurrency control to the longer-lived settings instance, similar tests should now support numerous kernels similarly hammered as long as they share the same settings instances.

Also, I guess now some proper documentation would be welcome. I started documenting that related PR for a multiconnector in the form of that Readme file that contains useful information to get hold of this connector, but that other PR is currently based on the previous merged version of this connector, so it will need some merge work once this one gets merged.

@jsboige
Copy link
Contributor Author

jsboige commented Aug 25, 2023

@jsboige Thank you for contribution! I left initial feedback. In order to proceed with this PR, please create GitHub Issue. Our team will triage and prioritize it accordingly. Thanks again!

Here is the corresponding issue

@dmytrostruk
Copy link
Member

@jsboige Thanks for all the hard work!
Before performing next round of review, I would like to share my general feedback and our long-term vision. It would be great to know your thoughts.

In general, as this functionality grows, it looks like separate Oobabooga .NET SDK, while in this repository we are focused more on lightweight connectors, that can be used with Semantic Kernel. Ideal scenario is when integration client is implemented as separate package, and we re-use it when implementing ITextCompletion and IChatCompletion interfaces (like adapters).

At the moment we are looking at opportunity to extract connectors and plugins to separate repositories (PR with contribution guidelines: #2503). We are working on mechanisms and will post official guidelines soon.

Meanwhile, do you think it will be possible for you to migrate Oobabooga client implementation to separate repository and package, so in this connector we will reference it and use necessary methods to implement ITextCompletion and IChatCompletion interfaces, so it can be used with Semantic Kernel?

Please let me know what you think about it, thank you!

@jsboige
Copy link
Contributor Author

jsboige commented Aug 30, 2023

@jsboige Thanks for all the hard work! Before performing next round of review, I would like to share my general feedback and our long-term vision. It would be great to know your thoughts.

In general, as this functionality grows, it looks like separate Oobabooga .NET SDK, while in this repository we are focused more on lightweight connectors, that can be used with Semantic Kernel. Ideal scenario is when integration client is implemented as separate package, and we re-use it when implementing ITextCompletion and IChatCompletion interfaces (like adapters).

At the moment we are looking at opportunity to extract connectors and plugins to separate repositories (PR with contribution guidelines: #2503). We are working on mechanisms and will post official guidelines soon.

Meanwhile, do you think it will be possible for you to migrate Oobabooga client implementation to separate repository and package, so in this connector we will reference it and use necessary methods to implement ITextCompletion and IChatCompletion interfaces, so it can be used with Semantic Kernel?

Please let me know what you think about it, thank you!

@dmytrostruk Thanks for following through with reviewing yet another PR of mine, and for extending your long-term vision.

Regarding #2503, I understand the motivation, which makes sense and, similarly to what happened with the recent extraction of chat-copilot, I'll be happy to commit to another dedicated connector's repository to keep this one light and focused.

Then extending the idea to extracting the Oobabooga's client from the connector itself, the way I understand it, you would like to reproduce the existing architecture of the OpenAI connector, which is implemented through as a wrapper to Azure SDK's Open AI client library's Nugget package, that library being an external projet that can be used to target Open AI models independently from semantic-kernel. Again that makes sense, and I suppose most connectors and plugin are meant to wrap some kind of external client library with bindings to the core interfaces and abstractions.

Here are several considerations that come to mind:

  • My company is a small single man's operation, my main occupation at the moment is teaching, and that side project of a card's game about argumentation is probably going to need a significant effort of mine in the near term even though I'm definitely planning to make SK a part of that project through the plugin I started to draft during the hackathon. Accordingly, taking ownership of an external repository is an extra commitment I should not take lightly, but I understand that also as an opportunity for me to grow my business by becoming a close partner to the official semantic-kernel channels, which I believe I'm willing to do provided I can make it somehow sustainable.

  • Then concerning the opportunity of extracting an external independent Oobabooga connector, I am not sure there is yet the material to do so.

    • Looking at the code itself, there does not seem to be much to extract yet from the SK's wrapper. Just like the Http client logic is very similar to Huggingface's and I could see opportunities to try and abstract some back into the kernel, the websocket logic from the streaming endpoint also looks pretty general to me, and that would leave us with just about the couple classes in charge of implementing the specific json text and chat request and response serialization schemas.

    • Now if you start adding embeddings, image generations etc. that would draw a different picture, but the way oobabooga is currently evolving, they chose to implement those through a distinct extension API that emulates Open AI's and that would actually perfectly fit adding a third implementation to the existing OpenAI's connector beside the Azure and OpenAI existing ones, because the needed abstraction work was already done for those 2; it should be straightforward to implement it on top of the same base classes and while I don't see myself adding much to this specific oobabooga connector in the near future as I believe the main official API is now entirely covered, the OpenAI extension is actually one of the next contributions I'm planning to bring to SK, whether it's inside the actual project or referencing the corresponding Nugget package (according to your preferences) in a distinct repository.

    • Then I also wonder if anyone would be willing to use such a connector outside of semantic-kernel. My understanding of the market is that most oobabooga users are Python developers, which I'm not, and they won't be willing to use a c# connector unless it brings them something they don't already find in Langchain or Guidance, typically what SK brings, either through Python.Net or a chat-copilot equivalent using external plugins of theirs, but I suppose preferably through a Python port of the connector. Then some .Net developers might be interested in a raw oobabooga connector, but to be honest I'm not, and I doubt anyone will be willing to push for that in Microsoft's sphere as it might be perceived as interfering with the standard toolkit more than through this SK connector.

    • This is not to say I don't see myself doing that extraction into yet another repository, but I'd rather keep that for a later term if you don't mind.

  • I suppose the same consideration applies to the multiconnector I recently introduced. If any, that one surely has the potential to become a major plugin and would need it's own external repository soon, as apparently the demand for such a component is big. I believe there is a natural continuation of that connector leveraging Infer.Net, which I'm willing to implement, based on a probabilistic model accommodating those 4 examples:

  • Add to that that I plan to implement a complementary distributed Spark.Net based connector, which I believe should be relatively straight-forward too, and that starts to make for a serious semantic-kernel code base to maintain.

Now here's for my side of the story.
I'm now teaching AI in .Net, after a previous career where I was a DNN evangelist in France, with several open source extensions including one that I'd like to revive with SK, that did not meet their public.

Doing and teaching AI in .Net has proved exhausting with most projects ending up losing traction one way or another (Solver foundation dead, Encog dead, Accord.Net too, Genetic Sharp stalled, AIMA.Net dead, DlxLib dead, Quickgraph dead, Heuristiclab dead, CNTK dead, KerasSharp dead, TensorflowSharp dead, etc.). Even those still active like Z3, OR-tools, ML.Net, Spark.Net, SciSharp, TorchSharp, Infer etc. are not super healthy. I hope you guys are aware of that grim history.

With that said I'm still pretty committed to that .Net stack and I'd rather see IronPython and IKVM properly revived that take the Python.Net bridging path, which I started doing occasionally.
This is to say I really like this library, I love what LLMs have to bring, and I see this project as one of the latest chance for c# to shine in the AI landscape, which probably is the reason I've been willing to invest so much in it over the last couple months.

Also I tried to avoid past pitfalls where significant contributions of mine would be wasted like this one or that one although I just realized the latest was eventually partially merged into a revival project.

Accordingly, I'm super grateful my contributions were accepted with open arms, and I believe I'm now ready to become the steward of some of it if you think that's the best path forward.
Let me know how best to proceed and I'll try to make some time to accommodate your decisions.

@dmytrostruk
Copy link
Member

@jsboige Thank you very much for your response and great contribution to this repository!
We will provide the next steps as soon as we align with the process. Let's keep this PR open for now.
Thanks again!

@dmytrostruk dmytrostruk assigned nacharya1 and unassigned dmytrostruk Sep 4, 2023
@nacharya1
Copy link
Contributor

@jsboige FYI: We recently updated our contributing guidelines [here](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#adding-plugins-and-memory-connectors , stating we are not directly adding plugin and connectors directly into the SDK repository. We are encouraging the community to host their own plugins and connectors separately in their own repositories. At the time we are exploring how we can provide a way to discover plugins and connectors and should have an update soon. I am going to close this issue given our change in contribution guidelines.
cc: @dmytrostruk

@nacharya1 nacharya1 closed this Sep 7, 2023
@jsboige
Copy link
Contributor Author

jsboige commented Sep 8, 2023

@nacharya1, @dmytrostruk
OK I will start building an alternate repo. I will let you know about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kernel Issues or pull requests impacting the core kernel .NET Issue or Pull requests regarding .NET code
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

.Net: adding chat completion and settings to Oobabooga connector
5 participants