From fb4b377d76eec05fea177e8853edffaec8e76954 Mon Sep 17 00:00:00 2001 From: iback Date: Mon, 5 Oct 2026 08:28:01 +0000 Subject: [PATCH 1/4] fix: make CLI messages name flags that exist and causes users can act on Four hints pointed at flags that were renamed (or never existed): "Subreg Mask already exists. Set -override_subreg to create it anew" # --override-semantic/-os "Vert Mask already exists. Set -override_vert to create it anew" # --override-instance/-oi "Centroids already exists ... Set -override_ctd = True" # --override-ctd/-oc "-model_instance/-mv resolved to None" # --model-instance/-mi and `--input` mishandled the two most common mistakes. An uncompressed `scan.nii`, which SPINEPS cannot read at all, had `.nii.gz` appended unconditionally and then failed with "-input does not exist or is not a file, got /abs/scan.nii.nii.gz" -- a path the user never typed. It now says the image must be gzipped and how to fix it, while `--input sub-01_T2w` (no extension) keeps working because the suffix is only appended when nothing exists at the given path. A dead `if dataset == ""` branch (unreachable after `Path.absolute()`) is removed. Also stops the log-argument filter from logging its own exclusion list. Co-Authored-By: Claude Opus 5 --- spineps/entrypoint.py | 24 ++++--- spineps/seg_run.py | 8 +-- unit_tests/test_cli_messages.py | 115 ++++++++++++++++++++++++++++++++ 3 files changed, 132 insertions(+), 15 deletions(-) create mode 100644 unit_tests/test_cli_messages.py diff --git a/spineps/entrypoint.py b/spineps/entrypoint.py index 5b0fef2..e59eca6 100755 --- a/spineps/entrypoint.py +++ b/spineps/entrypoint.py @@ -4,7 +4,6 @@ import argparse import cProfile -import os from argparse import Namespace from pathlib import Path from time import perf_counter @@ -247,20 +246,23 @@ def run_sample(opt: Namespace): int: ``1`` on completion. Raises: - ValueError: If only a filename was given instead of a path to the file. + ValueError: If the input is not a gzipped NIfTI image (``.nii.gz``). FileNotFoundError: If the input path's parent directory is missing, or the input file does not exist. """ input_path = Path(opt.input).absolute() + if not input_path.exists() and not input_path.name.endswith(".nii.gz"): + input_path = input_path.with_name(input_path.name + ".nii.gz") # allow --input without the extension + if not input_path.parent.exists(): + raise FileNotFoundError(f"--input/-i: the folder {input_path.parent} does not exist") + if not input_path.is_file(): + raise FileNotFoundError(f"--input/-i: {input_path} does not exist or is not a file") + if not input_path.name.endswith(".nii.gz"): + raise ValueError( + f"--input/-i: SPINEPS reads gzipped NIfTI images (.nii.gz), got {input_path.name}. " + f"Compress it first, e.g. 'gzip {input_path.name}'." + ) dataset = str(input_path.parent) - if dataset == "": - raise ValueError(f"-input you only gave a filename, not a path to the file, got {input_path}") - if not os.path.exists(dataset): # noqa: PTH110 - raise FileNotFoundError(f"-input parent directory does not exist, got {dataset}") input_path = str(input_path) - if not input_path.endswith(".nii.gz"): - input_path += ".nii.gz" - if not os.path.isfile(input_path): # noqa: PTH113 - raise FileNotFoundError(f"-input does not exist or is not a file, got {input_path}") # model semantic if "/" in str(opt.model_semantic): model_semantic = get_actual_model(opt.model_semantic, use_cpu=opt.cpu).load() @@ -375,7 +377,7 @@ def run_dataset(opt: Namespace): model_labeling = get_labeling_model(opt.model_labeling, use_cpu=opt.cpu).load() if model_instance is None: - raise ValueError("-model_instance/-mv resolved to None; pass a valid instance model id or path") + raise ValueError("--model-instance/-mi resolved to None; pass a valid instance model id or path") kwargs = { "dataset_path": input_dir, diff --git a/spineps/seg_run.py b/spineps/seg_run.py index 7e5e55d..5ee5355 100755 --- a/spineps/seg_run.py +++ b/spineps/seg_run.py @@ -182,7 +182,7 @@ def process_dataset( # noqa: C901 # Activate logger. Log the plain options plus the model ids -- the model objects stringify to their # entire inference config, which floods the log file. - _not_logged = ("model_instance", "model_semantic", "model_labeling", "compatible") + _not_logged = ("model_instance", "model_semantic", "model_labeling", "compatible", "_not_logged") args = {k: v for k, v in locals().items() if k not in _not_logged} args["model_instance"] = model_instance.modelid() args["model_semantic"] = [m.modelid() for m in model_semantic] @@ -599,7 +599,7 @@ def segment_image( # noqa: C901 logger.print(f"Predict semantic took: {perf_counter() - start_time2:.2f} seconds", Log_Type.OK, verbose=log_inference_time) start_time2 = perf_counter() else: - logger.print("Subreg Mask already exists. Set -override_subreg to create it anew") + logger.print("Semantic mask already exists, loading it. Pass --override-semantic/-os to redo it") seg_nii_modelres = NII.load(out_spine_raw, seg=True) logger.print("seg_nii", seg_nii_modelres.zoom, seg_nii_modelres.orientation, seg_nii_modelres.shape) # Second stage @@ -630,7 +630,7 @@ def segment_image( # noqa: C901 logger.print(f"Predict instance took: {perf_counter() - start_time2:.2f} seconds", Log_Type.OK, verbose=log_inference_time) start_time2 = perf_counter() else: - logger.print("Vert Mask already exists. Set -override_vert to create it anew") + logger.print("Vertebra mask already exists, loading it. Pass --override-instance/-oi to redo it") whole_vert_nii = NII.load(out_vert_raw, seg=True) # Cleanup Step @@ -694,7 +694,7 @@ def segment_image( # noqa: C901 logger.print(f"Centroids took: {perf_counter() - start_time2:.2f} seconds", Log_Type.OK, verbose=log_inference_time) start_time2 = perf_counter() else: - logger.print("Centroids already exists, will load instead. Set -override_ctd = True to create it anew") + logger.print("Centroids already exist, loading them. Pass --override-ctd/-oc to redo them") ctd = POI.load(out_ctd) # return_output_instead_of_save: diff --git a/unit_tests/test_cli_messages.py b/unit_tests/test_cli_messages.py new file mode 100644 index 0000000..dc3456b --- /dev/null +++ b/unit_tests/test_cli_messages.py @@ -0,0 +1,115 @@ +# Call 'python -m unittest' on this folder +"""Every user-facing message must name a flag that exists and a cause the user can act on. + +The hints told users to set flags that do not exist (`-override_subreg`, `-override_vert`, +`-override_ctd`, `-model_instance`; the real ones are `--override-semantic`, `--override-instance`, +`--override-ctd`, `--model-instance`), and `--input scan.nii` -- an uncompressed NIfTI, which SPINEPS +cannot read -- silently became `scan.nii.nii.gz` and then reported *"-input does not exist or is not a +file"* about a path the user never typed. +""" + +from __future__ import annotations + +import argparse +import inspect +import re +import tempfile +import unittest +from pathlib import Path + +from spineps import entrypoint, seg_run + + +def _all_flags() -> set[str]: + """Every option string the CLI actually accepts, including the --no-* forms argparse generates.""" + parser = argparse.ArgumentParser() + entrypoint.parser_arguments(parser) + flags: set[str] = set() + for action in parser._actions: + flags.update(action.option_strings) + if isinstance(action, argparse.BooleanOptionalAction): + flags.update("--no-" + s[2:] for s in action.option_strings if s.startswith("--")) + # the per-subcommand options, which parser_arguments does not register + flags.update( + [ + *("--input", "-i", "--directory", "-d", "--rawdata-name", "-rn", "--model-semantic", "-ms"), + *("--model-instance", "-mi", "-mv", "--model-labeling", "-ml", "--ignore-bids-filter", "-ibf"), + *("--ignore-model-compatibility", "-imc", "--save-log", "-sl", "--save-snaps-folder", "-ssf"), + ] + ) + return flags + + +class Test_Messages_Name_Real_Flags(unittest.TestCase): + """Any `-flag` mentioned in a message or exception must be one the parser knows.""" + + FLAG_PATTERN = re.compile(r"(? None: + flags = _all_flags() + source = inspect.getsource(module) + # only user-facing strings, i.e. logger.print(...) / raise ...(f"...") + for line in source.splitlines(): + if "logger.print(" not in line and "raise " not in line and 'f"' not in line: + continue + for quoted in re.findall(r'"([^"]*)"', line): + for candidate in self.FLAG_PATTERN.findall(quoted): + if "_" in candidate or candidate.startswith("--"): + with self.subTest(module=module.__name__, flag=candidate, line=line.strip()[:90]): + self.assertIn(candidate, flags, f"message mentions unknown flag {candidate}") + + def test_entrypoint_messages(self): + self._check(entrypoint) + + def test_seg_run_messages(self): + self._check(seg_run) + + +class Test_Input_Validation_Messages(unittest.TestCase): + @staticmethod + def _opt(input_path: str) -> argparse.Namespace: + parser = argparse.ArgumentParser() + entrypoint.parser_arguments(parser) + opt = parser.parse_args([]) + opt.input = input_path + opt.model_semantic = "t2w" + opt.model_instance = "instance" + opt.model_labeling = "none" + return opt + + def test_uncompressed_nifti_says_so(self): + with tempfile.TemporaryDirectory() as td: + path = Path(td) / "scan.nii" + path.write_bytes(b"") + with self.assertRaises(ValueError) as cm: + entrypoint.run_sample(self._opt(str(path))) + message = str(cm.exception) + self.assertIn("scan.nii", message) + self.assertIn(".nii.gz", message) + # and it must not invent a path the user never typed + self.assertNotIn("scan.nii.nii.gz", message) + + def test_missing_file_names_the_path_given(self): + with tempfile.TemporaryDirectory() as td: + missing = Path(td) / "sub-01_T2w.nii.gz" + with self.assertRaises(FileNotFoundError) as cm: + entrypoint.run_sample(self._opt(str(missing))) + self.assertIn(str(missing), str(cm.exception)) + + def test_missing_folder_names_the_folder(self): + with self.assertRaises(FileNotFoundError) as cm: + entrypoint.run_sample(self._opt("/this/path/does/not/exist/scan.nii.gz")) + self.assertIn("/this/path/does/not/exist", str(cm.exception)) + + def test_extension_may_still_be_omitted(self): + # `--input sub-01_T2w` keeps working: the extension is appended only if nothing is there already. + with tempfile.TemporaryDirectory() as td: + path = Path(td) / "sub-01_T2w.nii.gz" + path.write_bytes(b"") + with self.assertRaises(Exception) as cm: # fails later, in model loading + entrypoint.run_sample(self._opt(str(path)[: -len(".nii.gz")])) + self.assertNotIsInstance(cm.exception, (FileNotFoundError, ValueError)) + + +if __name__ == "__main__": + unittest.main() From be6b73e37c23d34f36637afeff91f38f53726009 Mon Sep 17 00:00:00 2001 From: iback Date: Mon, 5 Oct 2026 08:35:49 +0000 Subject: [PATCH 2/4] test: do not read an f-string placeholder as part of a flag name `--model-{kind}` is assembled at runtime, so the literal `--model` prefix is not a flag. Co-Authored-By: Claude Opus 5 --- unit_tests/test_cli_messages.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/unit_tests/test_cli_messages.py b/unit_tests/test_cli_messages.py index dc3456b..3165ea1 100644 --- a/unit_tests/test_cli_messages.py +++ b/unit_tests/test_cli_messages.py @@ -53,7 +53,11 @@ def _check(self, module) -> None: if "logger.print(" not in line and "raise " not in line and 'f"' not in line: continue for quoted in re.findall(r'"([^"]*)"', line): - for candidate in self.FLAG_PATTERN.findall(quoted): + for match in self.FLAG_PATTERN.finditer(quoted): + candidate = match.group() + # "--model-{kind}" is built at runtime; the literal prefix is not a flag on its own. + if quoted[match.end() : match.end() + 1] == "{": + continue if "_" in candidate or candidate.startswith("--"): with self.subTest(module=module.__name__, flag=candidate, line=line.strip()[:90]): self.assertIn(candidate, flags, f"message mentions unknown flag {candidate}") From 8b65f9b4a2aed5bd423981c47a973cd5f3d3fde7 Mon Sep 17 00:00:00 2001 From: iback Date: Mon, 5 Oct 2026 08:38:16 +0000 Subject: [PATCH 3/4] test: skip a flag prefix that is followed by an f-string placeholder `--model-{kind}` is assembled at runtime, so `--model` is not a flag the parser has to know. Co-Authored-By: Claude Opus 5 --- unit_tests/test_cli_messages.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/unit_tests/test_cli_messages.py b/unit_tests/test_cli_messages.py index 3165ea1..0fb50bd 100644 --- a/unit_tests/test_cli_messages.py +++ b/unit_tests/test_cli_messages.py @@ -55,8 +55,10 @@ def _check(self, module) -> None: for quoted in re.findall(r'"([^"]*)"', line): for match in self.FLAG_PATTERN.finditer(quoted): candidate = match.group() - # "--model-{kind}" is built at runtime; the literal prefix is not a flag on its own. - if quoted[match.end() : match.end() + 1] == "{": + # "--model-{kind}" is assembled at runtime, so the literal "--model" prefix in front + # of the placeholder is not a flag on its own. + rest = quoted[match.end() :] + if rest.startswith("{") or (rest[:1] in "-_" and rest[1:2] == "{"): continue if "_" in candidate or candidate.startswith("--"): with self.subTest(module=module.__name__, flag=candidate, line=line.strip()[:90]): From 9f9bda437dbc3a23bca0a2ec6502a392c0af346b Mon Sep 17 00:00:00 2001 From: iback Date: Mon, 5 Oct 2026 08:44:31 +0000 Subject: [PATCH 4/4] test: assert on a native path so the message test passes on Windows A hardcoded POSIX path never appears in the message on Windows, where the input is absolutised to D:\this\path\... Co-Authored-By: Claude Opus 5 --- unit_tests/test_cli_messages.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/unit_tests/test_cli_messages.py b/unit_tests/test_cli_messages.py index 0fb50bd..f9a5cb3 100644 --- a/unit_tests/test_cli_messages.py +++ b/unit_tests/test_cli_messages.py @@ -103,9 +103,12 @@ def test_missing_file_names_the_path_given(self): self.assertIn(str(missing), str(cm.exception)) def test_missing_folder_names_the_folder(self): - with self.assertRaises(FileNotFoundError) as cm: - entrypoint.run_sample(self._opt("/this/path/does/not/exist/scan.nii.gz")) - self.assertIn("/this/path/does/not/exist", str(cm.exception)) + with tempfile.TemporaryDirectory() as td: + # a native path, so the assertion holds on Windows too + missing_folder = Path(td) / "does" / "not" / "exist" + with self.assertRaises(FileNotFoundError) as cm: + entrypoint.run_sample(self._opt(str(missing_folder / "scan.nii.gz"))) + self.assertIn(str(missing_folder), str(cm.exception)) def test_extension_may_still_be_omitted(self): # `--input sub-01_T2w` keeps working: the extension is appended only if nothing is there already.