Swift 6 Complete Concurrency Checking #85
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #85 +/- ##
=======================================
Coverage 38.57% 38.57%
=======================================
Files 64 64
Lines 2331 2331
=======================================
Hits 899 899
Misses 1432 1432
Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Thanks @jdisho for the PR, great work! 🚀
As you mentioned, it makes sense to wait for the SpeziLLM OpenAI layer to be rewamped, but you can already start tackling the local layer in the meantime.
Also, as mentioned, SpeziSpeech and SpeziChat need to be lifted to Swift 6 language mode as well at some point, let me know if you're interested in that!
Edit: Tackled in StanfordSpezi/SpeziSpeech#10 and StanfordSpezi/SpeziChat#20
Package.swift
Outdated
| #if swift(<6) | ||
| let swiftConcurrency: SwiftSetting = .enableExperimentalFeature("StrictConcurrency") | ||
| #else | ||
| let swiftConcurrency: SwiftSetting = .enableUpcomingFeature("StrictConcurrency") | ||
| #endif | ||
|
|
There was a problem hiding this comment.
No need to do that if you just specify // swift-tools-version:6.0 at the top of the file.
See https://github.com/StanfordSpezi/Spezi/blob/main/Package.swift
|
@philippzagar - Thanks for the input! This is great 🚀 |
|
Ah, I just saw that you opened the PRs already. |
| } | ||
|
|
||
| @MainActor | ||
|
|
||
| private func downloadWithHub() async throws { | ||
| let repo = Hub.Repo(id: model.hubID) | ||
| let modelFiles = ["*.safetensors", "config.json"] | ||
|
|
||
| try await HubApi.shared.snapshot(from: repo, matching: modelFiles) { progress in |
There was a problem hiding this comment.
This is forcing me to make the whole class @unchecked Sendable, because we can't pass progress safely without potentially causing data races. Do you have another idea?
There was a problem hiding this comment.
Yeah we don't want to mark the manager Sendable cause it isn't.
The issue is the double capture of self, once within the snapshot() callback and once within the Task.
Did you try something like (rough sketch)
try await HubApi.shared.snapshot(from: repo, matching: modelFiles) { progress in
Task { @MainActor [mutate = self.mutate] in
mutate(progress)
}
}
}
@MainActor private func mutate(progress: Progress) {
self.state = .downloading(progress: progress)
}There was a problem hiding this comment.
That could work; I agree that marking the complete class as sendable without any manual locking implementations might not be great so I would aim to avoid it if we can or mark everything with main actor (which is also not great) which could basically also guarantee that?
There was a problem hiding this comment.
As you mentioned, marking everything as MainActor would be the simple solution here. However, we should then double-check that most long running operations in the download manager are performed on some arbitrary thread (e.g. the download of the model itself, copying over the model from the temporary directory to a more permanent one, etc.).
However, I'm not sure if that is really necessary if one includes some ugly workarounds (as shown above).
There was a problem hiding this comment.
Thank you for continuing to work on this @jdisho & @philippzagar! 🚀
| @@ -61,7 +54,7 @@ extension LLMFogSession { | |||
| } | |||
| } catch let error as APIErrorResponse { | |||
| // Sadly, there's no better way to check the error messages as there aren't any Ollama error codes as with the OpenAI API | |||
| if error.error.message.contains(Self.modelNotFoundRegex) { | |||
| if error.error.message.range(of: Self.modelNotFoundRegex, options: .regularExpression) != nil { | |||
| @@ -35,7 +34,7 @@ let package = Package( | |||
| .package(url: "https://github.com/StanfordSpezi/SpeziFoundation", from: "2.0.0"), | |||
| .package(url: "https://github.com/StanfordSpezi/SpeziStorage", from: "1.0.2"), | |||
| .package(url: "https://github.com/StanfordSpezi/SpeziOnboarding", from: "1.1.1"), | |||
| .package(url: "https://github.com/StanfordSpezi/SpeziChat", .upToNextMinor(from: "0.2.1")), | |||
| .package(url: "https://github.com/StanfordSpezi/SpeziChat", .upToNextMinor(from: "0.2.2")), | |||
| .package(url: "https://github.com/StanfordSpezi/SpeziViews", from: "1.3.1") | |||
There was a problem hiding this comment.
We should try to unify this across all packages, some of them use .git at the end, some of them don't. I would suggesting adding .git for all of them for now.
| } | ||
|
|
||
| @MainActor | ||
|
|
||
| private func downloadWithHub() async throws { | ||
| let repo = Hub.Repo(id: model.hubID) | ||
| let modelFiles = ["*.safetensors", "config.json"] | ||
|
|
||
| try await HubApi.shared.snapshot(from: repo, matching: modelFiles) { progress in |
There was a problem hiding this comment.
That could work; I agree that marking the complete class as sendable without any manual locking implementations might not be great so I would aim to avoid it if we can or mark everything with main actor (which is also not great) which could basically also guarantee that?
Addressing: #54
Swift6 - Complete Concurrency Checking
♻️ Current situation & Problem
Xcode is throwing concurrency warnings for the
SpeziLLM,SpeziLLMFog,SpeziLLMLocal,SpeziLLMLocalDownload, andSpeziLLMLocalOpenAItargets because they currently have concurrency concerns.⚙️ Release Notes
SpeziLLM,SpeziLLMFog,SpeziLLMLocalandSpeziLLMLocalDownloadare resolved. For the remaining targets, it would be best to wait until SpeziLLMOpenAI: Replace MacPaw/OpenAI With Generated API Calls #64 is merged.📚 Documentation
No additional changes.
✅ Testing
Warnings will be resolved after #64 is merged.
📝 Code of Conduct & Contributing Guidelines
By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: