Bound SAM3 concept-path postprocessing memory with chunked, cap-first pipeline#2670
Bound SAM3 concept-path postprocessing memory with chunked, cap-first pipeline#2670bigbitbus wants to merge 3 commits into
Conversation
… pipeline
The pinned sam3 package's PostProcessImage interpolates every
above-threshold mask to original resolution as one float32 batch and
applies the max_dets_per_img topk only afterwards; on GPU OOM its
try/except fallbacks materialize the whole k x H x W batch in host RAM
(+14 GiB transient measured at 100 instances on a 4032x4032 image).
Add ChunkedPostProcessImage, which folds the detection cap into the
threshold keep-mask before any mask work and runs interpolation + RLE
encoding in fixed-size chunks (SAM3_MASK_PROCESSING_CHUNK_SIZE, default
8), and expose max_detections / mask_format ("dense" | "rle") on
segment_with_text_prompts.
Measured on an L4 (k=100 instances @ 4032x4032): host RSS transient
+14.1 GiB -> 0, CUDA peak 17.9 -> 6.7 GiB, wall 5.3 -> 0.5 s with
cap+RLE; identical mask counts across the full test matrix. Defaults
(max_detections=-1, dense) preserve the existing response contract.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
👋 Thanks for the pull request! Here is how automated Claude review works here, so you spend credits (and reviewer time) wisely. 🚦 This PR is marked Ready for review, so automated Claude review will run — and every pass spends real credits. Warning 💸 The Claude reviewer bills in credits, not vibesAutomated review spins up a real agent that reads real code and spends real credits on every pass. It is glad to help — but it is not a rubber duck, a linter you poke in a loop, or a substitute for reading the contributing guide. Treat it like an expensive senior reviewer whose time you booked, and show up prepared. Draft when unsure, Ready when you mean it:
However you get there, arrive prepared:
Reviews are not free. A draft costs nothing to review; a Ready PR is a promise that it is worth reviewing.
|
Address cross-review findings on #2670: the early max_dets fold was gated on detection_threshold > 0, so requests with thresholding disabled skipped the cap until the parent's late topk — interpolating and materializing every query mask first. keep is now a list of index tensors built whenever a threshold OR cap is active; capped survivors come back score-descending, matching the parent's late-topk ordering exactly. New test_chunked_postprocessing.py (skipped where sam3 is stubbed) pins parity against the stock PostProcessImage: dense masks across chunk sizes 1/3/64, RLE counts, cap-as-topk ordering, cap with thresholding disabled, zero-detection shapes, and bitwise default-path parity. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
🤖 Claude review started at commit New commits are not auto-reviewed. Add the |
| from sam3.model.data_misc import interpolate | ||
| from sam3.train.masks_ops import robust_rle_encode | ||
|
|
||
| SAM3_MASK_PROCESSING_CHUNK_SIZE = int(os.getenv("SAM3_MASK_PROCESSING_CHUNK_SIZE", "8")) |
There was a problem hiding this comment.
FLAG — new env var is unvalidated, non-standard-named, and undocumented.
SAM3_MASK_PROCESSING_CHUNK_SIZE is read at module import with a bare int(os.getenv(...)). Three issues:
- Import-time crash on bad input: a non-integer value (e.g.
SAM3_MASK_PROCESSING_CHUNK_SIZE=big) raisesValueErrorwhile importingchunked_postprocessing, which is imported bysam3_torch.py— so the SAM3 model fails to load with an unhelpful traceback rather than a validatedInvalidEnvVariable. A value like0or a negative number silently passes here and is only clamped later bymax(1, ...). - Naming: the
inference_modelsconvention for new env flags isINFERENCE_MODELS_*(seeconfiguration.py);SAM3_MASK_PROCESSING_CHUNK_SIZEdoes not follow it. - Docs: not added to
inference_models/docs/how-to/environment-variables.mdand no changelog entry.
Please validate via configuration.py (raising InvalidEnvVariable), rename to the INFERENCE_MODELS_* scheme, and document it.
There was a problem hiding this comment.
Fixed in 4d05b9f. Moved to configuration.py as INFERENCE_MODELS_SAM3_MASK_PROCESSING_CHUNK_SIZE via get_integer_from_env — non-integer values now raise InvalidEnvVariable (verified: bogus and 0 both produce the proper error with the runtime-environment docs anchor, 0/negatives are rejected with an explicit >= 1 message rather than silently clamped). Documented in docs/how-to/environment-variables.md (Advanced Configuration → SAM3) and added a changelog entry under Unreleased.
| if mask_format not in ("dense", "rle"): | ||
| raise ModelInputError( | ||
| message=f"Unsupported mask_format: {mask_format}. Use 'dense' or 'rle'.", | ||
| help_url="https://todo", |
There was a problem hiding this comment.
NIT — help_url="https://todo" is a placeholder. Per the inference_models error convention, errors should carry a real docs anchor (https://inference-models.roboflow.com/errors/<page>/#<lowercased-classname>). Please point this at a real anchor or drop the arg.
There was a problem hiding this comment.
Fixed in 4d05b9f — now points at https://inference-models.roboflow.com/errors/input-validation/#modelinputerror (same anchor scheme the yolov8 models use for ModelInputError).
| max_detections: int = -1, | ||
| mask_format: str = "dense", |
There was a problem hiding this comment.
IMPORTANT — these two new params appear unwired to the production concept path.
The only caller of segment_with_text_prompts on the concept path is inference/models/sam3/segment_anything3_inference_models.py::segment_image (line ~118), which invokes it as:
self._model.segment_with_text_prompts(
images=[np_image],
prompts=prompt_dicts,
output_prob_thresh=float(min_threshold),
)It never forwards max_detections or mask_format, and segment_image itself doesn't accept them from the Sam3SegmentationRequest. So on /sam3/concept_segment these always fall back to max_detections=-1 (uncapped) and mask_format="dense".
Net effect: of the three fixes in the PR, only the chunked interpolation on the dense path is reachable in production. The cap-first truncation and the RLE fast path — which the headline measurements attribute the +14 GiB→0 RSS win to (RLE and cap + RLE rows) — are never exercised by the running endpoint.
Please confirm whether wiring these through the request/adapter is intended for this PR or a follow-up, and if follow-up, note that the RLE/cap measurements don't reflect the shipped behavior.
There was a problem hiding this comment.
Good catch — addressed in 4d05b9f, split by lever:
max_detections is now wired. New SAM3_MAX_DETECTIONS env in inference/core/env.py (default -1 = uncapped, so shipped behavior is unchanged until an operator opts in), forwarded by segment_image in segment_anything3_inference_models.py. The cap-first path is reachable in production via deploy config.
mask_format="rle" stays a model-API opt-in for this PR, deliberately. The adapter's cross-prompt NMS (_apply_nms_cross_prompt) and its polygon/RLE response conversion both consume dense arrays today; wiring RLE end-to-end means reworking that NMS to operate on run lists (pycocotools.mask.iou) — a self-contained follow-up already noted in the description.
On the measurements: the RLE / cap + RLE rows exercise the model API directly, as labeled. What ships by default is the chunked dense path — which is the row that removes the CUDA-peak scaling (17.9 → 6.4 GiB) — and the +14 GiB CPU-fallback transient those RLE rows eliminate is only reachable through the same model API that now avoids it. Description updated to state this explicitly.
|
@bigbitbus — thanks for the detailed writeup and the memory profiling. ⏳ This PR is ON HOLD pending your answers. The review will not advance to sign-off and the PR should not proceed to merge until the IMPORTANT items below are resolved. Unanswered items may keep this out of a release. Blocking finding1. Missing version bump + changelog companions (BLOCK). This is a functional change to IMPORTANT questions
Other (non-blocking, see inline)
Re-reviewNew commits are not auto-reviewed. After you answer the questions and/or push changes, add the Commands used: Reviewed at HEAD: 27982b8 |
…apter - Chunk size moves to configuration.py as INFERENCE_MODELS_SAM3_MASK_PROCESSING_CHUNK_SIZE via get_integer_from_env (InvalidEnvVariable on non-integer or < 1 instead of an import-time ValueError), documented in environment-variables.md and the changelog. - mask_format validation error points at the real ModelInputError docs anchor instead of the placeholder. - The concept adapter now forwards max_detections from a new SAM3_MAX_DETECTIONS env (default -1, behavior unchanged), making the cap-first path reachable in production; RLE consumption by the adapter remains a follow-up since its cross-prompt NMS operates on dense masks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Problem
The pinned
sam3==0.1.4package'sPostProcessImagehas three serving-hostile behaviors on the concept path (segment_with_text_prompts→/sam3/concept_segment):max_dets_per_img=-1; every above-threshold instance survives.k×1×H×Wfloat32 tensor (~6.5 GiB for 100 masks at 4032²). On GPU OOM, silenttry/exceptfallbacks (interpolate,robust_rle_encode) materialize that batch in host RAM.max_dets_per_imgtopk happens inprocess_resultsafter all masks are interpolated and encoded, so it bounds only the response, never peak memory. Measured: cap+RLE enabled together still spiked +14.1 GiB host RSS at 100 instances on a 4032² image.Direct contributor to the host-RAM profile behind the 2026-06-22 async-serverless OOM crash loop; upstream
facebookresearch/sam3has no fix or open issue for any of this.Fix
Blast radius: concept path only — the visual/interactive path and RF Instant are untouched, and defaults are bitwise-identical to main (verified below).
ChunkedPostProcessImage(newinference_models/models/sam3/chunked_postprocessing.py) subclasses the pinned package's postprocessor — subclassing keeps thesam3pin untouched and ships the fix on our release cadence. Two methods differ from the parent:keepbecomes a list of index tensors built whenever a threshold or cap is active — including with thresholding disabled (output_prob_thresh ≤ 0). Capped survivors are score-descending, matching the parent's late-topk ordering exactly; the parent's late topk remains as a no-op safety net._process_masksinterpolates + encodes in fixed slices (INFERENCE_MODELS_SAM3_MASK_PROCESSING_CHUNK_SIZE, validated, default 8), so neither device ever holds the full k×H×W float32 batch, and the GPU→CPU fallback blast radius is one chunk (~0.5 GiB at 4032²).Reviewer note:
_process_boxes_and_labelsintentionally mirrors ~30 lines of the parent with one added block — review it side-by-side withsam3/eval/postprocessors.py, then thesam3_torch.pywiring; the parity tests pin equivalence. When thesam3pin is bumped, re-diff that method against upstream.Wiring:
segment_with_text_promptsgainsmax_detections: int = -1andmask_format: "dense" | "rle"(RLEsizestays original image dims, so app consumers are unaffected).max_detectionsis production-reachable via the newSAM3_MAX_DETECTIONSenv (default-1— no behavior change until an operator opts in).mask_format="rle"is a model-API opt-in for now: the concept adapter's cross-prompt NMS consumes dense masks, and reworking it onto run lists (pycocotools.mask.iou) is a self-contained follow-up. The RLE measurement rows below exercise the model API, not shipped adapter behavior; the shipped default path gets the chunking wins.Measurements (L4, real
sam3/sam3_finalweights)Synthetic scale matrix (k tiled objects, cap 25; "before" = the stock postprocessor with the same knobs forced — main's API hardcoded
-1/dense):Mask counts identical in every cell; CUDA peak collapses to the model-forward floor in all configs — chunking alone recovers ~11.5 GiB on the default path.
Real photos (people-walking / bleachers / coin-counting at 4032-wide):
Real-data parity vs
origin/main(5 photos incl. multi-prompt): every mask IoU = 1.0000, score Δ = 0.00e+00, identical counts; branch RLE decodes pixel-identical to baseline dense masks. Re-verified after each review-fix commit.Tests
78 passed, 0 failed on a GPU box: 50
inference_models— including newtest_chunked_postprocessing.py(8 parity tests vs the stock parent: chunk sizes 1/3/>k, RLE counts, cap-as-topk ordering, cap with thresholding disabled, zero-detection shapes, bitwise default parity) and the real-weights integration suite — plus 28 inference-side. Env validation verified: non-integer or< 1chunk-size values raiseInvalidEnvVariable.Review
Cross-model review findings addressed in
27982b85(cap without threshold, ordering parity, parity tests) and4d05b9f4(validated+documented env var, realhelp_url,SAM3_MAX_DETECTIONSadapter wiring). Remaining follow-up: adapter-level RLE consumption.Notes
inference-modelsrelease + pin bump.facebookresearch/sam3(cap-before-interpolate + chunked encode) is worth filing; none exists today.🤖 Generated with Claude Code