Fix thread safety and Swift 6 warnings. #123 #125
Conversation
Libraries/LLM/Cohere.swift
Outdated
| @@ -145,7 +145,7 @@ public class CohereModelInner: Module { | |||
| } | |||
| } | |||
|
|
|||
| public class CohereModel: Module, LLMModel, KVCacheDimensionProvider { | |||
| public class CohereModel: Module, LLMModel, KVCacheDimensionProvider,@unchecked Sendable { | |||
There was a problem hiding this comment.
The models can't be @unchecked Sendable as they are entirely not thread safe (the MLXArrays themselves cannot be used in a multithreaded manner)
There was a problem hiding this comment.
If @unchecked Sendable is not used, there will be a Task warning in createModelSynchronous.
Value of non-Sendable type '@isolated(any) @async @callee_guaranteed @substituted <τ_0_0> () -> @out τ_0_0 for <()>' accessed after being transferred; later accesses could race; this is an error in the Swift 6 language mode
| semaphore.signal() | ||
| } | ||
|
|
||
| semaphore.wait() |
There was a problem hiding this comment.
Using semaphores like this is an anti-pattern:
| func setReplacement(_ replacement: String, for tokenizer: String) { | ||
| replacementTokenizers[tokenizer] = replacement | ||
| } | ||
| } |
There was a problem hiding this comment.
This part is fine but as you noted it makes async leak into parts we don't want. I think this could be encapsulated like this but maybe use an NSLock to guard the critical section, then it stays synchronous
|
- replaces #125 with simpler mechanism (NSLock) Co-authored-by: John Mai <maiqingqiang@gmail.com>
|
Check out #126 -- I used what you did but made it NSLock instead. This kept everything synchronous but let's use also make it thread safe. |
Nice! I've been a bit busy lately, so I'll close the current PR first. |
- replaces #125 with simpler mechanism (NSLock) Co-authored-by: John Mai <maiqingqiang@gmail.com>
…lore#126) - replaces ml-explore#125 with simpler mechanism (NSLock) Co-authored-by: John Mai <maiqingqiang@gmail.com>
Fix thread safety and Swift 6 warnings. #123