Add pet clustering with feedback, naming, and search#9665
Add pet clustering with feedback, naming, and search#9665Amrithesh-Kakkoth wants to merge 107 commits intoente-io:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 536ec02e71
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
3-phase Rust clustering pipeline (face cluster, body rescue, cross-merge) using average-linkage agglomerative clustering, validated against Python reference with 100% agreement (NMI=1.0). Includes Dart service layer, DB migrations, API bindings, and test binary for comparison.
Call PetClusteringService after face clustering in runAllML so pet clusters are formed automatically after indexing completes.
- LazySession was forcing CPU-only for ALL models; now only pet models use CPU-only policy (to avoid NNAPI/CoreML issues with FP16 models) - Restore preferNnapi and preferXnnpack to Platform.isAndroid - Restore GraphOptimizationLevel::Level3 (was downgraded to Level1) - Restore arena allocator on CPUExecutionProvider
Sessions stay loaded across images and get cleaned up when the isolate disposes via release_runtime(). Avoids expensive repeated ONNX session load/unload cycles on every image.
This was debug-only code that altered production indexing logic to re-process all files for pet-only mode. Removes the flag, the forceAll parameter threading through ml_util, and the ClipText skip hack.
- Inline embedding columns into CREATE TABLE (remove ALTER TABLE migrations) - Store landmarks inside detection JSON (remove separate column) - Remove migration tracking from PetVectorDB (tables are new) - Rename pet_bodies table to detected_objects with generic column names - Change species column from TEXT to INTEGER (store raw COCO class) - Revert shouldAutomaticDispose back to true - Add separate petMlVersion constant - Move embedding storage methods from MLService to MLDataDB
Add pet_indexed_files tracking table so files already indexed for faces+CLIP are re-queued when pet recognition is enabled. Also fix MLResult.ranML to include petsRan, which was causing pet-only analysis results to be silently discarded.
…sc bugs - Migrate ML runtime from Mutex to RwLock + OnceCell for concurrent inference (CLIP text no longer blocks image indexing) - Fix cat face detection: model outputs per-class scores, not a class ID; use argmax instead of truncating float to u8 - Fix species normalization: store 0/1 (dog/cat) not COCO 15/16 - Fix markPetIndexed guard to check shouldRunPets && rustPets - Add pet model paths to runtime config cache key - Batch pet body embeddings by species (matching face pattern) - Rename objectIDColumn to detectedObjectIDColumn to avoid ambiguity - Implement deletePetDataForFiles in DB layer - Fix design system usage in pet thumbnail widget
…ing column Co-Authored-By: Amrithesh <amritheshkakkoth10@gmail.com>
536ec02 to
a3b74d4
Compare
… missing schema tables
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2def4bb422
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
mobile/apps/photos/lib/services/machine_learning/pet_ml/pet_clustering_service.dart
Show resolved
Hide resolved
Extract shared clustering algorithm from pet module into reusable ml::cluster module. Add face-specific batch and incremental clustering functions with rejection support. Wire as fallback in ml_service when Dart linear fails and useRustForML is enabled. Also remove dead petIndexedFilesTable and redundant markPetIndexed calls.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f9c592313
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…esh, species-filtered moves
|
@codex review |
- Move cross-model human face suppression into pet detection loop for single-pass, zero-allocation filtering - Raise pet face detection threshold from 0.3 to 0.5 - Add toggle flag to disable cross-model filtering - Show only user-given names for pets, hide auto-generated labels like "Dog 1", "Cat 2"; show "Add name" prompt instead - Resolve merge conflict in feature flag service
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd3b0297a7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
mobile/apps/photos/lib/services/machine_learning/pet_ml/pet_service.dart
Show resolved
Hide resolved
mobile/apps/photos/lib/services/machine_learning/pet_ml/pet_service.dart
Show resolved
Hide resolved
- Swap reconcileClusters order: push local changes to remote before fetching remote feedback, preventing fresh local assignments from being deleted by the stale-cleanup pass - Clear remote assigned list for pets that lost all local clusters, preventing stale assignments from persisting on the server
|
@codex review |
Offline mode is view-only raw clustering output. Skip not-pet feedback table creation/deletion and rejected-assignment overrides in offline mode. Disable name editing, cluster view, merge, and selection bar in pet cluster UI when offline.
| _logger.warning("Failed to delete pet body vectors", e, s); | ||
| // Collect which clusters will lose members so we can recompute their | ||
| // summaries and centroids after the deletion. | ||
| final affectedClusterIds = <String>{}; |
There was a problem hiding this comment.
instead of recomputing, let's delete and recompute when actually needed? (assuming we keep centroid)
| 'pet_cluster_centroid_vector_id_map'; | ||
| const petClusterCentroidVectorIdColumn = 'pet_cluster_vector_id'; | ||
|
|
||
| const createPetClusterCentroidVectorIdMappingTable = ''' |
There was a problem hiding this comment.
Check if we're using this for clustering or if this can be removed
There was a problem hiding this comment.
It is used in clustering. It maps cluster_id to integer key
mobile/apps/photos/rust/Cargo.toml
Outdated
|
|
||
| [lib] | ||
| crate-type = ["cdylib", "staticlib"] | ||
| crate-type = ["cdylib", "staticlib", "rlib"] |
There was a problem hiding this comment.
Bump. check other old comments too
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fe3d34071
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
mobile/apps/photos/lib/services/machine_learning/pet_ml/pet_service.dart
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| Future<void> deletePet(String petID) async { | ||
| await entityService.deleteEntry(petID); |
There was a problem hiding this comment.
Remove cluster mappings when deleting a pet
Deleting a pet entity here does not clear pet_cluster_pet rows for that pet, so clusters remain locally mapped to a now-missing pet until a later sync pass cleans orphans. During that period, search/grouping still treats those clusters as merged under the deleted pet ID, which leaves inconsistent UI state and can propagate stale mapping behavior.
Useful? React with 👍 / 👎.
| eprintln!( | ||
| "[ml][pet] face detect: output_shape={:?}, row_len={}, detection_rows={}, is_2class={}", | ||
| output_shape, | ||
| row_len, | ||
| detection_rows, | ||
| row_len >= 13 | ||
| ); |
There was a problem hiding this comment.
Remove verbose stderr logging from pet detection loop
These unconditional eprintln! calls run in the inference hot path for every analyzed image (and per detection), which can flood log output and add avoidable overhead on-device. In production builds this can noticeably hurt ML throughput and make logs noisy; these statements should be gated behind debug-only logging or removed.
Useful? React with 👍 / 👎.
# Conflicts: # mobile/apps/photos/pubspec.yaml # mobile/apps/photos/pubspec_overrides.yaml
Summary