diff --git a/.claude/skills/pre-release/SKILL.md b/.claude/skills/pre-release/SKILL.md index 76ecd249..63634969 100644 --- a/.claude/skills/pre-release/SKILL.md +++ b/.claude/skills/pre-release/SKILL.md @@ -15,17 +15,34 @@ Execute the sections in order. After each, capture pass/fail and any findings. A ### Section 1 — API modules present in `docs/source/api.md` -For every `src/ptychodus/api/*.py` file (excluding `_*.py` private modules), confirm `docs/source/api.md` contains a matching `.. automodule:: ptychodus.api.` directive. +For every `src/ptychodus/api/**/*.py` file, confirm `docs/source/api.md` contains a matching `.. automodule:: ptychodus.api.` directive. The scan is recursive, so subpackage modules (`preprocess/`, `simulate/`) are covered under their dotted names. A path component starting with `_` is skipped at any depth — that covers `__init__.py`, `simulate/_phase_unwrap.py`, and any future private subpackage. -```sh -# Modules that should be documented: -ls src/ptychodus/api/*.py | xargs -n1 basename | sed 's/\.py$//' | grep -v '^_' | grep -v '^__' | sort +Do not enumerate with a flat `ls src/ptychodus/api/*.py`, and do not match the directive with a `[a-z_]+` character class: the former misses every subpackage module, and the latter has no `.` so `preprocess.diffraction` truncates to `preprocess`. The two errors cancel out and the section reports `PASS` while blind to an undocumented subpackage module. -# Modules currently documented: -grep -oE '\.\. automodule:: ptychodus\.api\.[a-z_]+' docs/source/api.md | sed 's|.*ptychodus\.api\.||' | sort +```sh +uv run python -c " +import pathlib, re +root = pathlib.Path('src/ptychodus/api') +have = set() +for p in sorted(root.rglob('*.py')): + rel = p.relative_to(root).with_suffix('') + if any(part.startswith('_') for part in rel.parts): + continue + have.add('.'.join(rel.parts)) +doc = set(re.findall(r'\.\. automodule:: ptychodus\.api\.([A-Za-z_][A-Za-z_0-9.]*)', + pathlib.Path('docs/source/api.md').read_text())) +print(f'modules={len(have)} documented={len(doc)}') +for m in sorted(have - doc): + print(f'MISSING FROM DOCS: ptychodus.api.{m}') +for m in sorted(doc - have): + print(f'STALE DOC ENTRY: ptychodus.api.{m}') +print('PASS' if have == doc else 'FAIL') +" ``` -Compute the diff. `PASS` if empty. `FAIL` with the list of missing module names. +`PASS` if the script prints `PASS`. `FAIL` with the list of `MISSING FROM DOCS` and `STALE DOC ENTRY` lines. + +A `STALE DOC ENTRY` is a module that `api.md` documents but that no longer exists. Sphinx catches this downstream as an autodoc import failure under Section 6's `-W`, but naming it here gives a far better message. The fix is to delete the orphaned stanza from `docs/source/api.md`. **Suggested fix per missing module**: append this stanza to `docs/source/api.md`, matching the existing pattern (title-case `##` heading, then the autodoc directive as raw reStructuredText inside an `{eval-rst}` block): @@ -85,41 +102,63 @@ print(f'--- {len(missing)} missing ---') ### Section 3 — Reader plugins represented in `docs/source/readers.md` -`docs/source/readers.md` is a curated bullet list, not a 1:1 file mapping. Cross-check by keyword: for every distinct beamline/format hint in `src/ptychodus/plugins/`, ensure the readers doc mentions it. - -Keyword map (edit as new plugins land): - -| Plugin filename substring | Expected mention in readers.md | -| --- | --- | -| `aps02id_` | "2-ID-D Bionanoprobe" or "2-ID-D Microprobe" or "2-ID-E Microprobe" | -| `aps04id_polar_` | "4-ID" and "Polar" (Polarization Modulation Spectroscopy) | -| `aps09id_cssi_` | "9-ID" and "CSSI" | -| `aps12id_` | "12-ID" and "SAXS" | -| `aps19id_isn_` | "19-ID" and "ISN" | -| `aps31id_lynx_` | "31-ID" and "LYNX" | -| `aps33id_velociprobe` | "33-ID" and "Velociprobe" | -| `lcls_` | "LCLS" | -| `max_iv_nanomax_` | "MAX IV" or "NanoMAX" | -| `fold_slice_` | "fold_slice" | -| `cxi_` | "CXI" or "*.cxi" | -| `csv_` | "CSV" or "Comma-Separated" | -| `mda_` | "MDA" or "*.mda" | -| `npy_` | "NumPy" or "*.npy" | -| `delimited_position_` | "Space-Separated" or "*.txt" | +`docs/source/readers.md` is a curated bullet list of user-facing names, not a 1:1 file mapping. Cross-check it against the plugin registry rather than against filenames: every plugin already declares a user-facing `display_name` in its `register_plugins` hook, and those strings are the authoritative source. -```sh -# List all plugin file stems: -ls src/ptychodus/plugins/*.py | xargs -n1 basename | sed 's/\.py$//' +Drive the check from `PluginRegistry.load_plugins()` so it reuses the exact discovery path the application uses. This matters because `pkgutil.iter_modules` is non-recursive: it yields `aps33id_velociprobe` as a *package* and calls the `register_plugins` in its `__init__.py`. A flat `ls src/ptychodus/plugins/*.py` misses that package entirely, and a hand-maintained keyword table drifts silently as new plugins land — do not reintroduce either. -# For each keyword above, verify readers.md contains the expected mention: -for kw in "2-ID-D Bionanoprobe" "4-ID" "9-ID" "12-ID" "19-ID" "31-ID" "33-ID" "LCLS" "NanoMAX" "fold_slice" "CXI" "CSV" "MDA" "NumPy" "Space-Separated"; do - grep -q "$kw" docs/source/readers.md || echo "MISSING: $kw" -done +Comparison is by facility token and beamline token, in two tiers: + +- **FAIL** — a token a plugin registers that `readers.md` never mentions. This is the real drift risk: a new beamline reader lands and the doc is not updated. +- **REVIEW** — a token the doc claims that no plugin display name mentions. Report-only, because the doc legitimately uses friendlier names than the plugins do. Making this a hard failure would produce false alarms. + +```sh +uv run python -c " +import logging, re, pathlib + +skipped = [] +class _H(logging.Handler): + def emit(self, record): + m = record.getMessage() + if m.startswith(('Skipping ', 'Failed to register ')): + skipped.append(m) +_lg = logging.getLogger('ptychodus.api.plugins') +_lg.addHandler(_H()) +_lg.setLevel(logging.WARNING) + +from ptychodus.api.plugins import PluginRegistry +registry = PluginRegistry.load_plugins() +names = {p.display_name + for a in vars(registry) if a.endswith('_file_readers') + for p in getattr(registry, a)} +doc = pathlib.Path('docs/source/readers.md').read_text() + +FACILITIES = ['APS', 'CNM', 'LCLS', 'MAX IV', 'NSLS-II', 'SLAC', 'SLS'] +BEAMLINE = re.compile(r'\b\d+-ID\b') + +def tokens(text): + found = {f for f in FACILITIES if re.search(rf'\b{re.escape(f)}\b', text)} + return found | set(BEAMLINE.findall(text)) + +plug = tokens(' | '.join(names)) +docs = tokens(doc) +print(f'reader plugins registered: {len(names)}; modules skipped at load: {len(skipped)}') +for m in skipped: + print(f' {m}') +fails = sorted(plug - docs) +for t in fails: + print(f'FAIL in plugins, missing from readers.md: {t}') +for t in sorted(docs - plug): + print(f'REVIEW in readers.md, no plugin names it: {t}') +print('PASS' if not fails else 'FAIL') +" ``` -Also flag *the reverse*: any bullet in `readers.md` whose beamline has no matching plugin file — that indicates a stale doc entry. +Two details in that script are load-bearing: + +- Facility matching uses `\b...\b`, not substring. A plain `'SLS' in text` also matches **NSLS-II**, which would silently mask a missing SLS entry. +- The script reports `modules skipped at load`. An optional-dependency plugin that fails to import registers nothing, which shrinks the plugin token set and weakens the check in the safe direction — under-reporting, never a false `FAIL`. Run the release gate in an environment with the full extras, and treat a nonzero skip count as a caveat on this section's coverage. -`PASS` if every plugin has a doc mention and every doc mention has a plugin. `FAIL` with lists of orphans in either direction. +`PASS` if the script prints `PASS` (no `FAIL` lines). `REVIEW` lines do not fail the gate, but report them so a human can adjudicate. **Suggested fix per finding**: propose a bullet insertion under the correct facility heading, matching the existing style (`- ()`). Ask the user to confirm the heading and wording — the doc uses friendly names, not filenames. diff --git a/docs/source/readers.md b/docs/source/readers.md index fc3867fd..5c26de02 100644 --- a/docs/source/readers.md +++ b/docs/source/readers.md @@ -12,7 +12,6 @@ File readers are implemented using a Python namespace plugin system. We would be - 19-ID-E In-Situ Nanoprobe (ISN) - 26-ID-C CNM/APS Hard X-ray Nanoprobe (HXN) - 31-ID-E LYNX - - 33-ID-C PtychoProbe - 33-ID-C Velociprobe - 34-ID-C Microdiffraction, Coherent X-ray Scattering - Linac Coherent Light Source (LCLS) diff --git a/src/ptychodus/api/plugins.py b/src/ptychodus/api/plugins.py index 5eadc960..8ea1a64c 100644 --- a/src/ptychodus/api/plugins.py +++ b/src/ptychodus/api/plugins.py @@ -116,6 +116,19 @@ def register_plugin(self, strategy: T, *, display_name: str, simple_name: str = if not simple_name: simple_name = re.sub(r'\W+', '', display_name) + # Settings persist the simple name, and lookup returns the first match, so a + # duplicate silently shadows the later registration and breaks its round trip. + # Warn rather than raise: load_plugins catches only AttributeError, and plugin + # loading must never be fatal. + for plugin in self._registered_plugins: + if plugin.simple_name.casefold() == simple_name.casefold(): + logger.warning( + f'Duplicate plugin simple name "{simple_name}": ' + f'"{display_name}" will be unreachable by that name ' + f'because "{plugin.display_name}" already claims it.' + ) + break + # The list is kept sorted by display name, so a registration can move the # selected plugin to a different index. Track it by identity across the sort # rather than by value: Plugin is a frozen dataclass, so equality compares the diff --git a/src/ptychodus/plugins/fold_slice_product_file.py b/src/ptychodus/plugins/fold_slice_product_file.py index 03565ad1..27cd0401 100644 --- a/src/ptychodus/plugins/fold_slice_product_file.py +++ b/src/ptychodus/plugins/fold_slice_product_file.py @@ -21,7 +21,7 @@ class FoldSliceProductFileReader(ProductFileReader): - SIMPLE_NAME: Final[str] = 'fold_slice' + SIMPLE_NAME: Final[str] = 'fold_slice_mat' DISPLAY_NAME: Final[str] = 'fold_slice Files (*.mat)' def read(self, file_path: Path) -> Product: diff --git a/src/ptychodus/plugins/mda_position_file.py b/src/ptychodus/plugins/mda_position_file.py index 213b957d..96851e1f 100644 --- a/src/ptychodus/plugins/mda_position_file.py +++ b/src/ptychodus/plugins/mda_position_file.py @@ -528,7 +528,7 @@ def register_plugins(registry: PluginRegistry) -> None: registry.probe_position_file_readers.register_plugin( MDAFlatScanPositionFileReader(scale_to_meters=1.0e-6), simple_name='CNM_APS_HXN', - display_name='CNM/APS Hard X-ray Nanoprobe Files (*.mda)', + display_name='CNM/APS 26-ID Hard X-ray Nanoprobe Files (*.mda)', ) diff --git a/src/ptychodus/plugins/nsls2_diffraction_file.py b/src/ptychodus/plugins/nsls2_diffraction_file.py index 8ee31b27..738a4a61 100644 --- a/src/ptychodus/plugins/nsls2_diffraction_file.py +++ b/src/ptychodus/plugins/nsls2_diffraction_file.py @@ -136,15 +136,15 @@ def register_plugins(registry: PluginRegistry) -> None: registry.diffraction_file_readers.register_plugin( NSLS2Style1DiffractionFileReader(), simple_name='NSLS_II_1', - display_name='NSLS-II Style 1 Files (*.h5 *.hdf5)', + display_name='NSLS-II 3-ID HXN Style 1 Files (*.h5 *.hdf5)', ) registry.diffraction_file_readers.register_plugin( NSLS2Style2DiffractionFileReader(), simple_name='NSLS_II_2', - display_name='NSLS-II Style 2 Files (*.h5 *.hdf5)', + display_name='NSLS-II 3-ID HXN Style 2 Files (*.h5 *.hdf5)', ) registry.diffraction_file_readers.register_plugin( NSLS2MATLABDiffractionFileReader(), simple_name='NSLS_II_MATLAB', - display_name='NSLS-II MATLAB Files (*.mat)', + display_name='NSLS-II 3-ID HXN MATLAB Files (*.mat)', ) diff --git a/src/ptychodus/plugins/nsls2_position_file.py b/src/ptychodus/plugins/nsls2_position_file.py index b9a30cdb..ac7756e9 100644 --- a/src/ptychodus/plugins/nsls2_position_file.py +++ b/src/ptychodus/plugins/nsls2_position_file.py @@ -54,10 +54,10 @@ def register_plugins(registry: PluginRegistry) -> None: registry.probe_position_file_readers.register_plugin( NSLS2Style1PositionFileReader(), simple_name='NSLS_II_1', - display_name='NSLS-II Style 1 Files (*.h5 *.hdf5)', + display_name='NSLS-II 3-ID HXN Style 1 Files (*.h5 *.hdf5)', ) registry.probe_position_file_readers.register_plugin( NSLS2Style2PositionFileReader(), simple_name='NSLS_II_2', - display_name='NSLS-II Style 2 Files (*.h5 *.hdf5)', + display_name='NSLS-II 3-ID HXN Style 2 Files (*.h5 *.hdf5)', ) diff --git a/src/ptychodus/plugins/nsls2_product_file.py b/src/ptychodus/plugins/nsls2_product_file.py index 5c638091..658f48bf 100644 --- a/src/ptychodus/plugins/nsls2_product_file.py +++ b/src/ptychodus/plugins/nsls2_product_file.py @@ -14,7 +14,7 @@ class NSLSIIProductFileReader(ProductFileReader): SIMPLE_NAME: Final[str] = 'NSLS_II_MATLAB' - DISPLAY_NAME: Final[str] = 'NSLS-II MATLAB Files (*.mat)' + DISPLAY_NAME: Final[str] = 'NSLS-II 3-ID HXN MATLAB Files (*.mat)' def read(self, file_path: Path) -> Product: point_list: list[ProbePosition] = list() diff --git a/src/ptychodus/plugins/tiff_diffraction_file.py b/src/ptychodus/plugins/tiff_diffraction_file.py index 5b59c352..e26d1149 100644 --- a/src/ptychodus/plugins/tiff_diffraction_file.py +++ b/src/ptychodus/plugins/tiff_diffraction_file.py @@ -91,11 +91,22 @@ def read(self, file_path: Path) -> DiffractionDataset: def register_plugins(registry: PluginRegistry) -> None: + file_reader = TiffDiffractionFileReader() registry.diffraction_file_readers.register_plugin( - TiffDiffractionFileReader(), + file_reader, simple_name='TIFF', display_name='Tagged Image File Format Files (*.tif *.tiff)', ) + registry.diffraction_file_readers.register_plugin( + file_reader, + simple_name='CNM_APS_HXN_TIFF', + display_name='CNM/APS 26-ID Hard X-ray Nanoprobe Files (*.tif *.tiff)', + ) + registry.diffraction_file_readers.register_plugin( + file_reader, + simple_name='APS_34IDC', + display_name='APS 34-ID-C Microdiffraction Files (*.tif *.tiff)', + ) if __name__ == '__main__': diff --git a/tests/test_plugins.py b/tests/test_plugins.py index c723d1ab..3308bece 100644 --- a/tests/test_plugins.py +++ b/tests/test_plugins.py @@ -13,11 +13,14 @@ from __future__ import annotations +from collections import Counter +import logging + import pytest from ptychodus.api.observer import Observable, Observer from ptychodus.api.parameters import ParameterGroup, StringParameter -from ptychodus.api.plugins import PluginChooser, PluginChooserParameter +from ptychodus.api.plugins import PluginChooser, PluginChooserParameter, PluginRegistry class _Counter(Observer): @@ -202,3 +205,60 @@ def test_two_adapters_over_one_chooser_both_track_it() -> None: assert parameter_b.get_value() == 'Zeta' assert settings_a.get_value() == 'Zeta' assert settings_b.get_value() == 'Zeta' + + +def test_register_duplicate_simple_name_warns(caplog: pytest.LogCaptureFixture) -> None: + """A shadowed registration is announced rather than failing silently. + + Settings persist the simple name and lookup returns the first match, so the + second registration under a taken name can never be selected from settings. + Registration still succeeds: plugin loading must never be fatal. + """ + chooser: PluginChooser[str] = PluginChooser() + chooser.register_plugin('h5', display_name='Example Files (*.h5)', simple_name='example') + + with caplog.at_level(logging.WARNING, logger='ptychodus.api.plugins'): + chooser.register_plugin('mat', display_name='Example Files (*.mat)', simple_name='example') + + assert 'Example Files (*.h5)' in caplog.text + assert 'Example Files (*.mat)' in caplog.text + + plugin = chooser.find_plugin('example') + assert plugin is not None + assert plugin.strategy == 'h5' + + +def test_register_distinct_simple_names_is_quiet(caplog: pytest.LogCaptureFixture) -> None: + chooser: PluginChooser[str] = PluginChooser() + chooser.register_plugin('h5', display_name='Example Files (*.h5)', simple_name='example') + + with caplog.at_level(logging.WARNING, logger='ptychodus.api.plugins'): + chooser.register_plugin( + 'mat', display_name='Example Files (*.mat)', simple_name='example_mat' + ) + + assert 'Duplicate plugin simple name' not in caplog.text + + +def test_load_plugins_has_no_duplicate_simple_names() -> None: + """Every chooser resolves each simple name to exactly one plugin. + + A duplicate makes the later registration unreachable from settings, which is + how the two fold_slice probe-position readers once shadowed each other. + """ + registry = PluginRegistry.load_plugins() + duplicates: list[str] = [] + + for attribute in dir(registry): + if attribute.startswith('_'): + continue + + chooser = getattr(registry, attribute) + + if not isinstance(chooser, PluginChooser): + continue + + counter = Counter(plugin.simple_name.casefold() for plugin in chooser) + duplicates.extend(f'{attribute}: {name}' for name, count in counter.items() if count > 1) + + assert not duplicates