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

#113: Semantic Kernel #155

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

#113: Semantic Kernel #155

wants to merge 7 commits into from

Conversation

zateutsch
Copy link
Contributor

fixes #113

Needed to add a dependency and update another for this, so would appreciate a double check that nothing went awry.

Also, this sample takes forever to load. Not sure if there is any way around it.

@zateutsch zateutsch requested a review from nmetulev February 6, 2025 04:43
@zateutsch
Copy link
Contributor Author

@nmetulev this sample has some problems, I'm investigating

@azchohfi
Copy link
Contributor

azchohfi commented Feb 7, 2025

@zateutsch I've fixed the sample with a more generic solution (using IChatClient's overload). This will also work well will PhiSilica, so its only a plus :) The package you were referencing have its own implementation of the equivalent of IChatClient for ORT, so we should not use it (it was fixed to the CPU version).

@zateutsch
Copy link
Contributor Author

I don't think we should merge this until I've double checked that that memory leak is gone

@zateutsch
Copy link
Contributor Author

zateutsch commented Feb 10, 2025

I don't think we should merge this until I've double checked that that memory leak is gone

Okay, I took a look at this and everything seems to get garbage collected a lot quicker with AsChatCompletionService. Something about cancellation during Unloaded is still weird with this sample, and it stays in memory for 10-15s compared to the other samples that get collected almost right away. This only happens if you navigate during generation, it works as expected if the sample is idle and you navigate.

I think it should be fine to merge how it is now. @nmetulev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[SAMPLE] Create a Semantic Kernel Chat with Onnx
2 participants