Skip to content

Commit 25a093b

Browse files
update sam3 CLI flags and default values with new CLI unit tests (#167)
1 parent e397e29 commit 25a093b

4 files changed

Lines changed: 219 additions & 55 deletions

File tree

models/sam3/README.md

Lines changed: 13 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -39,18 +39,19 @@ uv run export.py --help
3939

4040
**Options:**
4141

42-
| Flag | Description | Default |
43-
|------------------------|------------------------------------------------|------------------------|
44-
| `--full` | Export plain HF `Sam3Model` (no iOS targeting) ||
45-
| `--output-dir` | Output directory for the bundle | `<repo-root>/exports/` |
46-
| `--output-name` | Custom bundle directory name | derived |
47-
| `--image-size` | Input resolution (336 lite / 1008 full) | `336` / `1008` |
48-
| `--max-text-seq-len` | (lite) Static text sequence length | `32` |
49-
| `--n-bits` | (lite) Uniform palettization bit-width override applied to BOTH encoders | asymmetric: image w4, text w6 |
50-
| `--group-size` | (lite) Uniform palettization group-size override applied to BOTH encoders | asymmetric: image gs32, text gs8 |
51-
| `--dtype` | (`--full`) Torch dtype: `float16` or `float32` | `float32` |
52-
| `--overwrite` | Overwrite existing bundle ||
53-
| `--dry-run` | Print resolved config and exit ||
42+
| Flag | Description | Default |
43+
|----------------------|---------------------------------------------------------------------------|----------------------------------|
44+
| `--model` | Model to export: `sam3` (shortname) or `facebook/sam3` | `facebook/sam3` |
45+
| `--full` | Export plain HF `Sam3Model` (no iOS targeting) ||
46+
| `--output-dir` | Output directory for the bundle | `<repo-root>/exports/` |
47+
| `--output-name` | Custom bundle directory name | derived |
48+
| `--image-size` | Input resolution (336 lite / 1008 full) | `336` / `1008` |
49+
| `--max-text-seq-len` | (lite) Static text sequence length | `32` |
50+
| `--n-bits` | (lite) Uniform palettization bit-width override applied to BOTH encoders | asymmetric: image w4, text w6 |
51+
| `--group-size` | (lite) Uniform palettization group-size override applied to BOTH encoders | asymmetric: image gs32, text gs8 |
52+
| `--dtype` | (`--full`) Torch dtype: `float16` or `float32` | `float32` |
53+
| `--overwrite` | Overwrite existing bundle ||
54+
| `--dry-run` | Print resolved config and exit ||
5455

5556
`image-size=336` is the resolution we recommend for iOS deployment.
5657

models/sam3/export.py

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -44,18 +44,12 @@
4444
uv run models/sam3/export.py [--image-size N] [--n-bits N] [--output-dir PATH] ...
4545
"""
4646

47-
import sys
48-
4947

5048
def main() -> None:
5149
# Lazy import so the inline-script header parses cleanly even if the
5250
# workspace package isn't on sys.path yet (uv handles that before main()).
5351
from coreai_models.segmentation.export import main as segmentation_main
5452

55-
# The shared CLI takes a `model` positional. This script only handles SAM3,
56-
# so inject "sam3" and forward any user-supplied flags untouched.
57-
if len(sys.argv) == 1 or sys.argv[1].startswith("-"):
58-
sys.argv.insert(1, "sam3")
5953
segmentation_main()
6054

6155

python/src/coreai_models/segmentation/export.py

Lines changed: 41 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -25,25 +25,20 @@
2525
import logging
2626
from pathlib import Path
2727

28-
from coreai_models.model_registry import lookup_utility_model
2928
from coreai_models.segmentation.pipeline import (
3029
FullExportConfig,
3130
SegmentationExportConfig,
3231
export_full,
3332
export_segmentation,
3433
)
3534

35+
# Accepted ``--model`` spellings → canonical HF id. Doubles as the source of
36+
# truth for the flag's ``choices``, so adding an alias here is all it takes.
3637
_SUPPORTED = {
3738
"sam3": "facebook/sam3",
3839
"facebook/sam3": "facebook/sam3",
3940
}
4041

41-
# Defaults that differ between the two export paths. Used only when
42-
# ``--image-size`` isn't passed explicitly so each mode picks the
43-
# resolution it was designed for.
44-
_LITE_DEFAULT_IMAGE_SIZE = 336
45-
_FULL_DEFAULT_IMAGE_SIZE = 1008
46-
4742

4843
def _find_repo_root() -> Path | None:
4944
d = Path(__file__).resolve().parent
@@ -60,16 +55,12 @@ def _default_output_dir() -> str:
6055

6156

6257
def _resolve_hf_model_id(model: str) -> str:
63-
"""Accept registry short-name or HF id; reject anything else."""
64-
if model in _SUPPORTED:
65-
return _SUPPORTED[model]
66-
preset = lookup_utility_model(model)
67-
if preset is not None and preset.task == "segmentation" and preset.hf_id in _SUPPORTED:
68-
return _SUPPORTED[preset.hf_id]
69-
raise SystemExit(
70-
f"Error: '{model}' is not a supported segmentation model. "
71-
f"Supported: {sorted(set(_SUPPORTED.values()))}"
72-
)
58+
"""Map an accepted ``--model`` value to its canonical HF id.
59+
60+
``--model`` derives its ``choices`` from ``_SUPPORTED``, so argparse has
61+
already rejected anything unknown by the time this runs.
62+
"""
63+
return _SUPPORTED[model]
7364

7465

7566
def build_parser() -> argparse.ArgumentParser:
@@ -84,7 +75,9 @@ def build_parser() -> argparse.ArgumentParser:
8475
),
8576
)
8677
parser.add_argument(
87-
"model",
78+
"--model",
79+
choices=sorted(_SUPPORTED),
80+
default="facebook/sam3",
8881
help=(
8982
"Segmentation model. Either the registry short-name (e.g. 'sam3') "
9083
"or its HuggingFace id (e.g. 'facebook/sam3')."
@@ -118,11 +111,14 @@ def build_parser() -> argparse.ArgumentParser:
118111
help=("Input resolution. Defaults to 336 (lite) or 1008 (--full). "),
119112
)
120113
# ---- Lite-only flags -------------------------------------------
114+
# Mode-specific flags default to None rather than their real default so
115+
# _warn_unused_flags can tell "user passed this" from "user left it alone",
116+
# even when the value passed matches the default. Resolved in main().
121117
parser.add_argument(
122118
"--max-text-seq-len",
123119
type=int,
124-
default=32,
125-
help="(lite) Static text sequence length used at export time.",
120+
default=None,
121+
help="(lite) Static text sequence length used at export time. Default: 32.",
126122
)
127123
parser.add_argument(
128124
"--n-bits",
@@ -147,8 +143,8 @@ def build_parser() -> argparse.ArgumentParser:
147143
parser.add_argument(
148144
"--dtype",
149145
choices=["float16", "float32"],
150-
default="float32",
151-
help="(--full) Torch dtype to use for the model.",
146+
default=None,
147+
help="(--full) Torch dtype to use for the model. Default: float32.",
152148
)
153149
# ---- Shared flags ---------------------------------------------------
154150
parser.add_argument(
@@ -171,32 +167,40 @@ def build_parser() -> argparse.ArgumentParser:
171167

172168

173169
def _resolve_image_size(args: argparse.Namespace) -> int:
170+
"""Pick the resolution each mode was designed for when --image-size is omitted.
171+
172+
Mode-specific flags default to None so ``_warn_unused_flags`` can detect
173+
"passed" regardless of value; the real defaults live on the config
174+
dataclasses. ``@dataclass`` leaves plain defaults as class attributes, so
175+
they're readable without constructing a config.
176+
"""
174177
if args.image_size is not None:
175178
return args.image_size
176-
return _FULL_DEFAULT_IMAGE_SIZE if args.full else _LITE_DEFAULT_IMAGE_SIZE
179+
return FullExportConfig.image_size if args.full else SegmentationExportConfig.image_size
177180

178181

179-
def _warn_unused_flags(parser: argparse.ArgumentParser, args: argparse.Namespace) -> None:
182+
def _warn_unused_flags(args: argparse.Namespace) -> None:
180183
"""Surface flags that don't apply to the chosen mode so users notice typos.
181184
182-
argparse can't natively express "this flag only applies when --full
183-
is set" without subparsers, so we just check after parsing.
185+
argparse can't natively express "this flag only applies when --full is set"
186+
without subparsers. Every mode-specific flag defaults to ``None``, so "not
187+
None" means the user passed it explicitly — including when the value they
188+
passed happens to equal the mode's resolved default.
184189
"""
185-
parser_defaults = parser.parse_args([args.model])
186190
if args.full:
187191
# Lite-only flags shouldn't be set in full mode.
188-
ignored = []
189-
for name in ("max_text_seq_len", "n_bits", "group_size"):
190-
if getattr(args, name) != getattr(parser_defaults, name):
191-
ignored.append(name.replace("_", "-"))
192+
ignored = [
193+
name.replace("_", "-")
194+
for name in ("max_text_seq_len", "n_bits", "group_size")
195+
if getattr(args, name) is not None
196+
]
192197
if ignored:
193198
logging.warning(
194199
"Ignoring lite-only flag(s) in full mode: %s",
195200
", ".join(f"--{n}" for n in ignored),
196201
)
197-
else:
198-
if args.dtype != parser_defaults.dtype:
199-
logging.warning("Ignoring --dtype outside full mode (lite path is fp16).")
202+
elif args.dtype is not None:
203+
logging.warning("Ignoring --dtype outside full mode (lite path is fp16).")
200204

201205

202206
def main() -> None:
@@ -211,13 +215,13 @@ def main() -> None:
211215

212216
hf_model_id = _resolve_hf_model_id(args.model)
213217
image_size = _resolve_image_size(args)
214-
_warn_unused_flags(parser, args)
218+
_warn_unused_flags(args)
215219

216220
if args.full:
217221
config = FullExportConfig(
218222
hf_model_id=hf_model_id,
219223
image_size=image_size,
220-
dtype=args.dtype,
224+
dtype=args.dtype or FullExportConfig.dtype,
221225
output_dir=args.output_dir or _default_output_dir(),
222226
output_name=args.output_name,
223227
overwrite=args.overwrite,
@@ -249,7 +253,7 @@ def main() -> None:
249253
config = SegmentationExportConfig(
250254
hf_model_id=hf_model_id,
251255
image_size=image_size,
252-
max_text_seq_len=args.max_text_seq_len,
256+
max_text_seq_len=args.max_text_seq_len or defaults.max_text_seq_len,
253257
image_n_bits=image_n_bits,
254258
image_group_size=image_group_size,
255259
text_n_bits=text_n_bits,
Lines changed: 165 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,165 @@
1+
# Copyright 2026 Apple Inc.
2+
#
3+
# Use of this source code is governed by a BSD-3-clause license that can
4+
# be found in the LICENSE file or at https://opensource.org/licenses/BSD-3-Clause
5+
6+
"""Tests for the ``coreai.segmentation.export`` CLI.
7+
8+
Flag plumbing only — nothing here downloads weights or runs an export, so
9+
these are safe on any machine.
10+
"""
11+
12+
from __future__ import annotations
13+
14+
import argparse
15+
import logging
16+
17+
import pytest
18+
19+
from coreai_models.segmentation.export import (
20+
_SUPPORTED,
21+
_resolve_hf_model_id,
22+
_resolve_image_size,
23+
_warn_unused_flags,
24+
build_parser,
25+
)
26+
from coreai_models.segmentation.pipeline import FullExportConfig, SegmentationExportConfig
27+
28+
29+
def _parse(*argv: str) -> tuple[argparse.ArgumentParser, argparse.Namespace]:
30+
"""Build a fresh parser and parse ``argv``, returning both."""
31+
parser = build_parser()
32+
return parser, parser.parse_args(list(argv))
33+
34+
35+
# --- --model ---------------------------------------------------------
36+
37+
38+
def test_every_supported_spelling_parses_and_resolves() -> None:
39+
"""``--model`` takes its ``choices`` from ``_SUPPORTED``, which is what lets
40+
``_resolve_hf_model_id`` be a bare dict lookup. Pin that coupling: every
41+
key must parse, and every value must be a canonical HF id."""
42+
for spelling in _SUPPORTED:
43+
_, args = _parse("--model", spelling)
44+
assert _resolve_hf_model_id(args.model) == _SUPPORTED[spelling]
45+
46+
47+
def test_model_defaults_to_sam3() -> None:
48+
_, args = _parse()
49+
assert _resolve_hf_model_id(args.model) == "facebook/sam3"
50+
51+
52+
def test_model_accepts_registry_short_name() -> None:
53+
_, args = _parse("--model", "sam3")
54+
assert _resolve_hf_model_id(args.model) == "facebook/sam3"
55+
56+
57+
def test_model_accepts_hf_id() -> None:
58+
_, args = _parse("--model", "facebook/sam3")
59+
assert _resolve_hf_model_id(args.model) == "facebook/sam3"
60+
61+
62+
def test_model_rejects_unknown_value() -> None:
63+
parser = build_parser()
64+
with pytest.raises(SystemExit):
65+
parser.parse_args(["--model", "facebook/sam2"])
66+
67+
68+
def test_bare_positional_is_rejected() -> None:
69+
"""``--model`` replaced a positional, so a bare value is no longer valid."""
70+
parser = build_parser()
71+
with pytest.raises(SystemExit):
72+
parser.parse_args(["sam3"])
73+
74+
75+
# --- mode + image size -----------------------------------------------
76+
77+
78+
def test_lite_is_the_default_mode() -> None:
79+
_, args = _parse()
80+
assert args.full is False
81+
82+
83+
def test_image_size_defaults_per_mode() -> None:
84+
_, lite = _parse()
85+
_, full = _parse("--full")
86+
assert _resolve_image_size(lite) == SegmentationExportConfig.image_size
87+
assert _resolve_image_size(full) == FullExportConfig.image_size
88+
89+
90+
def test_explicit_image_size_overrides_mode_default() -> None:
91+
_, args = _parse("--full", "--image-size", "512")
92+
assert _resolve_image_size(args) == 512
93+
94+
95+
# --- _warn_unused_flags ----------------------------------------------
96+
97+
98+
def test_warn_unused_flags_does_not_reparse_argv() -> None:
99+
"""Regression: this recovered defaults via ``parse_args([args.model])``,
100+
which argparse rejected as a stray positional once ``model`` became
101+
``--model`` — killing the process before the export started."""
102+
_, args = _parse("--full", "--dtype", "float16")
103+
_warn_unused_flags(args)
104+
105+
106+
def test_warns_on_dtype_equal_to_default_in_lite_mode(
107+
caplog: pytest.LogCaptureFixture,
108+
) -> None:
109+
"""Regression: comparing against the default missed ``--dtype float32``,
110+
since float32 *is* the default — the flag was silently ignored with no
111+
warning. Mode-specific flags now default to None so "passed" is detectable
112+
regardless of the value."""
113+
_, args = _parse("--dtype", "float32")
114+
with caplog.at_level(logging.WARNING):
115+
_warn_unused_flags(args)
116+
assert "--dtype" in caplog.text
117+
118+
119+
def test_warns_on_lite_only_flag_equal_to_default_in_full_mode(
120+
caplog: pytest.LogCaptureFixture,
121+
) -> None:
122+
"""Same class of bug as the --dtype case: 32 is the resolved default."""
123+
_, args = _parse("--full", "--max-text-seq-len", "32")
124+
with caplog.at_level(logging.WARNING):
125+
_warn_unused_flags(args)
126+
assert "--max-text-seq-len" in caplog.text
127+
128+
129+
def test_warns_on_lite_only_flags_in_full_mode(caplog: pytest.LogCaptureFixture) -> None:
130+
_, args = _parse("--full", "--n-bits", "4", "--max-text-seq-len", "64")
131+
with caplog.at_level(logging.WARNING):
132+
_warn_unused_flags(args)
133+
assert "--n-bits" in caplog.text
134+
assert "--max-text-seq-len" in caplog.text
135+
136+
137+
def test_warns_on_dtype_in_lite_mode(caplog: pytest.LogCaptureFixture) -> None:
138+
_, args = _parse("--dtype", "float16")
139+
with caplog.at_level(logging.WARNING):
140+
_warn_unused_flags(args)
141+
assert "--dtype" in caplog.text
142+
143+
144+
def test_no_warning_for_lite_flags_in_lite_mode(caplog: pytest.LogCaptureFixture) -> None:
145+
_, args = _parse("--n-bits", "4", "--group-size", "16")
146+
with caplog.at_level(logging.WARNING):
147+
_warn_unused_flags(args)
148+
assert caplog.text == ""
149+
150+
151+
def test_no_warning_for_dtype_in_full_mode(caplog: pytest.LogCaptureFixture) -> None:
152+
_, args = _parse("--full", "--dtype", "float16")
153+
with caplog.at_level(logging.WARNING):
154+
_warn_unused_flags(args)
155+
assert caplog.text == ""
156+
157+
158+
def test_no_warning_when_nothing_mode_specific_is_passed(
159+
caplog: pytest.LogCaptureFixture,
160+
) -> None:
161+
for argv in ((), ("--full",)):
162+
_, args = _parse(*argv)
163+
with caplog.at_level(logging.WARNING):
164+
_warn_unused_flags(args)
165+
assert caplog.text == "", f"unexpected warning for {argv}"

0 commit comments

Comments
 (0)