SAM3: "person:1" is not equivalent to "person" — the ":N" suffix leaks into the encoded prompt
- Dominant language
- Python
- Stars
- 133k
- Forks
- 15.7k
- Avg merge
- 1d 7h
- Merged PRs (30d)
- 158
Description
### Custom Node Testing
- [ ] I have tried disabling custom nodes and the issue persists
*(Not applicable: repro #1 below imports `comfy.text_encoders.sam3_clip` directly and never starts the node system, so no custom node can be involved. Repro #2 uses only comfy-core nodes, though it was run on an install that has custom nodes present. Happy to re-run repro #2 with `--disable-all-custom-nodes` if useful.)*
### Expected Behavior
In the SAM3 detection prompt, `:N` sets `max_detections` for a category, and omitting it defaults to `1` (`comfy/text_encoders/sam3_clip.py` `_parse_prompts()`: `result.append((part, 1))`).
So `person:1` should be **equivalent to** `person` — both meaning "detect `person`, keep at most 1".
### Actual Behavior
`person:1` is **not** equivalent to `person`. The literal string `"person:1"` is sent to the text encoder as the detection phrase, so `SAM3_Detect` grounds on `"person:1"` instead of `"person"`.
`_parse_prompts()` itself is correct — it does strip the suffix. The bug is that the single-prompt fast path in `SAM3TokenizerWrapper.tokenize_with_weights()` forwards the **raw, unparsed** `text` instead of the parsed phrase:
https://github.com/Comfy-Org/ComfyUI/blob/master/comfy/text_encoders/sam3_clip.py#L51-L54
```python
def tokenize_with_weights(self, text: str, return_word_ids=False, **kwargs):
parsed = _parse_prompts(text)
if len(parsed) <= 1 and (not parsed or parsed[0][1] == 1):
return super().tokenize_with_weights(text, return_word_ids, **kwargs)
# ^^^^ raw text, still contains ":1"
```
The branch is only taken when there is exactly one prompt **and** its `max_detections == 1`, i.e. precisely the `foo:1` case (and the bare `foo` case, which is unaffected because there is nothing to strip).
Impact depends on the wording — for some phrases the stray `:1` barely changes the embedding, for others it destroys the grounding. With `person` it is catastrophic.
### Steps to Reproduce
**1. Code-level repro (deterministic, no model or image needed):**
```python
from comfy.text_encoders.sam3_clip import _parse_prompts
for t in ["person", "person:1", "person:2", "person:1,person:1"]:
p = _parse_prompts(t)
fast = len(p) <= 1 and (not p or p[0][1] == 1)
print(f"{t!r:20} parsed={p} fast_path={fast} encoded={t if fast else [x[0] for x in p]}")
```
Output on v0.33.1:
```
'person' parsed=[('person', 1)] fast_path=True encoded='person'
'person:1' parsed=[('person', 1)] fast_path=True encoded='person:1' <-- ":1" leaks into the prompt
'person:2' parsed=[('person', 2)] fast_path=False encoded=['person']
'person:1,person:1' parsed=[('person', 1), ('person', 1)] fast_path=False encoded=['person', 'person']
```
`_parse_prompts` produces `('person', 1)` in every case, yet the fast path re-encodes the original string.
**2. End-to-end repro:**
`CheckpointLoaderSimple` (`sam3.1_multiplex_fp16.safetensors`) → `CLIPTextEncode` → `SAM3_Detect` (`threshold=0.5`, `refine_iterations=1`, `individual_masks=True`), on a 1376×768 illustration containing a single person. Measuring the fraction of the frame covered by the returned mask:
| prompt | code path | mask coverage | result |
|---|---|---|---|
| `person` | fast path | **31.6 %** | correct — the person |
| `person:1` | fast path (`":1"` leaks) | **0.6 %** | wrong — a small unrelated object |
| `person:2` | multi-prompt path | **31.6 %** | correct |
| `person:1,person:1` | multi-prompt path | **31.6 %** ×2 | correct |
`person:1` and `person:1,person:1` request the same thing and differ only by which code path they take.
A second single-person image of the same size reproduced it: `person` 22.5 %, `person:1` 0.3 %, `person:2` 22.5 %.
For contrast, the same test with `girl` gave 31.53 % and `girl:1` gave 31.58 % — still a different embedding, but harmless for that wording. This is why the bug can go unnoticed.
### Suggested fix
Pass the parsed phrase instead of the raw text:
```python
if len(parsed) <= 1 and (not parsed or parsed[0][1] == 1):
return super().tokenize_with_weights(parsed[0][0] if parsed else text, return_word_ids, **kwargs)
```
### Debug Logs
```
ComfyUI 0.33.1 (git 72865f4f, tag v0.33.1)
Windows 11 Pro 26200 / RTX 4090 (driver 610.47)
Python 3.12.11 / torch 2.13.0+cu130
model: sam3/sam3.1_multiplex_fp16.safetensors
```
`comfy/text_encoders/sam3_clip.py` on this install is byte-identical to `master` (ignoring line endings), so the issue is present on `master` as well.
### Other
Related but distinct: #14087 (missing `max_detections` widget).
Contributor guide
Research direction
Start in comfy/text_encoders/sam3_clip.py by reading _parse_prompts() and SAM3TokenizerWrapper.tokenize_with_weights(). Run the deterministic _parse_prompts() reproduction from the issue, then verify that the single-prompt path encodes the parsed phrase for person:1 while preserving the existing person:2 behavior.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- machine-learning
- Issue type
- Bug
- Difficulty
- 1/5
- Estimated time
- Under an hour
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 91/100