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: Add multitargeting to .NET libraries #4421

Merged
merged 6 commits into from
May 13, 2024

Conversation

stephentoub
Copy link
Member

Adds net8.0 targets and updates various code to take advantage of newer APIs and also fix analyzers.
Fixes #4308

@shawncal shawncal added .NET Issue or Pull requests regarding .NET code kernel Issues or pull requests impacting the core kernel memory connector kernel.core labels Dec 25, 2023
@github-actions github-actions bot changed the title Add multitargeting to .NET libraries .Net: Add multitargeting to .NET libraries Dec 25, 2023
@Krzysztof318
Copy link
Contributor

What benefit does net8.0 targeting give us if we won't be able to use all the new APIs to maintain compatibility with net standard 2.0?

@stephentoub
Copy link
Member Author

stephentoub commented Dec 31, 2023

What benefit does net8.0 targeting give us if we won't be able to use all the new APIs to maintain compatibility with net standard 2.0?

When running on .NET 8+, apps will use the net8.0 binaries rather than the netstandard2.0 binaries. That means it can use newer APIs. So, for example, it can use SocketsHttpHandler and its finer-grained control when on .NET 8+. It can use overloads that take a CancellationToken that don't exist in netstandard in order to improve cancellation and responsiveness. It can use built-in collections like PriorityQueue rather than being to ship its own version. It can use the regex source generator. And so on. It also implicitly benefits from new APIs in places the compiler targets them, eg interpolated strings will compile with the more efficient implementation introduced in C# 10 / .NET 6. It further has deployment benefits, eg System.Text.Json doesn't need to be deployed as part of the app because it's built-in to netcoreapp8. Etc.

@stephentoub
Copy link
Member Author

Depends on #5802

@stephentoub
Copy link
Member Author

Depends on #5819

@stephentoub
Copy link
Member Author

Still need to figure out why code coverage is unhappy, but other than that, this should be ready for review @markwallace-microsoft

@stephentoub stephentoub force-pushed the multitarget branch 8 times, most recently from a5440e7 to 42f05f7 Compare May 7, 2024 16:35
@stephentoub
Copy link
Member Author

@dmytrostruk, I think you set up the code coverage checking? Can you make a recommendation for what to do here?
image

@stephentoub
Copy link
Member Author

stephentoub commented May 7, 2024

@dmytrostruk, I think you set up the code coverage checking? Can you make a recommendation for what to do here?

@stephentoub Yes, I can see some changes in OpenAI connector, so it worth to run dotnet/code-coverage.ps1 locally to generate HTML report and see potential places that can be covered with unit-tests in that assembly. If you want, I can assist with that.

There aren't any meaningful changes. I think the problem is that the library is already razor-thin above the code coverage line (main is sitting at 81% branch coverage for the OpenAI connector, and at < 80% CI fails), so anything that perturbs it even slightly could cause CI to totally fail. There's nothing related to my changes that wasn't covered, so I went and added unrelated tests to boost the coverage back above 80%, but that's not really a sustainable position. I suggest someone take some time to bump the coverage number up above 90% so there's much more wiggle room.

@stephentoub stephentoub force-pushed the multitarget branch 2 times, most recently from 4914509 to 2081f06 Compare May 7, 2024 20:22
@stephentoub
Copy link
Member Author

Finally green. @markwallace-microsoft, please review when you get a chance.

@stephentoub stephentoub enabled auto-merge May 7, 2024 20:46
@dmytrostruk
Copy link
Member

I think the problem is that the library is already razor-thin above the code coverage line (main is sitting at 81% branch coverage for the OpenAI connector, and at < 80% CI fails), so anything that perturbs it even slightly could cause CI to totally fail.

@stephentoub That's true. When I was improving code coverage, I spent some time to hit 80% threshold by adding a lot of tests and back then I couldn't afford even more time to make it 90% or higher. On the other hand, I haven't noticed this problem recurring often, but maybe it's just me.

There's nothing related to my changes that wasn't covered, so I went and added unrelated tests to boost the coverage back above 80%, but that's not really a sustainable position. I suggest someone take some time to bump the coverage number up above 90% so there's much more wiggle room.

Thank you! Yes, I agree, we should add more tests to give us some space before it breaks CI pipeline.

@stephentoub
Copy link
Member Author

stephentoub commented May 9, 2024

On the other hand, I haven't noticed this problem recurring often, but maybe it's just me.

I think it got worse for this particular library when all the various audio, image, etc. services were added, as well as the update function calling abstractions, as they don't have as good coverage in the tests.

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.

Some minor changes. LGTM

@stephentoub stephentoub added this pull request to the merge queue May 13, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 13, 2024
@markwallace-microsoft markwallace-microsoft added this pull request to the merge queue May 13, 2024
Merged via the queue into microsoft:main with commit c369ab3 May 13, 2024
15 checks passed
johnoliver pushed a commit to johnoliver/semantic-kernel that referenced this pull request Jun 5, 2024
Adds net8.0 targets and updates various code to take advantage of newer
APIs and also fix analyzers.
Fixes microsoft#4308
johnoliver pushed a commit to johnoliver/semantic-kernel that referenced this pull request Jun 5, 2024
Adds net8.0 targets and updates various code to take advantage of newer
APIs and also fix analyzers.
Fixes microsoft#4308
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation kernel.core kernel Issues or pull requests impacting the core kernel memory connector memory .NET Issue or Pull requests regarding .NET code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

.Net: Add net8.0 target framework
8 participants