Fix/avoid implicit cuda0 - #97
Closed
NLP-traveller wants to merge 102 commits into
Closed
NLP-traveller wants to merge 102 commits into
NLP-traveller wants to merge 102 commits into
Conversation
- actions/checkout@v4 -> @v5 - astral-sh/setup-uv@v3 -> @v8.1.0 (pinned exact tag; no floating v8 tag) - softprops/action-gh-release@v2 -> @V3 - explicit 'version: latest' on setup-uv Silences the Node.js 20 deprecation warnings reported on recent runs. No version bump or tag triggered by this commit.
- vector_db_server: add GET /v1/admin/indexes and /v1/admin/data_folders, read-only walks of data_dir with a 60s TTL size cache. Auth follows the existing bearer-token middleware (reused api_key). Cache is invalidated on collection create/upsert/delete. - embedding_server: add EmbeddingServerManager.device_info() which queries the remote host with nvidia-smi via the existing SSH channel and returns structured GPU info. Never raises. - colpali: add an optional on_progress callback to MultiModalRetrieverModel.index / add_to_index. Emits start, file_start, page, file_done and all_done events. Best-effort: callback exceptions are swallowed. - Tests: admin endpoints (12), device_info (3), on_progress (2).
- VectorDBServerManager and EmbeddingServerManager gain: * redeploy(on_line=None): force-rebuild + restart regardless of state * is_running(): non-raising container status probe * get_remote_metadata(): non-raising metadata getter - _run_remote(cmd, on_line=None): streams stdout line by line into a callback when provided, otherwise preserves the existing buffered behaviour. Used by interactive UIs (Streamlit) to surface Docker build/pull/run output in real time. - _deploy(on_line=None): threads the callback through every long-running remote command. Tests: +8 (redeploy, on_line streaming, no-callback fallback for both managers).
Both VectorDBServerManager and EmbeddingServerManager were calling
paramiko.SSHClient.connect with a raw hostname string, bypassing
~/.ssh/config. This caused socket.gaierror ('Name or service not
known') for any host that exists only as an alias in ~/.ssh/config
(e.g. with a ProxyCommand for a host behind a bastion).
Introduce foretrieval.ssh_utils.open_ssh_client(), a small helper
that:
- Parses ~/.ssh/config via paramiko.SSHConfig
- Resolves HostName, User, Port, IdentityFile, ProxyCommand and
ProxyJump
- Honours explicit overrides from the manager config (ssh_user,
ssh_key_path) over the SSH config
- Falls back to a direct connect when the SSH config file is
missing, malformed, or has no matching Host block
Both managers' _get_ssh() delegate to this helper. Existing tests
unaffected (they mock _run_remote, not _get_ssh).
Tests: +12 (9 ssh_utils + 2 manager regressions + 1 cache check).
…a paths
The previous code used '~/foretrieval_db_build' for SFTP put and then
stripped the '~' with .replace('~', ''), producing a root-relative
'/foretrieval_db_build' path. SFTP returned ENOENT (FileNotFoundError)
on every real deploy. The remote shell happens to expand '~' in plain
_run_remote calls, so cd/cat/echo on the same path coincidentally
worked — masking the bug for some operations.
Fix:
- Drop '~/'-prefixed remote constants. Use relative subpaths:
_REMOTE_BUILD_SUBDIR = 'foretrieval_db_build'
_REMOTE_METADATA_SUBPATH = '.foretrieval/db_deployment.json' (vdb)
_REMOTE_METADATA_SUBPATH = '.foretrieval/deployment.json' (embed)
- New _remote_home() helper on both managers caches the result of
sftp.normalize('.') for the duration of the manager's life.
- _upload_build_context, _read_remote_metadata, _write_remote_metadata,
stop(): build absolute paths by joining _remote_home() with the
relative subpath. No more '~' anywhere in remote command strings.
Tests: +7 (remote home resolution + caching, absolute metadata path,
absolute build-context SFTP destination, regression for the
stop()-removes-tilde-path case).
Files written by the FORetrieval vector-DB container under the bind-mounted data_dir (/opt/for_index_db) were owned by root because the container ran as root. This caused an ownership mismatch with SFTP-uploaded _data/ directories (owned by the SSH user) and prevented the SSH user from cleaning up or replacing index files. - New _resolve_remote_uid_gid() helper: runs 'id -u && id -g' on the remote host via the existing SSH channel. Returns (uid, gid) on success or (None, None) on any failure. - _build_docker_run_cmd(): adds '--user uid:gid' when resolution succeeds, so container-written files are owned by the SSH user on the host. Falls back silently when resolution fails (backward compatible with hosts without 'id' or where SSH is not configured). - _make_manager() test helper now also stubs _resolve_remote_uid_gid so the existing _build_docker_run_cmd tests are unaffected. Tests: +2 (--user flag present when uid resolved; absent when None).
The vector-DB server was unreachable because Python initialises the
top-level foretrieval package when any sub-module is imported with
'-m foretrieval.vector_db_server.server_main'. The eager __init__
pulled in colpali.py -> colpali_engine -> transformers -> torch, and
torch._dynamo crashed on the CPU-only Docker image:
AssertionError: Artifact of type=precompile already registered
in mega-cache artifact factory
The server does not use ColPali, transformers or torch at all — it
only needs foretrieval.vector_db_server.* and foretrieval.vector_store.*.
Those sub-packages use relative imports and never touch the top-level
__init__, so the fix is simply to make __init__.py lazy.
Switch to PEP 562 module-level __getattr__:
from foretrieval import MultiModalRetrieverModel # still works
import foretrieval; foretrieval.MultiModalRetrieverModel # still works
import foretrieval # no longer imports colpali/torch as a side-effect
Tests: +4 (no colpali side-effect, both public names accessible,
AttributeError on unknown names).
…_dir GET /v1/admin/indexes only returned an entry if the directory contained index_config.json.gz or metadata.json.gz. Those files are client-side sidecars written to ~/.forag/remote_indexes on the Streamlit host, never to the server's data_dir. So the endpoint always returned an empty list and the Load tab showed 'No indexes found.' The correct sentinel is index.json, written by _write_meta() on every POST /v1/collection, POST /v1/collection/open, and lazy upsert. This file exists in data_dir/<name>/ on the server from the moment a collection is created. Updated the test fixture to write index.json instead of index_config.json.gz.
device_info() queried the remote host via nvidia-smi over SSH to surface GPU load in the FORag Streamlit UI. That GPU panel has been removed from the UI; device_info() has no remaining callers. Remove the method and its three unit tests from test_embedding_server.py. n_gpus and _resolve_n_gpus (used for vLLM tensor-parallel sizing) are unchanged.
Add server-side persistence of ColPali index bookkeeping (model name,
doc metadata, file-name map, per-embedding extras) so that in remote
mode the client no longer needs a local index directory.
- base.VectorStore: add export_bookkeeping/load_bookkeeping (no-op
default) + supports_remote_bookkeeping().
- vector_db_server.server: PUT/GET /v1/collection/{name}/bookkeeping,
storing the blob under <data_dir>/<name>/bookkeeping.pt.
- vector_db_server.client: put_bookkeeping/get_bookkeeping.
- vector_store.remote: implement bookkeeping methods forwarding to the
client.
- colpali: in remote mode, route _export_index/_load_index_state through
the vector store instead of local sidecar files; from_index bootstraps
the model name and state from the server (connection config supplied by
the caller, never persisted).
- retriever.from_index: accept storage_backend to force remote loading.
Local/qdrant/milvus backends are unchanged and keep their local sidecar
files. Tests added for the new endpoints, client methods and remote
store; remote-backend dispatch tests updated for the new behaviour.
In remote mode the local index directory is irrelevant (vectors and bookkeeping live on the server). The existing index() guard treated a stale local index/<name> directory as 'already exists' and aborted indexing, leaving the vector store unopened and causing search() to fail. Skip that guard when remote bookkeeping is in use.
- index(), _process_directory(), update_index_from_folder() now use
rglob('*') instead of iterdir(), so all files in subdirectories are
indexed automatically
- replace print() in _process_directory with logger.debug + tqdm progress
- remove 9 unused imports found by ruff (F401) across 6 files
- fix 2 empty f-strings (F541)
- fix _FakeColPaliModel in test_colpali_on_progress: add missing
storage_backend and storage_config attributes
- add test_recursive_indexing.py: 6 unit tests (no GPU) covering
index(), _process_directory(), update_index_from_folder() recursive
traversal and overwrite=False with no existing index
- remove test_colpali.py and test_colqwen.py: load-only tests with no
functional assertions, superseded by test_e2e_rag.py
- update README: note recursive indexing behavior
test_init_lazy_import.py purges all foretrieval.* entries from sys.modules to verify lazy-import semantics. Without cleanup this permanently splits module identity for the rest of the pytest session: tests that ran later would hold OLD class objects (bound at collection time) while sys.modules contained NEW re-imported versions, causing isinstance() checks, patch() targets, and module-global reads to all fail silently. Add an autouse function-scoped fixture that snapshots sys.modules before each test and fully restores it (including overwriting any re-imported variants) in teardown. This eliminates 10 order-dependent failures in test_qdrant.py, test_vector_store_qdrant.py, test_vector_store_milvus.py, test_vector_db_server_app.py, and test_vector_store_factory.py. Fast suite: 362 passed, 0 failed.
…olormap Bug 1 (models_metadata.py): build_metadata_list_for_dir was non-recursive (iterdir + None placeholders for dirs) while index() enumerates files recursively via rglob. Rewritten to match index() exactly: rglob sorted by relative path, one DocMetadata per file, no dir placeholders. Bug 2 (colpali.py): add _cleanup_failed_index helper that removes the partial local index directory (shutil.rmtree) and best-effort deletes the remote collection on failure. index() now wraps its body in try/except and calls this helper on any exception, ensuring a clean retry is always possible. Bug 3 (colpali.py): _load_index_state and _apply_bookkeeping_blob derived doc_ids / highest_doc_id from doc_id_to_metadata.keys(). When an index was built without add_metadata those keys are empty, leaving doc_ids=set() and highest_doc_id=-1 despite documents being present. Fixed to derive from doc_ids_to_file_names (always written), falling back to metadata keys only for legacy indexes that lack a filenames sidecar. Bug 4 (plot_utils.py): replace deprecated matplotlib.cm.get_cmap (removed in matplotlib >= 3.9) with matplotlib.colormaps[cmap]. Tests: update test_metadata_no_ai.py for new recursive semantics; add test_task10_bugfixes.py covering Bugs 2, 3, and 4.
…ata load, colormap, filename display (v2026.6.2)
…e argmax draw_circle_on_max_patch now accepts patch_grow_pct/grow_mode params and applies grow_heatmap_patches_torch before computing the argmax patch, so the circle center lands on the same visual peak as the heatmap overlay. Colpali call site updated to pass patch_grow_pct=300.0, grow_mode='mean' matching the heatmap call (same values, same function order). grow_heatmap_patches_torch moved before draw_circle_on_max_patch to fix forward-reference (no-op at runtime, required for clarity). Tests: 8 new tests in tests/test_task11_features.py (all pass).
Author
|
Just a mistake. Sorry |
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
4 participants
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.
Replace "cuda:0" by the use of given device