From 0a2c2d3e7bc1c6100969e4ff9fe79068facc5af0 Mon Sep 17 00:00:00 2001 From: Andrey Fedorov Date: Thu, 3 Sep 2026 10:45:36 -0400 Subject: [PATCH] Stop the version check from silently not running MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `python scripts/check_version.py` appeared in SKILL.md and seven guides. It cannot run on a bare Homebrew macOS install, which has no `python`, and fails with `command not found` — a non-fatal shell error easily mistaken for noise, leaving the check skipped entirely. `python3` is not the fix: it is absent inside a Windows virtual environment, and on Windows either name can resolve to a Microsoft Store alias stub that opens the Store instead of running the script. No interpreter name is portable, which is the same defect TestInstallCommands already pins for `pip` — an ambiguous name in front of the thing you meant to run. So the guides now name the script by path and tell the caller to use the interpreter that will run idc-index, and to confirm the "meets pinned minimum" line rather than assume the command ran. The idc-index setup snippet repeats the check in Python, where it cannot be skipped or fail to launch: the interpreter that imports idc_index is by construction the one that will query. This matters because the failure it guards is silent. Identifier values are renormalised between releases while the reported data version is not: 0.12.3 and 0.12.5 both serve v24, but spell the Pan-Cancer nuclei segmentations Pan-Cancer-Nuclei-Seg-DICOM and pan_cancer_nuclei_seg_dicom respectively. A query written against the wrong one returns zero rows rather than raising, so it reads as "no such data" instead of "your index is stale". The stale-index output now says that instead of only reporting the version gap. Also bumps the version pins, which 1.8.2 left at 1.8.1. Both agreed with each other, so the existing sync tests passed while every current checkout reported "Skill 1.8.2 available (you have 1.8.1)". They now track the CHANGELOG too. Found reviewing a pull request against IDC-Tutorials: the review ran against a stale 0.12.3, got zero rows, and reported a working notebook as broken. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 27 ++++++++++++++++++++ SKILL.md | 23 ++++++++++------- references/cli_guide.md | 42 ++++++++++++++++++++++++++++++- references/clinical_data_guide.md | 2 +- references/index_tables_guide.md | 2 +- references/mcp_guide.md | 2 +- references/rest_api_guide.md | 6 ++--- references/sql_patterns.md | 2 +- references/use_cases.md | 2 +- scripts/check_version.py | 12 +++++++-- tests/test_check_version.py | 13 ++++++++++ tests/test_structure.py | 39 ++++++++++++++++++++++++++++ 12 files changed, 152 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b76973b..3f7c788 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,33 @@ All notable changes to the Imaging Data Commons Skill are documented in this fil The format is based on [Keep a Changelog](https://keepachangelog.com/), and this project adheres to [Semantic Versioning](https://semver.org/). +## [1.8.3] - 2026-09-03 + +A stale index returns zero rows rather than an error, and `scripts/check_version.py` was +too easy to skip. Prompted by an IDC-Tutorials review that ran against 0.12.3 and reported +a working notebook as broken. + +### Fixed + +- Documented invocation of `scripts/check_version.py` no longer hardcodes an interpreter. + No name is portable — `python` and `python3` each go missing on common platforms, and + the failure is a non-fatal `command not found`. Pinned by `tests/test_structure.py` +- Version pins, left at 1.8.1 by the 1.8.2 release, so every checkout reported an update + that was already installed. Now tracked by `tests/test_check_version.py` + +### Added + +- `SKILL.md` *Running the version check*: confirm the `meets pinned minimum` line rather + than assume the command ran +- Setup snippet repeats the check in Python, where it cannot be skipped or fail to launch +- Stale-index output states the consequence — zero rows, not an error — instead of only + the version gap + +### Changed + +- Access-path gate step 2 asks whether `idc-index` is "installed and current", not just + "installed", which an agent that already has it reads as a satisfied precondition + ## [1.8.2] - 2026-08-12 ### Added diff --git a/SKILL.md b/SKILL.md index 2b64459..ecd64ab 100644 --- a/SKILL.md +++ b/SKILL.md @@ -3,7 +3,7 @@ name: imaging-data-commons description: Query and download public cancer imaging data from NCI Imaging Data Commons. Invoke for any question about IDC collections, cancer imaging datasets, DICOM data access, radiology (CT, MR, PET) or pathology AI training sets, metadata queries, visualization, or license checks — even when the user doesn't explicitly mention "IDC". No authentication required. license: This skill is provided under the MIT License. IDC data itself has individual licensing (mostly CC-BY, some CC-NC) that must be respected when using the data. metadata: - version: 1.8.1 + version: 1.8.3 skill-author: Andrey Fedorov, @fedorov idc-index: "0.12.5" idc-data-version: "v24" @@ -25,8 +25,9 @@ on the session and the task. 1. **Session already has the IDC MCP server?** Route discovery and metadata there — see *IDC MCP Server*. -2. **Otherwise, is `idc-index` installed?** Run `python scripts/check_version.py`. If it passes, - use `idc-index` for everything. +2. **Otherwise, is `idc-index` installed and current?** Run `scripts/check_version.py`; confirm + it prints `meets pinned minimum` — a `command not found` is not a pass, see + `references/cli_guide.md`. If it passes, use `idc-index` for everything. 3. **Not installed, and the task is read-only metadata** — counts, attribute values, collection lookups, SQL under 10 000 rows, licenses, citations, viewer URLs? **Use the REST API over `curl`; do not install anything.** Installing costs ~77 MB of packaged index data plus @@ -38,19 +39,23 @@ on the session and the task. the exact install command for the running interpreter. Prefer a virtual environment, then restart Python. -`idc-index` ([GitHub](https://github.com/imagingdatacommons/idc-index)) is still the most -capable path and the only one that moves image bytes; the rule is just not to pay for it before -the task calls for it. `check_version.py` never installs anything itself — it also flags a newer +`idc-index` ([GitHub](https://github.com/imagingdatacommons/idc-index)) is still the most capable +path and the only one that moves image bytes; the rule is just not to pay for it before the task +calls for it. `check_version.py` never installs anything itself — it also flags a newer `idc-index` or skill release when one exists. **Setup for the `idc-index` path:** ```python +import re, idc_index from idc_index import IDCClient -client = IDCClient() +# Repeat the startup check where it cannot be skipped — see references/cli_guide.md. +if [int(re.sub(r"\D.*", "", p) or 0) for p in idc_index.__version__.split(".")[:3]] < [0, 12, 5]: + raise RuntimeError(f"idc-index {idc_index.__version__} is stale (need 0.12.5): a stale " + "index returns zero rows, not an error. Run scripts/check_version.py.") -# Verify IDC data version (should be "v24") -print(f"IDC data version: {client.get_idc_version()}") +client = IDCClient() +print(f"IDC data version: {client.get_idc_version()}") # should be "v24" ``` **Core workflow:** query metadata with `client.sql_query()` → download with diff --git a/references/cli_guide.md b/references/cli_guide.md index 38ccba0..eda4b54 100644 --- a/references/cli_guide.md +++ b/references/cli_guide.md @@ -4,11 +4,51 @@ The `idc-index` package provides command-line tools for downloading DICOM data f ## Installation -Needs `idc-index` installed — run `python scripts/check_version.py`, which reports the installed +Needs `idc-index` installed — run `scripts/check_version.py`, which reports the installed version and prints the install command for the interpreter you are running. After installation, the `idc` command is available in your terminal. +### Running the version check + +Invoke the script with **the interpreter that will run `idc-index`** — it exists to inspect +*that* environment, so `python`, `python3`, `py -3`, or an explicit virtual-environment path +are all correct as long as it is the same one. The guides deliberately never put an interpreter +name in front of the path, because no name is portable: + +| Environment | `python` | `python3` | +|-------------|----------|-----------| +| Bare Homebrew macOS | absent | present | +| Windows (python.org installer) | present | absent | +| Windows virtual environment | present | absent | +| POSIX virtual environment | present | present | + +On Windows there is a worse case: `%LOCALAPPDATA%\Microsoft\WindowsApps` ships alias stubs for +both names that open the Microsoft Store instead of running anything, so the command appears to +resolve and still produces no check. + +**Confirm the outcome, not the command.** A pass prints + +``` +idc-index meets pinned minimum (0.12.5) +``` + +and exits 0. Anything else — `command not found`, a permission error (the bundled script is not +marked executable, and vendoring often drops the bit anyway), or no output at all — means the +check did not run. That is not a pass. + +The distinction matters because the failure it guards against is silent. Identifier values are +renormalised between `idc-index` releases while the reported IDC data version stays the same: +0.12.3 and 0.12.5 both report `v24`, but the Pan-Cancer nuclei segmentations are +`Pan-Cancer-Nuclei-Seg-DICOM` in the first and `pan_cancer_nuclei_seg_dicom` in the second. A +query written against the wrong one returns zero rows rather than raising, which reads as "no +such data" instead of "your index is stale". `get_idc_version()` cannot distinguish them, so +the package version is the only signal. + +For that reason the `idc-index` setup snippet in `SKILL.md` repeats the check in Python, after +importing `idc_index`. That copy cannot be skipped or fail to launch: the interpreter that +imports the package is by definition the one that will run the queries. + ## Available Commands | Command | Purpose | diff --git a/references/clinical_data_guide.md b/references/clinical_data_guide.md index cd82ad2..3b2d9e3 100644 --- a/references/clinical_data_guide.md +++ b/references/clinical_data_guide.md @@ -16,7 +16,7 @@ For basic clinical data access, see the "Clinical Data Access" section in the ma ## Prerequisites -Needs `idc-index` installed — run `python scripts/check_version.py`, which reports the installed +Needs `idc-index` installed — run `scripts/check_version.py`, which reports the installed version and prints the install command for the interpreter you are running. No BigQuery credentials required - clinical data is packaged with `idc-index`. diff --git a/references/index_tables_guide.md b/references/index_tables_guide.md index 7943381..d89ff9b 100644 --- a/references/index_tables_guide.md +++ b/references/index_tables_guide.md @@ -17,7 +17,7 @@ For SQL query examples (filter discovery, finding annotations, size estimation), ## Prerequisites -Needs `idc-index` installed — run `python scripts/check_version.py`, which reports the installed +Needs `idc-index` installed — run `scripts/check_version.py`, which reports the installed version and prints the install command for the interpreter you are running. ## Available Tables diff --git a/references/mcp_guide.md b/references/mcp_guide.md index 43c72d3..7f3ad99 100644 --- a/references/mcp_guide.md +++ b/references/mcp_guide.md @@ -130,7 +130,7 @@ client.download_from_selection( ) ``` -Run `python scripts/check_version.py` before the first `idc-index` call in a session, even if +Run `scripts/check_version.py` before the first `idc-index` call in a session, even if discovery happened server-side — the two components version independently. `get_cohort_urls` also returns ready-made `idc` CLI commands. Those are the better handoff diff --git a/references/rest_api_guide.md b/references/rest_api_guide.md index 7be46f7..6b10a89 100644 --- a/references/rest_api_guide.md +++ b/references/rest_api_guide.md @@ -106,7 +106,7 @@ but says nothing about the index build. When the two disagree, say so and name both versions, then reconcile rather than mixing results: upgrading `idc-index` brings the local side to the newer `idc-index-data`, and -`python scripts/check_version.py` reports whether an upgrade is available and prints the +`scripts/check_version.py` reports whether an upgrade is available and prints the command for the interpreter you are running. Do not present API-derived and locally-derived counts side by side as if they came from one index. @@ -480,7 +480,7 @@ not contain are silently dropped from the selection. **Fix it one of two ways:** -1. **Upgrade** — upgrade `idc-index` (`python scripts/check_version.py` prints the command), +1. **Upgrade** — upgrade `idc-index` (`scripts/check_version.py` prints the command), then re-run. This is the right answer whenever it is possible; it restores the hierarchy, size checks, and progress reporting. 2. **Bypass the index** — transfer directly from the bucket. The manifest URLs are @@ -578,7 +578,7 @@ client.download_from_selection( ) ``` -Run `python scripts/check_version.py` before the first `idc-index` call in a session, even when +Run `scripts/check_version.py` before the first `idc-index` call in a session, even when discovery happened over the API — the two version independently. Compare `idc_index_data_version` on both sides first (see *Checking the API against a local idc-index*). diff --git a/references/sql_patterns.md b/references/sql_patterns.md index 7130251..05691fe 100644 --- a/references/sql_patterns.md +++ b/references/sql_patterns.md @@ -20,7 +20,7 @@ For table schemas, DataFrame access, and join column references, see `references ## Prerequisites -Needs `idc-index` installed — run `python scripts/check_version.py`, which reports the installed +Needs `idc-index` installed — run `scripts/check_version.py`, which reports the installed version and prints the install command for the interpreter you are running. ```python diff --git a/references/use_cases.md b/references/use_cases.md index d0a5c4b..46f5687 100644 --- a/references/use_cases.md +++ b/references/use_cases.md @@ -16,7 +16,7 @@ For core API patterns (query, download, visualize, citations), see the "Core Cap ## Prerequisites -Needs `idc-index` installed — run `python scripts/check_version.py`, which reports the installed +Needs `idc-index` installed — run `scripts/check_version.py`, which reports the installed version and prints the install command for the interpreter you are running. ## Use Case 1: Find and Download Lung CT Scans for Deep Learning diff --git a/scripts/check_version.py b/scripts/check_version.py index d93253b..921a31f 100644 --- a/scripts/check_version.py +++ b/scripts/check_version.py @@ -1,7 +1,11 @@ #!/usr/bin/env python3 """Check the idc-index package and this skill for required/available updates. -Run FIRST at the start of an IDC session: python scripts/check_version.py +Run FIRST at the start of an IDC session, with the interpreter that will run +idc-index -- the point is to inspect that environment. No interpreter name is +portable enough to hardcode in the docs (`python` is missing on a bare Homebrew +macOS install, `python3` inside a Windows venv), so SKILL.md tells the caller to +confirm the "meets pinned minimum" line rather than to type a fixed command. - Verifies that idc-index is installed and at least MIN_VERSION. It never installs or upgrades anything itself: if the requirement is not met it prints @@ -18,7 +22,7 @@ import sys MIN_VERSION = "0.12.5" # keep in sync with metadata.idc-index in SKILL.md -SKILL_VERSION = "1.8.1" # keep in sync with metadata.version in SKILL.md +SKILL_VERSION = "1.8.3" # keep in sync with metadata.version in SKILL.md REPO = "ImagingDataCommons/imaging-data-commons-skill" _LEADING_DIGITS = re.compile(r"\d+") @@ -100,6 +104,10 @@ def check_minimum(): installed = idc_index.__version__ if parse_version(installed) < parse_version(MIN_VERSION): print(f"idc-index {installed} is below the pinned minimum {MIN_VERSION}.") + print("Identifier values change between releases even when the reported IDC data") + print("version does not, so a stale index returns zero rows instead of an error:") + print("the Pan-Cancer nuclei segmentations are 'Pan-Cancer-Nuclei-Seg-DICOM' in") + print("0.12.3 and 'pan_cancer_nuclei_seg_dicom' in 0.12.5, and both report v24.") print_install_instructions(f"idc-index=={MIN_VERSION}") return None diff --git a/tests/test_check_version.py b/tests/test_check_version.py index 0e9c009..9b1b4ad 100644 --- a/tests/test_check_version.py +++ b/tests/test_check_version.py @@ -172,6 +172,19 @@ def test_skill_version_matches_metadata(self): meta = re.search(r"version:\s*\"?([\d.]+)\"?", self._frontmatter()).group(1) assert check_version.SKILL_VERSION == meta + def test_skill_version_matches_changelog(self): + """The pins must also track the CHANGELOG, which the two tests above cannot see. + + The 1.8.2 release bumped the CHANGELOG and neither pin. Both pins still agreed + with each other at 1.8.1, so CI passed while every fresh checkout printed + "Skill 1.8.2 available (you have 1.8.1)" — the update notice permanently, and + wrongly, telling users they were a release behind. + """ + changelog = os.path.join(os.path.dirname(__file__), "..", "CHANGELOG.md") + with open(changelog, encoding="utf-8") as handle: + newest = re.search(r"^## \[([\d.]+)\]", handle.read(), re.M).group(1) + assert check_version.SKILL_VERSION == newest + class _FakeIdcIndex: """Stand-in for the real package, so the check runs with nothing installed.""" diff --git a/tests/test_structure.py b/tests/test_structure.py index b14470c..d510ebd 100644 --- a/tests/test_structure.py +++ b/tests/test_structure.py @@ -175,3 +175,42 @@ def test_no_installer_commands_in_skill_or_references(self): "state the package that is needed and point at scripts/check_version.py " "instead of hardcoding an installer:\n" + "\n".join(offenders) ) + + +class TestInterpreterCommands: + """No hardcoded interpreter name in front of a bundled script. + + The same defect as TestInstallCommands, one level up. `python scripts/check_version.py` + names an interpreter that need not exist: `python` is absent on a bare Homebrew macOS + install, `python3` inside a Windows virtual environment, and on Windows either can + resolve to a Microsoft Store alias stub that opens the Store instead of running the + script. Every one of those failures surfaces as a shell error rather than a check + result, so a caller that scrolls past it proceeds on an unverified — possibly stale — + index, which returns zero rows instead of raising. + + SKILL.md therefore names the success line to confirm rather than a command to type, + and the setup snippet re-checks in the interpreter that imports idc_index. + """ + + def _documents(self): + documents = {"SKILL.md": _read(_SKILL_MD)} + for name in sorted(os.listdir(_REFERENCES)): + if name.endswith(".md"): + documents[f"references/{name}"] = _read(os.path.join(_REFERENCES, name)) + scripts = os.path.join(_ROOT, "scripts") + for name in sorted(os.listdir(scripts)): + if name.endswith(".py"): + documents[f"scripts/{name}"] = _read(os.path.join(scripts, name)) + return documents + + def test_no_interpreter_prefix_before_bundled_scripts(self): + pattern = re.compile(r"\b(?:python[\d.]*|py)\s+(?:-\S+\s+)?scripts/") + offenders = [] + for name, text in self._documents().items(): + for number, line in enumerate(text.splitlines(), 1): + if pattern.search(line): + offenders.append(f"{name}:{number}: {line.strip()}") + assert not offenders, ( + "refer to the script by path and let the caller pick the interpreter that will " + "run idc-index; no interpreter name is portable:\n" + "\n".join(offenders) + )