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

Add LLM functionality from SpeziFHIR #53

Merged
merged 10 commits into from
Jan 20, 2025
Merged

Add LLM functionality from SpeziFHIR #53

merged 10 commits into from
Jan 20, 2025

Conversation

LeonNissen
Copy link
Contributor

@LeonNissen LeonNissen commented Dec 11, 2024

Add LLM functionality from SpeziFHIR

♻️ Current situation & Problem

SpeziFHIR contained LLM-based summary and interpretation functionality that uses OpenAI API-s and local models like Llama, all of which are tightly coupled with app-specific logic. This coupling makes it more difficult for other apps to use SpeziFHIR without inheriting unnecessary LLM-related dependencies and functionality they may not need. Since this functionality is closer to LLMonFHIR, we are migrating it here.

StanfordSpezi/SpeziFHIR#24

⚙️ Release Notes

  • Migrated LLM summary and interpretation functionality from SpeziFHIR to LLMonFHIR.

📚 Documentation

Code comments are added alongside the code.

✅ Testing

No additional testing code and this would be beyond the scope of this PR.

Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

Sorry, something went wrong.

@jdisho jdisho self-requested a review January 8, 2025 13:03
@jdisho
Copy link
Collaborator

jdisho commented Jan 8, 2025

Hey @LeonNissen, thanks for the great work here. Before I jump doing a full review, are the changes here copy&paste from SpeziFHIR with some minor adaptations?

@jdisho
Copy link
Collaborator

jdisho commented Jan 16, 2025

Hey @LeonNissen - Just pushed two commits. The first one resolves merge conflicts with main, and the second fixes dependency issues that were preventing the app from building.

At least this runs now 🎉

@jdisho jdisho force-pushed the feature/local-llm branch from febaddb to f3bfc5d Compare January 16, 2025 17:20
@LeonNissen
Copy link
Contributor Author

LeonNissen commented Jan 16, 2025

Hey @LeonNissen, thanks for the great work here. Before I jump doing a full review, are the changes here copy&paste from SpeziFHIR with some minor adaptations?

@jdisho Oh I am so sorry, I just read it now! But yes there are just copy & past changes 🚀

@LeonNissen
Copy link
Contributor Author

LeonNissen commented Jan 16, 2025

Hey @LeonNissen - Just pushed two commits. The first one resolves merge conflicts with main, and the second fixes dependency issues that were preventing the app from building.

At least this runs now 🎉

@jdisho Awesome great work! I think thats a good starting point for further improvements 🥳

@jdisho jdisho force-pushed the feature/local-llm branch from 5c4104a to c6f0273 Compare January 17, 2025 13:59
@StanfordBDHG StanfordBDHG deleted a comment from codecov bot Jan 17, 2025
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 36.77419% with 588 lines in your changes missing coverage. Please review.

Project coverage is 43.41%. Comparing base (9430b47) to head (f2494fd).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...pleResources/FHIRMultipleResourceInterpreter.swift 6.82% 82 Missing ⚠️
LLMonFHIR/Settings/SettingsView.swift 0.00% 74 Missing ⚠️
LLMonFHIR/FHIR Views/InspectResourceView.swift 37.50% 70 Missing ⚠️
...MultipleResources/FHIRGetResourceLLMFunction.swift 18.08% 68 Missing ⚠️
.../MultipleResources/MultipleResourcesChatView.swift 0.00% 64 Missing ⚠️
LLMonFHIR/Helper/FHIRStore+Interpretation.swift 47.50% 63 Missing ⚠️
...LMonFHIR/FHIRSummary/FHIRResourceSummaryView.swift 44.27% 34 Missing ⚠️
LLMonFHIR/Settings/FHIRPromptSettingsView.swift 0.00% 30 Missing ⚠️
...LMonFHIR/FHIRProcessor/FHIRResourceProcessor.swift 27.03% 27 Missing ⚠️
LLMonFHIR/FHIRSummary/FHIRResourceSummary.swift 60.00% 18 Missing ⚠️
... and 6 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #53      +/-   ##
==========================================
- Coverage   49.15%   43.41%   -5.74%     
==========================================
  Files          16       32      +16     
  Lines         584     1447     +863     
==========================================
+ Hits          287      628     +341     
- Misses        297      819     +522     
Files with missing lines Coverage Δ
LLMonFHIR/Helper/Binding+Negate.swift 100.00% <100.00%> (ø)
...MonFHIR/Helper/CodableArray+RawRepresentable.swift 0.00% <ø> (ø)
LLMonFHIR/Home.swift 73.53% <ø> (ø)
LLMonFHIR/LLMonFHIR.swift 100.00% <ø> (ø)
LLMonFHIR/LLMonFHIRDelegate.swift 100.00% <100.00%> (ø)
LLMonFHIR/LLMonFHIRStandard.swift 8.70% <ø> (ø)
LLMonFHIR/ResourceView.swift 92.00% <ø> (ø)
...ation/SingleResource/FHIRResourceInterpreter.swift 81.82% <81.82%> (ø)
LLMonFHIR/FHIRInterpretationModule.swift 87.28% <87.28%> (ø)
...HIR/FHIRProcessor/FHIRResourceProcessorError.swift 0.00% <0.00%> (ø)
... and 13 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9430b47...f2494fd. Read the comment docs.

@PSchmiedmayer PSchmiedmayer added good first issue Good for newcomers and removed good first issue Good for newcomers labels Jan 19, 2025
Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Thank you @LeonNissen & @jdisho!

I fixed the CodeQL build issue and would be happy to see the PR merged.

We should also scan the code and ensure that we remove elements that are no longer needed and try to cleanup a few things. I also created #56 in response to this PR to ensure that we setup some automated setup for this.

Let's get this merged and address the open issues towards an usability study.

Comment on lines +33 to +37
.task {
#if !TEST
// interpret()
#endif
}
Copy link
Member

Choose a reason for hiding this comment

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

We might want to go though the elements like this and ensure that we remove uncommented code & other elements in the long run.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for updating the setup here!

@PSchmiedmayer PSchmiedmayer added the enhancement New feature or request label Jan 19, 2025
@PSchmiedmayer
Copy link
Member

@LeonNissen & @jdisho Would be amazing if we can update the PR description to reflect the changes & if we create open issue for any of the elements that we should address in future PRs based on the code that we pull in here.

Examples for this include issues such as #56.

Feel free to ping me here once this is done so I can merge the PR using admin permissions despite the falling code coverage. We could see if we can pull some UI tests from SpeziFHIR in here if possible.

dismiss()
}
}
}
}
}

private var downloadModelSettings: some View {
Copy link
Collaborator

@jdisho jdisho Jan 19, 2025

Choose a reason for hiding this comment

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

This appears to be an addition to what we have in main. Since this PR primarily focuses on moving the app code from SpeziFHIR to LLMonFHIR, I'm wondering if this change belongs here. Also, I noticed that even after downloading the model, the chat still interacts with OpenAI, which makes me question its current value. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right. I think there is some code that is still in progress to switch from OpenAI models to local ones. Maybe we should first merge everything "as is" into the different repositories and afterwards make changes to, e.g. use the local model.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fine by me. But since we're merging this to main, we should at least make this feature inaccessible in Settings. Should be a quick change, which I can do it right now.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds great thank you @jdisho!
Let's add an issue for this in this repo so we don't forget about this when the PR is merged.

@jdisho
Copy link
Collaborator

jdisho commented Jan 19, 2025

We could see if we can pull some UI tests from SpeziFHIR in here if possible.

@PSchmiedmayer - Great idea! I can take care of this both in SpeziFHIR and here (new PR).

@PSchmiedmayer
Copy link
Member

Amazing; thank you @jdisho!

Should I merge the PR as is despite the code coverage drops using admin permissions?

@jdisho
Copy link
Collaborator

jdisho commented Jan 20, 2025

Let's merge it 🚀
Is that okay with you @LeonNissen?

@PSchmiedmayer PSchmiedmayer merged commit 5a3b793 into main Jan 20, 2025
6 of 8 checks passed
@PSchmiedmayer PSchmiedmayer deleted the feature/local-llm branch January 20, 2025 07:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants