-
Notifications
You must be signed in to change notification settings - Fork 6.7k
fix: skip _fix_blob_seq_ids sqlite open on already-migrated palaces (#1090) #1177
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -159,6 +159,9 @@ def _pin_hnsw_threads(collection) -> None: | |
| logger.debug("_pin_hnsw_threads modify failed", exc_info=True) | ||
|
|
||
|
|
||
| _BLOB_FIX_MARKER = ".blob_seq_ids_migrated" | ||
|
|
||
|
|
||
| def _fix_blob_seq_ids(palace_path: str) -> None: | ||
| """Fix ChromaDB 0.6.x -> 1.5.x migration bug: BLOB seq_ids -> INTEGER. | ||
|
|
||
|
|
@@ -168,10 +171,19 @@ def _fix_blob_seq_ids(palace_path: str) -> None: | |
| type INTEGER) is not compatible with SQL type BLOB". | ||
|
|
||
| Must run BEFORE PersistentClient is created (the compactor fires on init). | ||
|
|
||
| Opening a Python sqlite3 connection against a ChromaDB 1.5.x WAL-mode | ||
| database leaves state that segfaults the next PersistentClient call. After | ||
| the migration has run once successfully, a marker file is written so | ||
| subsequent opens skip the sqlite connection entirely. Already-migrated | ||
| palaces can touch the marker manually to opt into the fast path. | ||
| """ | ||
| db_path = os.path.join(palace_path, "chroma.sqlite3") | ||
| if not os.path.isfile(db_path): | ||
| return | ||
| marker = os.path.join(palace_path, _BLOB_FIX_MARKER) | ||
| if os.path.isfile(marker): | ||
| return | ||
| try: | ||
| with sqlite3.connect(db_path) as conn: | ||
|
Comment on lines
188
to
189
|
||
| for table in ("embeddings", "max_seq_id"): | ||
|
|
@@ -189,6 +201,14 @@ def _fix_blob_seq_ids(palace_path: str) -> None: | |
| conn.commit() | ||
| except Exception: | ||
| logger.exception("Could not fix BLOB seq_ids in %s", db_path) | ||
| return | ||
| # Write marker whether or not rows needed migration — the palace is now | ||
| # confirmed to be in the INTEGER-seq_id state and future opens can skip the | ||
| # sqlite3.connect() entirely. | ||
| try: | ||
| open(marker, "a").close() | ||
| except OSError: | ||
| logger.exception("Could not write migration marker %s", marker) | ||
|
|
||
|
|
||
|
Comment on lines
+191
to
214
|
||
| # --------------------------------------------------------------------------- | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New marker-file behavior isn’t covered by tests: there are existing tests for
_fix_blob_seq_ids, but none assert (1) marker creation on successful no-op/migration, and (2) early return when the marker exists (ensuringsqlite3.connect()is not called). Adding/adjusting tests would prevent regressions on this reliability guard.