Make WhisperKit Sendable and add locks#427
Open
naykutguven wants to merge 2 commits intoargmaxinc:swift-6from
Open
Make WhisperKit Sendable and add locks#427naykutguven wants to merge 2 commits intoargmaxinc:swift-6from
naykutguven wants to merge 2 commits intoargmaxinc:swift-6from
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces comprehensive thread safety improvements to the WhisperKit core class by synchronizing all mutable state with an unfair lock and marking the class as @unchecked Sendable. The changes also include extensive test coverage to verify property synchronization, callback behavior, and concurrent access patterns.
Changes:
- Refactored
WhisperKitto useOSAllocatedUnfairLockfor synchronizing all mutable properties, ensuring thread-safe access across concurrency domains - Added comprehensive unit tests for tokenizer synchronization, decoder replacement, callback ordering, and concurrent property access
- Introduced test utilities including
LockedStorefor thread-safe test state aggregation andWhisperTokenizerMockfor controlled testing
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Sources/WhisperKit/Core/WhisperKit.swift | Transformed all public properties to use private backing variables with lock-protected getters/setters; added @unchecked Sendable conformance and comprehensive documentation |
| Tests/WhisperKitTests/WhisperKitTests.swift | Added new test suite verifying tokenizer synchronization, decoder replacement behavior, callback transition order, and concurrent property access |
| Tests/WhisperKitTests/Utils/LockedStore.swift | Introduced utility class for thread-safe state aggregation in tests using OSAllocatedUnfairLock |
| Tests/WhisperKitTests/Mocks/WhisperTokenizerMock.swift | Added mock tokenizer implementation for controlled testing of tokenizer-related features |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
d110539 to
6d4e8c9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces extensive improvements to the
WhisperKitcore class, focusing on thread safety, property synchronization, and test coverage. The main change is refactoringWhisperKitto synchronize all mutable state using an unfair lock, ensuring safe concurrent access. The update also adds comprehensive unit tests and utilities to verify correct property synchronization and callback behavior.Thread safety and state synchronization:
WhisperKitto synchronize all mutable properties using anOSAllocatedUnfairLock, ensuring thread-safe access and mutation across concurrency domains. Added awithStateLockhelper for internal synchronization.WhisperKitas@unchecked Sendableand documented thread-safety guarantees, clarifying that injected dependencies must manage their own synchronization.Property synchronization and callbacks:
WhisperKitto use synchronized backing variables, ensuring atomic access. Enhanced synchronization betweentokenizerandtextDecoder.tokenizerfor consistency.modelStateCallback, ensuring correct transition order and thread safety during state changes.Testing and utilities:
WhisperKitTests.swiftwith tests for tokenizer synchronization, decoder replacement, callback transition order, and concurrent property access, verifying thread safety and correct behavior.LockedStoreutility for safely aggregating state in tests, using an unfair lock for thread-safe mutation.WhisperTokenizerMockfor controlled testing of tokenizer-related features.