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

Pull in latest from upstream, update to dotnet 8 #25

Merged
merged 15 commits into from
Jan 3, 2025
Merged

Conversation

mcmcgrath13
Copy link
Member

@mcmcgrath13 mcmcgrath13 commented Dec 18, 2024

Pull in the latest commits from the Microsoft upstream repo. This includes updating the repo to dotnet v8, which required a tiny bit more work to keep our github actions running

Copy link

@akasper akasper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've double-checked for all mentions of dotnet versions throughout the codebase, and I haven't found any that you didn't find yourself. I'm able to build the image and perform basic conversion via the convert-to-fhir and fhir-converter/convert-to-fhir endpoints.

So far, it's looking like the upgrade to dotnet 8 does not introduce any new problems.

I'll spend some time today testing out more sophisticated eCRs and input types. I am also currently digging into other possible impacts this version change could have on the entire orchestration pipeline.

@mcmcgrath13 Do you have any doubts about this PR? Anyplace where you think we need to ask extra human-being-using-thinking-brain questions?

@mcmcgrath13
Copy link
Member Author

Thanks for the thorough checking @akasper. I'm not too worried about this as it should be pretty isolated to the fhir-converter's container and all tests are passing (including the conversion tests)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm. I'm noticing some surprising slow downs. In github actions it's taking 15 minutes with this change compare that to the average of 9-10 minutes for the rest . Locally just running dotnet I'm also seeing something similar of previously it was ~2.5 seconds and now it's ~3.5 seconds (this could just be a my machine problem though.
Net 8.0
image
Net 6.0
image

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm - the internet AI robot says

While generally considered to have performance improvements over .NET 6, in certain specific scenarios, .NET 8 might show performance regressions compared to .NET 6, particularly in areas like null validation, array creation, type casting, and string handling, where some benchmarks have indicated slight slowdowns; however, for most typical applications, .NET 8 is expected to be faster overall

which fhir conversion does quite a bit of array creation and string handling (and maybe the others too under the hood)

Do we want to stay behind the upstream because of this? I don't love the slowdown, but I also don't love letting ourselves drift very far from the Microsoft version of the repo

cc @austin-hall-skylight

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm poking around at upgrading deps to see if that gets us back some perf

@austin-hall-skylight
Copy link

Let's go ahead and upgrade. It's unfortunate that we're going to take a performance hit but I'd prefer that over any potential security issues from using a dotnet version that's unsupported.

@mcmcgrath13 mcmcgrath13 merged commit c9d2151 into main Jan 3, 2025
2 checks passed
@mcmcgrath13 mcmcgrath13 deleted the mcm/dotnet-8 branch January 3, 2025 20:48
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.

6 participants