Faiss in CI and default vector db#12265
Conversation
enable unit tests
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| recover_path = Path(self.path).parent / "recover" | ||
| if recover_path.exists(): | ||
| # move all files from recover dir that might be left after index failing | ||
| for item in recover_path.iterdir(): | ||
| if item.is_dir(): | ||
| continue | ||
| item.rename(Path(self.path).parent / item.name) |
There was a problem hiding this comment.
Correctness: In FaissIndex.__init__, moving files from recover_path to the parent directory is performed unconditionally. If a crash occurs after a new index is saved but before the recover directory is deleted, this logic will overwrite the new index with the old backup. This recovery should only be performed if the primary index file (self.path) does not already exist.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `mindsdb/integrations/handlers/duckdb_faiss_handler/faiss_index.py` (lines 57-63), prevent recovery from overwriting a valid index. Update the recovery guard to only move files back when the main index file is missing (e.g., `if recover_path.exists() and not os.path.exists(self.path):`). Ensure existing index files are not replaced by stale recover artifacts.
| # remove index files (everything except duckdb) | ||
| for item in Path(self.path).parent.iterdir(): | ||
| if item.is_dir() or item.name.startswith("duckdb."): | ||
| continue |
There was a problem hiding this comment.
Correctness: The drop method iterates over the parent directory and unlinks all files that do not start with duckdb.. This is destructive and will delete unrelated files stored in the same directory. Limit the deletion to files explicitly managed by this class, for example by checking if the filename starts with faiss_index.
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `mindsdb/integrations/handlers/duckdb_faiss_handler/faiss_index.py` around lines 203-206, `drop()` currently unlinks every non-`duckdb.*` file in the index directory. This risks deleting unrelated files. Update the loop to only delete FAISS-related files (e.g., names starting with `faiss_`). Apply the provided diff suggestion exactly.
| now_ts = time.time() | ||
| to_close: List[str] = [] | ||
| for table_name, entry in self.tables_cache.items(): |
There was a problem hiding this comment.
Correctness: in_use_count, but increments/decrements happen outside tables_cache_lock in open_table. This allows a race where in_use_count is observed as 0 while another thread is about to use the table, causing it to be closed mid-use. 🔥 Move the in_use_count/last_used_ts updates under the same lock (or use a dedicated lock) so pruning sees consistent state.
Affected Locations:
- mindsdb/integrations/handlers/duckdb_faiss_handler/duckdb_faiss_handler.py:149-151
- mindsdb/integrations/handlers/duckdb_faiss_handler/duckdb_faiss_handler.py:185-197
🤖 AI Agent Prompt for Cursor/Windsurf
📋 Copy this prompt to your AI coding assistant (Cursor, Windsurf, etc.) to get help fixing this issue
In `mindsdb/integrations/handlers/duckdb_faiss_handler/duckdb_faiss_handler.py` around lines 149-151, `_close_old_tables_cache` prunes based on `entry.in_use_count`. Update `open_table` so increments/decrements of `entry.in_use_count` (and `last_used_ts`) are performed while holding `self.tables_cache_lock`, or otherwise synchronize access, to prevent pruning a table while it is being used.
# Conflicts: # mindsdb/api/executor/sql_query/sql_query.py
# Conflicts: # mindsdb/api/executor/sql_query/sql_query.py # mindsdb/interfaces/knowledge_base/controller.py
|
@mindsdb-devops, I made some updates of CI actions in this PR: replaced chromadb with faiss |
# Conflicts: # requirements/requirements-kb.txt
2 participants
Description
Updates:
Fixes https://linear.app/mindsdb/issue/FQE-2098/enable-unit-tests-for-macos-and-windows
Type of change
(Please delete options that are not relevant)
Verification Process
To ensure the changes are working as expected:
Additional Media:
Checklist:
Confidence Score: 3/5 - Review Recommended
Files requiring special attention
mindsdb/interfaces/knowledge_base/controller.py