All critical bugs (thread‑safety, transaction scope, tag filtering, …#1
Merged
Conversation
…blocking saves, fragile JSON queries, and serialization) have been fixed SqlServerStoreBase – Changed to create a new pooled connection per call (matching Postgres) with thread‑safe locking for externally provided connections, and updated tests accordingly. PostgresModelStore.LoadAsync – Already correct (uses a transaction for metadata + large objects) – no changes needed. FileSystem stores – Implemented in‑memory tag filtering in ListAsync (using LoadManifestOnlyAsync) and updated tests to expect filtering. BackgroundSaver – Converted Enqueue to EnqueueAsync (returns ValueTask) and updated CheckpointManager/SessionManager call sites to await it. PostgresStoreBase – Already correct (all callers use await using) – no changes needed. SQL Server tag filtering – Replaced fragile LIKE with JSON_VALUE and corrected table names (ModelManifests, InferenceSessions). IModelStore.ListAsync – Not yet addressed (design improvement deferred to a separate PR). CheckpointManager.SaveAsync – Added overload that accepts a ModelCheckpoint object directly. TokenizerData.MergeRules – Changed property type from List<(string,string)>? to List<MergeRule>? for proper JSON serialization. SqlServerStoreBase namespace – Corrected from Mysql to MsSql and updated using statements. Removed sqlserver (previously wrongly named MySql) and Postgres specific store namespace and flattened all to store only.
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
1 participant
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.
…blocking saves, fragile JSON queries, and serialization) have been fixed
SqlServerStoreBase – Changed to create a new pooled connection per call (matching Postgres) with thread‑safe locking for externally provided connections, and updated tests accordingly.
PostgresModelStore.LoadAsync – Already correct (uses a transaction for metadata + large objects) – no changes needed.
FileSystem stores – Implemented in‑memory tag filtering in ListAsync (using LoadManifestOnlyAsync) and updated tests to expect filtering.
BackgroundSaver – Converted Enqueue to EnqueueAsync (returns ValueTask) and updated CheckpointManager/SessionManager call sites to await it.
PostgresStoreBase – Already correct (all callers use await using) – no changes needed.
SQL Server tag filtering – Replaced fragile LIKE with JSON_VALUE and corrected table names (ModelManifests, InferenceSessions).
IModelStore.ListAsync – Not yet addressed (design improvement deferred to a separate PR).
CheckpointManager.SaveAsync – Added overload that accepts a ModelCheckpoint object directly.
TokenizerData.MergeRules – Changed property type from List<(string,string)>? to List? for proper JSON serialization.
SqlServerStoreBase namespace – Corrected from Mysql to MsSql and updated using statements.
Removed sqlserver (previously wrongly named MySql) and Postgres specific store namespace and flattened all to store only.