Jm/sam - #5
Conversation
- Implemented SAM2HieraLarge model in `mblt_vision/mask_generation/sam2.py` for promptable mask generation using point prompts. - Added configuration file `SAM2HieraLarge.yaml` for model artifact paths and settings. - Created unit tests in `tests/test_mask_generation.py` to validate model functionality and backend interactions. - Introduced hardware tests in `tests/test_mask_generation_hardware.py` for real NPU performance evaluation. - Added differential tests in `tests/test_mask_generation_prompt_encoding.py` to ensure fidelity with the official `facebookresearch/sam2` predictor.
- Introduced readiness checks for the SA-V dataset in readiness.py, including validation for video IDs, image and annotation directory structure, and mask consistency. - Implemented evaluation metrics for SA-V mask generation in a new eval_sav.py module, including mean IoU calculations and candidate selection logic. - Updated dataset organization functions to handle SA-V dataset structure. - Enhanced CLI validation to support SA-V evaluation parameters. - Added comprehensive tests for SA-V dataset readiness, organization, and evaluation metrics.
- Updated SAM2HieraLarge.yaml to include ONNX artifact paths and clarify usage. - Refactored readiness.py to consolidate mask and image path handling for better readability. - Modified eval_sav.py to introduce Protocols for type safety and improved model interface. - Adjusted postprocess modules (yolo_anchorless_post.py, yolo_dflfree_post.py) to accommodate wider return types for final outputs. - Enhanced letterbox.py to ensure proper border value handling for multi-channel images. - Updated results.py to correctly cast output types from mask generation. - Expanded CLI tests to validate ONNX path arguments for model predictions. - Improved eval_sav tests to utilize a structured mock for predictions, enhancing type visibility. - Added comprehensive tests for ONNX framework integration, ensuring proper input handling and output validation. - Introduced new tests for SAM2HieraLarge ONNX predictions, verifying segmentation accuracy on synthetic images.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8c0da654d
ℹ️ 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".
Fixes all nine review comments from the automated review of b8c0da6. Correctness: - Reject --model-path/--mxq-path/--onnx-path in every CLI command that builds a mask generation engine, not just predict. Validation previously ignored them and silently evaluated the downloaded default, invalidating results. - Build host prompt tensors on the weights' device, and construct the padding point/label on the incoming tensors' device, so device="cuda" and .cuda() work instead of raising a device mismatch on every predict. - Pin the SA-V annotated-mask inventory (31967, measured from the organized sha256-pinned sav_val.tar). Video and masklet counts alone accepted a source truncated to a few annotated frames per masklet. - Require every non-zero SA-V mask value to be a single object ID, in both the organizer's staged check and the readiness check. Counting unique values accepted a {1, 2} mask that "> 0" binarization turns entirely foreground. - Validate point labels as exactly 1/0 and one-dimensional; other values get neither learned embedding and yield a meaningless mask instead of an error. Robustness: - Dispose a backend that fails after create() inside its builder. The caller assigns it only on success, so the constructor's cleanup could not reach it. - Load onnxruntime before downloading any artifact, so a missing optional dependency reports the package extra rather than a network failure. Packaging and docs: - Bump __version__ to 0.0.3 for the new public mask_generation surface. - Document mask generation in the root README (task lists, point-prompt and two-artifact CLI usage) and correct the claim that every model uses the separate preprocess/raw-call/postprocess steps. - Sync AGENTS.md and the mblt-vision skill with the new durable contracts. Verified: prompt-encoder numerics still bit-for-bit against the real facebookresearch/sam2 predictor; CUDA predict and both NPU/ONNX integration suites pass on hardware; readiness accepts the real organized SA-V dataset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All nine findings from the previous review are addressed in
Notes for the next pass
@codex review |
There was a problem hiding this comment.
💡 Codex Review
mblt-vision-python/assets/airport.jpg
Line 1 in aea76a1
Remove the unreferenced image assets
A repo-wide rg for all six added filenames finds no package, test, or documentation consumer, yet these binaries add about 4.3 MB permanently to the repository. They appear to be local inference/test inputs rather than required package data, which the repository guide explicitly says not to commit; remove them from this change.
AGENTS.md reference: AGENTS.md:L189-L190
ℹ️ 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".
Validation hardening: - Inspect point labels before the int64 cast. Casting first truncated 0.5, 1.9 and -0.1 into valid 0/1 labels, defeating the exact-label contract. - Reject non-finite point coordinates before Fourier prompt encoding, which would otherwise contaminate tokens, masks, and IoU scores. Also rejected in the CLI's parse_point, since float() accepts "nan"/"inf" and the reported repro was `--point nan,240,1`. - Enforce the three-candidate decoder contract in classify_decoder_outputs so a stale or differently exported decoder fails there rather than returning an incompatible Results.masks shape to ordinary callers. - Replace eval_sav's `assert result.selected is not None` with an explicit ValueError: the protocol permits None, the evaluator documents ValueError for prediction-contract violations, and an assert vanishes under `python -O`. - Apply the same reasoning to readiness._sav_ready's `assert first_pair`, which had identical `-O` exposure in a function documented to answer with False. Packaging: - Add MANIFEST.in pruning `assets`. Sample images are development-only inputs kept in git deliberately; setuptools already excluded them, so this pins the intent rather than fixing a leak. Verified: wheel and sdist contain zero images, package data (315 model YAMLs, dataset YAML/txt, py.typed) intact, and the built wheel imports and discovers mask_generation in isolation. - Record that deliberate exception in AGENTS.md and the skill, which previously said not to commit test assets at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Second round addressed in
On the image assetsKeeping them is a maintainer decision, not an oversight. Your point about shipped weight is fully addressed and verified:
Your repository-weight point stands on its own and is not refuted by that check — the 4.3 MB is accepted deliberately. The finding was also correct against the guide as written, since Notes for the next pass
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e565233647
ℹ️ 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".
Security:
- Reject SA-V video ids that fail SAV_VIDEO_ID_PATTERN, and confirm every
staged path resolves inside the staging tree, before any makedirs/copy.
Ids come from sav_val.txt contents rather than a directory listing, so an
entry like `../../escape` could copy attacker-controlled PNGs into the
output parent before organization failed. COCO already applied the
equivalent guard to its JSON-declared file names; swept the other
organizers and SA-V was the only one building paths from file contents
without it.
Correctness:
- Validate every cached SA-V mask, not just the first pair per video. A cache
with a valid first mask and a later {1, 2} mask previously passed readiness
and was then turned entirely to foreground by CustomSAV's `> 0`. Geometry
and values are now checked per mask; the official split is 1-bit bilevel, a
format that cannot encode a non-zero background, so those masks validate
from the header and only non-bilevel masks are decoded. Real-dataset
dataset_ready stays at 3.2s for all 31967 masks (decoding every mask would
be ~380s on every val run).
- Reject non-finite decoder outputs in classify_decoder_outputs, so a NaN from
a numerical/runtime failure cannot reach argmax over the IoU scores or the
`> 0` mask threshold and silently corrupt predictions and SA-V metrics.
- Never return the organized cache from _find_existing_source. Fixed
generically rather than by renaming one candidate: both `sa-v` and
`nyu-depth` name their cache directory identically to a source candidate, so
`data_path.parent / name` resolved back to `data_path` and organizing from
the incomplete cache failed instead of downloading the default archive.
Docs: sync AGENTS.md and the mblt-vision skill with the new contracts.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Third round addressed in
Two findings generalized rather than patched at the reported line
On the every-mask validation costThis runs on every The official masks are 1-bit bilevel ( Result: Notes for the next pass
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33318481bd
ℹ️ 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".
| resolved_prompt_weights_path = prompt_weights_path or download_hub_artifact( | ||
| repo_id=_REPO_ID, | ||
| filename=_PROMPT_WEIGHTS_FILENAME, | ||
| revision=resolved_revision, | ||
| ) |
There was a problem hiding this comment.
When a caller supplies a nonexistent encoder_mxq_path/decoder_mxq_path (or ONNX equivalent) without prompt_weights_path, this download runs before either explicit artifact is checked. In an offline environment the constructor therefore reports a Hub/network failure instead of the invalid local path; online it performs an unnecessary download before failing later in backend creation. Validate every explicitly supplied artifact path before resolving downloads, matching MBLT_Engine's fail-fast FileNotFoundError behavior.
AGENTS.md reference: AGENTS.md:L41-L43
Useful? React with 👍 / 👎.
| if len(got) != len(want) or any( | ||
| expected_dim != -1 and actual_dim != expected_dim | ||
| for actual_dim, expected_dim in zip(got, want) |
There was a problem hiding this comment.
When a decoder ONNX graph was exported with a fixed token dimension such as (1, 8, 256), this predicate skips the comparison because the expected dimension is -1, so the graph incorrectly passes construction validation. One-point inference then works, but the advertised two- and three-point prompts produce 9- or 10-token feeds and fail inside ONNX Runtime; require the session metadata itself to be dynamic on this axis rather than treating -1 as an unrestricted construction-time wildcard.
AGENTS.md reference: AGENTS.md:L96-L98
Useful? React with 👍 / 👎.
| except Exception: | ||
| self.close() | ||
| raise |
There was a problem hiding this comment.
When decoder construction, graph validation, or prompt-weight loading raises after a backend has been assigned, this handler calls the public non-suppressing close(). If either runtime also raises from dispose(), that cleanup exception replaces the original construction failure presented to the caller; the helper-level failure handlers have the same behavior. Suppress disposal failures while unwinding construction, as the base engine does, so the actionable original error remains primary.
AGENTS.md reference: AGENTS.md:L44-L50
Useful? React with 👍 / 👎.
| "Candidate masks and ground truth shapes must match: " | ||
| f"candidates {candidates.shape[1:]}, ground truth {gt.shape}." | ||
| ) | ||
| return [mask_iou(candidate.astype(bool), gt) for candidate in candidates] |
There was a problem hiding this comment.
When a structural PointPromptedEngine returns mask logits, probabilities, or non-finite values rather than already-binarized masks, this cast treats every nonzero value—including negative logits and NaNs—as foreground. eval_sav then reports plausible but corrupted IoUs instead of identifying the prediction-contract violation; validate that each candidate is a finite binary map (while allowing conventional binary encodings) before converting it to bool.
AGENTS.md reference: AGENTS.md:L166-L167
Useful? React with 👍 / 👎.
1 participant
No description provided.