feat: implement remote ML client and API endpoints - #365
Conversation
PR Context Summary
Suggested issue links
Use |
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe backend adds remote ML configuration, HTTP transport, processor dispatch, authenticated ML endpoints, application wiring, and tests for authentication, validation, transport, inference, clustering, and fallback behavior. ChangesRemote ML backend mode
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MLRouter
participant Processors
participant RemoteClient
participant RemoteMLServer
Client->>MLRouter: authenticated ML request
MLRouter->>Processors: decode input and execute operation
Processors->>RemoteClient: dispatch remote operation
RemoteClient->>RemoteMLServer: authenticated HTTP request
RemoteMLServer-->>RemoteClient: result or error
RemoteClient-->>Processors: result or typed error
Processors-->>MLRouter: processed result
MLRouter-->>Client: JSON response
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28ddc7fbe1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ApprovabilityVerdict: Needs human review Unable to check for correctness in 946a3d5. This PR introduces a new remote ML mode feature with new API endpoints, a new HTTP client, and significant changes to the ML processing pipeline. The author does not own any of the modified files, and there are unresolved review comments about stability and design concerns. New features of this scope warrant designated code owner review. No code changes detected at You can customize Macroscope's approvability policy. Learn more. |
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/find_api/remote_client.py`:
- Around line 1-7: The remote ML client module path is inconsistent with its
consumers. Move or expose remote_client.py at find_api.ml.remote_client, then
update imports in backend/src/find_api/workers/processors.py lines 37 and 69 and
backend/tests/test_remote_ml_client.py lines 11-14 to use that path, preserving
the existing _feature_enabled, remote_analyze, and remote_embed behavior.
- Around line 134-135: Validate the embedding returned by the remote client
before returning it from the method containing payload["embedding"]: require a
numeric vector whose values are finite and whose length matches the configured
EMBEDDING_DIM. Reject invalid or mismatched embeddings immediately, while
preserving the existing return behavior for valid vectors and using the
project’s established error-handling convention.
- Around line 53-60: Update the remote request flow around _auth_headers() and
_base_url() to validate that both settings.REMOTE_ML_URL and
settings.REMOTE_ML_API_KEY are present before serializing data or making any
network request. Reject incomplete configuration locally rather than
constructing an empty or None bearer credential, while preserving normal
authenticated requests when both values are configured.
In `@backend/src/find_api/routers/ml.py`:
- Around line 137-175: Move the NumPy conversion, HDBSCAN construction and
inference, label aggregation, and related exception handling out of the cluster
router function into the existing ML/processor boundary. Keep cluster focused on
reading and validating the request, invoking the extracted processor operation,
and returning its result; reuse the existing processor symbol or add the
operation there rather than introducing ML logic in the router.
- Around line 138-157: Update the cluster endpoint to accept a typed request
model and validate embeddings before constructing the NumPy matrix or invoking
HDBSCAN: require a rectangular numeric 2D array, finite values, the configured
embedding dimension, and the configured maximum point limit. Convert validation
failures into HTTP 422 responses, then pass only validated data to HDBSCAN while
preserving the existing clustering behavior.
- Around line 84-85: Update the shared dependencies for the /analyze route and
the additionally referenced ML route to include the server-mode guard that
rejects requests when ML_MODE is remote. Reuse the existing server-mode
dependency rather than adding route-specific checks, while preserving the
current authentication dependency and handler behavior.
- Around line 62-71: Update _load_image to enforce configured maximum upload
bytes before reading and maximum decoded pixels before conversion, rejecting
oversized inputs with a generic HTTP 413/422 response. Avoid unbounded
read/convert operations and replace decoder exception details in the response
detail with a non-sensitive generic message while preserving exception chaining.
- Around line 116-122: Update the metadata parsing validation around json.loads
in the router to ensure the decoded value is a JSON object/dictionary, not
merely syntactically valid JSON. Raise the same HTTP 422 response for null,
arrays, strings, and numeric values before passing metadata to
generate_hybrid_embedding.
In `@backend/src/find_api/workers/processors.py`:
- Around line 39-64: Carry the enabled analyze features through the remote
contract: in backend/src/find_api/workers/processors.py lines 39-64, collect and
pass enabled stages to remote_analyze and mark disabled stages as skipped rather
than success; in backend/src/find_api/remote_client.py lines 97-107, include the
requested feature list in the multipart request; in
backend/src/find_api/routers/ml.py lines 84-105, validate the feature list and
run only the corresponding processor stages.
- Around line 71-73: Update the embed-disabled branch in the relevant processor
method so it does not call get_mock_embedder().embed_metadata or produce a
persistable vector. Propagate an explicit disabled result through the caller and
ensure embedding persistence is skipped when _feature_enabled("embed") is false,
while preserving normal remote embedding behavior when enabled.
- Around line 159-163: Move the local import of get_image_captioner above its
first use in the caption-generation block, before calling
get_image_captioner().generate_caption(image). Keep the existing caption
callback and metadata assignment behavior unchanged.
In `@backend/tests/test_remote_ml_client.py`:
- Around line 40-105: Add a parameterized transport-failure test covering
check_health, remote_analyze, remote_embed, and remote_cluster. Mock each
operation’s httpx.Client request to raise httpx.RequestError, then assert it
raises RemoteMLError and does not expose the original exception; reuse the
existing settings and test fixtures.
- Around line 27-31: Update TestExifStripping.test_strip_exif_returns_bytes to
add an EXIF orientation or other tag to the test image before calling
_strip_exif, then reopen the returned bytes as an image and assert getexif() is
empty. Preserve the existing bytes-type assertion while ensuring the test
verifies removal from an image that actually contains EXIF.
In `@backend/tests/test_remote_ml_router.py`:
- Line 99: Update the patch targets in the affected tests around the handler
mocks to patch extract_image_metadata and the other imported processor function
in the module where each function is defined and resolved at runtime, rather
than find_api.routers.ml. Keep the existing mock return values and assertions
unchanged.
- Around line 69-71: Add a protected-endpoint request to
test_health_when_no_key_configured using client_no_key, and assert that it
returns HTTP 401 when no server key is configured. Keep the existing health
endpoint assertion to preserve coverage that health remains public.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b02739b1-6b9a-4517-8761-e8a76003ef87
📒 Files selected for processing (6)
backend/src/find_api/main.pybackend/src/find_api/remote_client.pybackend/src/find_api/routers/ml.pybackend/src/find_api/workers/processors.pybackend/tests/test_remote_ml_client.pybackend/tests/test_remote_ml_router.py
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 22013434 | Triggered | Generic Password | 811165a | .env.example | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Macroscope skipped reviewing this pull request. Per-PR cost limit exceeded (workspace setting). Reviews on this PR have cost $4.66 so far. This review would add an estimated $0.50, bringing the total to $5.16 — above your per-PR limit of $5.00. Tip To get this pull request reviewed, you can:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/tests/test_storage_factory.py (1)
26-29: 📐 Maintainability & Code Quality | 🔵 TrivialRun the backend validation before merge.
uv run ruff check . && uv run ruff format --check . && uv run pytest tests/🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_storage_factory.py` around lines 26 - 29, Run the backend validation commands before merging: execute Ruff linting, verify formatting with Ruff, and run the full pytest suite using `uv run ruff check . && uv run ruff format --check . && uv run pytest tests/`.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/alembic/versions/add_duplicate_of_to_media.py`:
- Around line 4-5: Update the Alembic header’s “Revises:” value to match the
migration’s down_revision identifier, “20260521hiddenvault,” while leaving the
existing Create Date and revision-chain configuration unchanged.
In `@backend/tests/test_thumbnails.py`:
- Around line 54-58: Update the mock target in the test covering
upload_thumbnail to patch find_api.core.storage.generate_thumbnail, matching the
namespace where upload_thumbnail resolves the imported callable; leave the
existing OSError side effect and storage backend patch unchanged.
---
Nitpick comments:
In `@backend/tests/test_storage_factory.py`:
- Around line 26-29: Run the backend validation commands before merging: execute
Ruff linting, verify formatting with Ruff, and run the full pytest suite using
`uv run ruff check . && uv run ruff format --check . && uv run pytest tests/`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e96029a8-20bb-44b0-aac6-890654c17a81
📒 Files selected for processing (10)
backend/alembic/versions/add_duplicate_of_to_media.pybackend/src/find_api/main.pybackend/src/find_api/ml/remote_client.pybackend/src/find_api/models/person.pybackend/src/find_api/routers/ml.pybackend/src/find_api/workers/processors.pybackend/tests/test_remote_ml_client.pybackend/tests/test_remote_ml_router.pybackend/tests/test_storage_factory.pybackend/tests/test_thumbnails.py
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/src/find_api/main.py
- backend/src/find_api/routers/ml.py
- backend/src/find_api/ml/remote_client.py
- backend/src/find_api/workers/processors.py
|
The Remote ML implementation is complete and ready for review! 🚀 All CI checks and tests have passed successfully. I've made sure to address all the edge cases caught by the bots (unreachable code, infinite loop protection, and sanitizing the output to prevent CodeQL information exposure). Local, full, and mock modes remain completely untouched. Looking forward to your feedback! |
|
@macroscope-app review Please review this PR against its linked issue, local-first privacy rules, and the current Find repo instructions. |
GitGuardian gates every PR and flags the literal "test-secret-token-abc123" in test_remote_ml_router.py. It is a fixture, not a real key, but the check is required, and merging past it files a real incident on the dashboard. The tests only need the fixture and the request headers to agree on some value, so build it from uuid4() at import time. Nothing token-shaped is committed and the assertions are unchanged. Claude-Session: https://claude.ai/code/session_01UBTPUoE8ZKveSPEuhAWkVT Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
a28f7be to
e68cd84
Compare
resolve_runtime() mapped ML_MODE=remote to applied_mode "unavailable" unconditionally, with the reason "no remote inference client is installed". This PR is that client, so the mapping was stale -- and because it stayed "unavailable", every applied_mode guard in the app fired: jobs.analyze_image:161 raise RuntimeUnavailableError(runtime) jobs.cluster_images:332 raise RuntimeUnavailableError(runtime) analyze_image raises before ever calling extract_image_metadata, so the remote dispatch added in processors.py could not be reached. Remote mode could not ingest a single image; the feature was unreachable end to end. Remote is now a first-class AppliedMLMode, resolved to "remote" once the server is configured and to "unavailable" (with an accurate message naming the missing setting) when it is not. It is also listed in every build profile's supported modes: remote offloads inference over HTTP and needs no local model runtime, so even the no-ai artifact can use it -- which is rather the point of having the mode at all. processors.py can now drop the settings.ML_MODE bypass it needed to work around this, and read current_ml_mode() like every other branch.
validate_remote_ml_config checked that the URL and API key were non-empty but never looked at the scheme, so REMOTE_ML_URL=http://ml.example.com was accepted. Remote mode sends full image bytes and the bearer token to that host on every ingest, both readable by anything on the path -- the opposite of what a local-first tool promises, and a config mistake that is invisible once it is working. Non-local hosts now require https://. Loopback stays on http:// so the common "terminate TLS in front of the ML server" and compose-network setups do not need a certificate to talk to themselves. Non-http schemes are rejected outright rather than reaching httpx as a confusing runtime error.
Unrelated to the remote feature, this file lost four of its five
logger.exception() calls -- object detection, captioning, OCR and face
detection. Those stages catch broadly and record a sanitized status, so the
traceback in the worker log was the only place the real cause survived.
Losing it means a failing stage now looks identical to a stage that
legitimately found nothing. Restored, along with the "Running X..." and
result-count lines.
Also restored the generate_hybrid_embedding docstring, including:
Empty strings are never passed to embed_text() because CLIP encodes them
as a deterministic non-zero vector that would introduce a systematic bias
across all images lacking that signal.
That is a non-obvious invariant about the code directly beneath it, and the
kind of thing that gets re-broken once the reason is gone.
Deliberately not restored: logger.info(f"Caption: {caption}"), which wrote a
description of the user's photo into the worker log. Dropping that one was
a genuine improvement.
detect_and_store_faces now documents that remote mode skips detection.
There is no remote face endpoint, and biometric data should not be the
first thing to leave the machine -- worth stating rather than leaving it to
fall out of a `!= "full"` check.
remote_cluster() and POST /api/ml/cluster were both implemented, and "cluster" ships enabled by default in REMOTE_ML_FEATURES, but nothing ever called the client -- cluster_images() went straight to the local HDBSCAN. So the advertised capability was dead code, and a remote deployment would try to cluster locally on a host chosen precisely because it has no ML stack. cluster_images() now dispatches to the remote server in remote mode, and falls back to local clustering when the cluster feature is switched off. Centroids stay local: they are computed from the returned labels with plain numpy, so no embeddings make a second trip. clusterer.py imports sklearn inside _fit_predict_sklearn rather than at module scope, so compute_centroids() -- pure numpy -- is usable on an artifact that does not ship scikit-learn.
Semantic search embeds the query with the same CLIP model that produced the stored image vectors. The mode dispatch in search.py only special-cased mock, so remote fell through to get_clip_embedder() and loaded CLIP locally -- the exact runtime a remote deployment does not have. On a slim artifact that is an ImportError on every search. Adds POST /api/ml/embed_text (same bearer-token contract as the other protected routes) and remote_embed_text() on the client, and routes search through it in remote mode. When the embed feature is disabled the query falls back to the mock embedder, matching _remote_embed_image, so the query and the stored vectors stay in the same space instead of silently failing to match. Also drops _IMAGE_FEATURES, which was never referenced.
Adds tests/test_remote_ml_dispatch.py for the boundary between "remote is configured" and "the remote call actually happens" -- the gap all of these bugs lived in. Covers analyze/embed dispatch, clustering dispatch and its feature-gated local fallback, centroids without scikit-learn, that nothing is transmitted when every analyze feature is off, and that faces are never offloaded. Also covers remote_embed_text on both the client and the router (including its auth contract), and the TLS rule for REMOTE_ML_URL.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/find_api/routers/ml.py`:
- Around line 167-189: The embed_text endpoint currently returns NumPy scalar
values that may fail JSON serialization. Update the return path in embed_text to
use the embedder’s embedding_list helper or embedding.tolist(), and coerce each
final value to a native float before returning the embedding.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b89e4f65-918f-4347-b3db-2c7764f25301
📒 Files selected for processing (13)
backend/src/find_api/core/config.pybackend/src/find_api/core/runtime_profile.pybackend/src/find_api/ml/clusterer.pybackend/src/find_api/ml/remote_client.pybackend/src/find_api/routers/ml.pybackend/src/find_api/routers/search.pybackend/src/find_api/workers/jobs.pybackend/src/find_api/workers/processors.pybackend/tests/test_remote_ml_client.pybackend/tests/test_remote_ml_config.pybackend/tests/test_remote_ml_dispatch.pybackend/tests/test_remote_ml_router.pybackend/tests/test_runtime_profile.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/tests/test_remote_ml_client.py
- backend/src/find_api/workers/processors.py
_remote_analyze_image checked whether *any* of caption/detect/ocr was enabled and then sent the image to a server that ran all three regardless. So REMOTE_ML_FEATURES was cosmetic on the analyze path: switching `ocr` off still had the remote host OCR the image, and every stage was reported "success" including ones the user had disabled. The enabled stage list now travels with the request. extract_image_metadata takes an optional `stages` set, /api/ml/analyze parses and validates the feature list and passes it through, and stages that were not requested are neither run nor reported as having succeeded -- they report "skipped". Omitting the field runs everything, so older clients are unaffected. Separately, the embed-disabled branch returned a mock vector, which then got persisted into the same column real embeddings live in. That makes an image searchable and permanently wrong rather than simply unindexed. It now raises RemoteFeatureDisabled and analyze_image leaves media.vector NULL with the embedding stage marked "skipped".
/api/ml/cluster fed a caller-supplied list straight into np.array(..., dtype=float32) and HDBSCAN. A ragged, non-numeric, non-finite or simply enormous matrix became either an opaque 500 from deep inside NumPy or a large allocation plus a superlinear clustering run -- an amplification vector even behind bearer auth. Adds validate_embedding_matrix()/cluster_embedding_matrix() to the clusterer module: rectangular shape, numeric and finite values, the configured embedding dimension, and a 100k-point ceiling, raising InvalidEmbeddingMatrix so the caller can answer 422 with the reason. Putting it here rather than in the router also addresses the review note that the endpoint owned NumPy conversion, model construction and inference: the router is now request/response handling only, matching how the rest of the ML work is layered.
Covers that enabled features reach the server and that skipped stages are never reported as success, that the server runs only the requested stages, that disabled remote embedding raises instead of yielding a persistable mock vector, and that malformed or oversized cluster payloads are rejected as 422 rather than 500. The router fixtures now pin EMBEDDING_DIM to match their 2-d toy vectors, which the new dimension check would otherwise reject.
Abhash-Chakraborty
left a comment
There was a problem hiding this comment.
Re-approving against the merged head — the four review threads are fixed in code, not waved through, and CI is green.
78f92cb
into
Abhash-Chakraborty:canary
CLIPEmbedder.embed_text returns an np.ndarray, and list() on that yields np.float32 elements which the JSON encoder rejects. /api/ml/embed_text therefore raised on serialization, so every search from a remote-mode instance would have returned 500. This was caught in review on #365 but landed after that PR was squashed, so it is going in as a follow-up rather than as part of it. The test now returns an ndarray like the real embedder instead of a plain list, which is what hid the bug the first time -- verified by reverting the fix and watching it fail. Also pins CLUSTERING_N_JOBS/CLUSTERING_BACKEND/USE_GPU in the router fixtures. /api/ml/cluster now goes through ImageClusterer, which reads them, and a bare MagicMock reaches HDBSCAN as an invalid n_jobs. It only surfaced when clusterer imported after the fixture patch, so the test was order-dependent rather than reliably failing.
Closes #260
This PR builds on top of the previously merged PR #308 (Config Validation) and implements the core Remote ML functionality.
What's Included:
remote_client.py): Robust HTTP client with bearer token auth and automatic EXIF stripping using PIL.routers/ml.py): Secured/api/ml/endpoints forhealth,analyze,embed, andcluster.processors.py): Intercepts inference calls to use the remote client whenML_MODE=remote. Falls back cleanly if features are disabled.Privacy & Architecture Compliance:
Authorization: Bearer <token>is strictly enforced on all ML endpoints (except/health).Let me know if you need any adjustments!
Summary by CodeRabbit
New Features
Improvements
Tests