search

Azure-Samples / azure-search-openai-demo-csharp

A sample app for the Retrieval-Augmented Generation pattern running in Azure, using Azure Cognitive Search for retrieval and Azure OpenAI large language models to power ChatGPT-style and Q&A experiences.
MIT License
555 stars 346 forks source link

Upgrade to .NET 8 #191

Closed IEvangelist closed 8 months ago

IEvangelist commented 8 months ago

I just now saw #186, I didn't realize before that there was an effort in place to do this work. There are parts of that PR that look good—@luisquintanilla how should we proceed?

I pulled in the changes from the PR that I liked. ~Let's add the CPM in a separate PR, since versions weren't bumped~.

I've bumped the versions, and I'm updating this PR to utilize the CPM feature since it does improve the PacakgeReference experience.

luisquintanilla commented 8 months ago

Thanks @IEvangelist

luisquintanilla commented 8 months ago

I just now saw #186, I didn't realize before that there was an effort in place to do this work. There are parts of that PR that look good—@luisquintanilla how should we proceed?

I pulled in the changes from the PR that I liked. ~Let's add the CPM in a separate PR, since versions weren't bumped~.

I've bumped the versions, and I'm updating this PR to utilize the CPM feature since it does improve the PacakgeReference experience.

Sounds good @IEvangelist. Thanks.

Which parts from #186 are not in this PR?

aaronpowell commented 8 months ago

Just be aware that #186 builds on top of #184 so there are some unrelated changes are the VS Code experience in the PR.

aaronpowell commented 8 months ago

I noticed that this PR also upgrades to C# 12 preview - I'd suggest not doing that in this PR but in a follow-up one to ensure that it's easier to review the .NET 8 specific changes without also having language changes in there.

Small and targeted PRs are easier to review and remediate.

luisquintanilla commented 8 months ago

@LittleLittleCloud do these changes affect the App Insight / Authentication changes you've made?

LittleLittleCloud commented 8 months ago

@luisquintanilla Shouldn't be effected.

luisquintanilla commented 8 months ago

Cool. Let's merge those in first then and do a quick test and follow of these changes.

aaronpowell commented 8 months ago

Shall we close #186 in favour of this?

Yesterday I noticed a problem in my PR with VS Code debugging which I addressed in https://github.com/Azure-Samples/azure-search-openai-demo-csharp/pull/186/commits/178aa843c4f8149e779387ff3f3931aa8c2eb492 (although there's another problem that I'm digging through)

luisquintanilla commented 8 months ago

Shall we close #186 in favour of this?

Yesterday I noticed a problem in my PR with VS Code debugging which I addressed in 178aa84 (although there's another problem that I'm digging through)

I'm okay with that. Should #184 be closed as well since #186 built on top of it?

aaronpowell commented 8 months ago

Shall we close #186 in favour of this? Yesterday I noticed a problem in my PR with VS Code debugging which I addressed in 178aa84 (although there's another problem that I'm digging through)

I'm okay with that. Should #184 be closed as well since #186 built on top of it?

I'd merge #184 first as the changes it introduces are unrelated to the .NET 8 upgrade, so it would give better history tracking of when/why changes were made.