From f8cb39ade8ba80a4772e881d9fecc1083035e435 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 15:38:15 +0000 Subject: [PATCH 1/4] PP-11c: split conversion.py into plan, reuse and write modules (move only) conversion.py was at 587 of 600 lines. Move plan_conversion and PlannedCopy to conversion_plan.py, the comparison with an earlier copy tree to conversion_reuse.py and the .partial writing to conversion_write.py. No behaviour changes; the private helpers used across modules lose their underscore, and the tests patch write_named_copy and os.replace where they are now used. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- AGENTS.md | 11 +- pyproject.toml | 3 + src/morphobricks_preprocessing/conversion.py | 320 +----------------- .../conversion_plan.py | 153 +++++++++ .../conversion_reuse.py | 65 ++++ .../conversion_write.py | 137 ++++++++ tests/test_convert_run.py | 11 +- 7 files changed, 382 insertions(+), 318 deletions(-) create mode 100644 src/morphobricks_preprocessing/conversion_plan.py create mode 100644 src/morphobricks_preprocessing/conversion_reuse.py create mode 100644 src/morphobricks_preprocessing/conversion_write.py diff --git a/AGENTS.md b/AGENTS.md index 2fae5804..c3618a8f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -158,10 +158,13 @@ src/morphobricks_preprocessing/ conversion_record.py the copy tree's .morphobricks-conversion.json (fingerprints, roles, sizes), review_conversion (the run's conversion_source_changed and copy_changed_after_conversion warnings) - conversion.py convert_experiment/plan_conversion: the copy tree under its lock, .partial - files moved into place together (any failure removes what it wrote), - planes checked against the memory limit first, unchanged copies reused, - record last; conversion_issues.py its messages (pure) + conversion.py convert_experiment: the copy tree under its lock, record last; + conversion_issues.py its messages (pure) + conversion_plan.py plan_conversion: headers checked with the confirmed roles, planes checked + against the memory limit first, PlannedCopy per image + conversion_reuse.py compare: unchanged copies reused, anything else there refused + conversion_write.py write_all: .partial files moved into place together (any failure + removes what it wrote) zarr_input.py OME-Zarr input: open_ngff (OME-NGFF 0.4/0.5 metadata -> level-0 array, axis letters and physical sizes from the scale and axis units), ngff_label_names/open_ngff_labels (label images, for later bricks), diff --git a/pyproject.toml b/pyproject.toml index 3415b659..f8e77cac 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -159,6 +159,9 @@ baseline = "tools/code_rules_baseline.json" "morphobricks_preprocessing.cli_convert" = "ui" "morphobricks_preprocessing.conversion" = "orchestration" "morphobricks_preprocessing.conversion_issues" = "pure" +"morphobricks_preprocessing.conversion_plan" = "orchestration" +"morphobricks_preprocessing.conversion_reuse" = "io" +"morphobricks_preprocessing.conversion_write" = "io" "morphobricks_preprocessing.conversion_record" = "io" "morphobricks_preprocessing.copy_axes" = "pure" "morphobricks_preprocessing.ome_tiff_copy" = "io" diff --git a/src/morphobricks_preprocessing/conversion.py b/src/morphobricks_preprocessing/conversion.py index ac808fee..bfe4d1af 100644 --- a/src/morphobricks_preprocessing/conversion.py +++ b/src/morphobricks_preprocessing/conversion.py @@ -31,34 +31,22 @@ import logging import os -import shutil -from collections.abc import Callable, Iterable, Mapping -from dataclasses import dataclass, field, replace +from collections.abc import Callable, Iterable +from dataclasses import dataclass, field from datetime import UTC, datetime from pathlib import Path from ._io import free_space from .axes import AxisMapping, prefill_mapping -from .axis_check import ( - AXES_NOT_NAMED, - AXIS_ERROR_CODES, - ConfirmedAxes, - confirmation_of, - header_axis_issue, -) -from .axis_review import review_axes +from .axis_check import ConfirmedAxes, confirmation_of from .conversion_issues import ( - differs_issue, - failed_issue, lock_issue, not_a_folder_issue, - not_empty_issue, - other_experiment_issue, overlap_issue, - pixel_type_issue, record_issue, space_issue, ) +from .conversion_plan import PlannedCopy, plan_conversion from .conversion_record import ( CONVERTED, COPIED, @@ -68,51 +56,15 @@ read_record, write_record, ) -from .copy_axes import CopyLayout, copy_layout -from .fingerprint import Fingerprint, current_fingerprint +from .conversion_reuse import compare +from .conversion_write import ConversionFailed, write_all from .manifest import package_versions -from .metadata import TiffMetadata -from .ome_tiff_copy import CopyWriteError, copy_nbytes, write_named_copy -from .planes import DEFAULT_MEMORY_LIMIT, PlaneReadError, check_planes -from .run.lock import LOCK_FILENAME, OutputLockedError, output_lock -from .run.preflight import preflight +from .planes import DEFAULT_MEMORY_LIMIT +from .run.lock import OutputLockedError, output_lock from .validation import ValidationIssue, has_errors logger = logging.getLogger(__name__) -_NOT_IN_OME = frozenset({"int64", "uint64"}) -"""Pixel types the brick reads that OME-TIFF cannot hold: they are never changed to fit.""" - -PARTIAL_SUFFIX = ".partial" -"""A copy is written as ``.partial`` and renamed once every copy is written.""" - - -@dataclass(frozen=True) -class PlannedCopy: - """One image of the copy tree, before it is written. - - Attributes - ---------- - entry - Its conversion-record entry, without the copy's fingerprint. - layout - The named-axis copy of a converted image, else None. - mapping - The roles a converted image is read with, else None. - nbytes - The bytes the copy takes (pixels only for a converted image). - """ - - entry: CopyEntry - layout: CopyLayout | None = None - mapping: AxisMapping | None = None - nbytes: int = 0 - - @property - def relative_path(self) -> str: - """The image, relative to the experiment and to the copy tree.""" - return self.entry.relative_path - @dataclass(frozen=True) class ConversionResult: @@ -232,99 +184,6 @@ class _Options: memory_limit: int -def plan_conversion( - root: Path, - mapping: AxisMapping | ConfirmedAxes, - destination: Path, - exclude: Iterable[str] = (), - missing_as_singleton: bool = False, - memory_limit: int = DEFAULT_MEMORY_LIMIT, -) -> tuple[tuple[PlannedCopy, ...], list[ValidationIssue]]: - """Check the experiment with the confirmed roles and plan each image's copy. - - Only the headers are read. The axis errors do not stop the conversion: they are what - it corrects. Any other error does, and nothing is planned: so does an image to convert - of a pixel type OME-TIFF cannot hold, or whose planes cannot be read within the memory - limit, not even in blocks (checked as the run checks them, before anything is written). - - Parameters - ---------- - root - The experiment folder (absolute). - mapping - The confirmed mapping. - destination - The copy tree (absolute). - exclude - Images to leave unread. - missing_as_singleton - As :func:`convert_experiment`. - memory_limit - As :func:`convert_experiment`. - - Returns - ------- - tuple - The planned copies, sorted by path (empty when an error stops the conversion), - and the issues: the errors that stop it, and the warnings. - """ - pre = preflight(root, destination, exclude=exclude) - review = review_axes(pre.metadata, mapping, missing_as_singleton=missing_as_singleton) - others = [issue for issue in pre.issues if issue not in pre.metadata.issues] - issues = [*others, *review.combined_issues(pre.metadata)] - if any(i.is_error and i.code not in AXIS_ERROR_CODES for i in issues): - return (), [i for i in issues if not (i.is_error and i.code in AXIS_ERROR_CODES)] - # The axis errors and unnamed axes are what is corrected (each entry records its code). - issues = [i for i in issues if not i.is_error and i.code != AXES_NOT_NAMED] - to_convert = set(review.axis_error_files) | set(review.confirmed_files) - headers = {meta.relative_path: meta for meta in pre.metadata.files} - issues += _cannot_convert(root, to_convert, headers, review.file_mappings, memory_limit) - if has_errors(issues): - return (), issues - planned = [] - for rel in pre.discovery.images: - source = Fingerprint.of(root / rel) - if rel in to_convert: - planned.append(_converted(headers[rel], review.file_mappings[rel], source)) - else: - reason = "excluded" if rel in pre.excluded else None - meta = headers.get(rel) - entry = CopyEntry(rel, COPIED, reason, source, source_axes=meta.axes if meta else "") - planned.append(PlannedCopy(entry, nbytes=source.size)) - return tuple(planned), issues - - -def _cannot_convert( - root: Path, - to_convert: set[str], - headers: dict[str, TiffMetadata], - mappings: Mapping[str, AxisMapping], - memory_limit: int, -) -> list[ValidationIssue]: - """Return why images cannot be converted (errors) and the planes read in blocks.""" - wide = sorted(rel for rel in to_convert if headers[rel].dtype.name in _NOT_IN_OME) - if wide: - return [pixel_type_issue(wide, headers[wide[0]].dtype.name)] - return check_planes(root, {rel: mappings[rel] for rel in to_convert}, memory_limit=memory_limit) - - -def _converted(meta: TiffMetadata, mapping: AxisMapping, source: Fingerprint) -> PlannedCopy: - """Plan the named-axis copy of one image read under its confirmed roles.""" - layout = copy_layout(meta, mapping) - entry = CopyEntry( - relative_path=meta.relative_path, - action=CONVERTED, - reason=header_axis_issue(meta, mapping), - source=source, - source_axes=meta.axes, - roles=mapping.roles, - copy_axes=layout.axes, - pixel_sizes=layout.sizes, - unknown_sizes=layout.unknown, - ) - return PlannedCopy(entry, layout, mapping, copy_nbytes(layout, meta.dtype)) - - def _locked_conversion( root: Path, dest: Path, @@ -338,13 +197,13 @@ def _locked_conversion( earlier = read_record(dest) except ConversionRecordError as exc: return ConversionResult(dest, False, (record_issue(exc, dest),)) - reused, to_write, refusal = _compare(root, dest, planned, earlier) + reused, to_write, refusal = compare(root, dest, planned, earlier) refusal = refusal or _space_issue(dest, to_write) if refusal is not None: return ConversionResult(dest, False, (refusal, *issues)) try: - written = _write_all(root, dest, to_write, options.memory_limit, progress) - except _ConversionFailed as failed: + written = write_all(root, dest, to_write, options.memory_limit, progress) + except ConversionFailed as failed: return ConversionResult(dest, False, (failed.issue, *issues)) files = sorted([*reused, *written], key=lambda entry: entry.relative_path) sha256 = _publish_record(root, dest, files, options, earlier) @@ -393,163 +252,6 @@ def _publish_record( return write_record(record, dest) -def _compare( - root: Path, - dest: Path, - planned: tuple[PlannedCopy, ...], - earlier: tuple[ConversionRecord, str] | None, -) -> tuple[list[CopyEntry], list[PlannedCopy], ValidationIssue | None]: - """Split the plan into copies to reuse and to write; refuse anything else there.""" - if earlier is None: - unknown = _unknown_content(dest, planned) - return [], list(planned), not_empty_issue(dest, unknown) if unknown else None - record = earlier[0] - if record.source_root != root.as_posix(): - return [], [], other_experiment_issue(dest, record.source_root) - reused, to_write, differing = [], [], [] - for item in planned: - old = record.entry(item.relative_path) - target = dest / item.relative_path - if old is None: - (differing if os.path.lexists(target) else to_write).append(item) - elif old.same_plan(item.entry) and current_fingerprint(target) == old.copy: - reused.append(old) - else: - differing.append(item) - paths = [item.relative_path for item in differing] - names = {item.relative_path for item in planned} - paths += [e.relative_path for e in record.files if e.relative_path not in names] - return reused, to_write, differs_issue(dest, sorted(paths)) if paths else None - - -def _unknown_content(dest: Path, planned: tuple[PlannedCopy, ...]) -> list[str]: - """Return the files in a folder without a record that this conversion did not leave. - - Only the lock and the ``.partial`` files of the planned copies (left by an interrupted - conversion, removed before they are written again) are allowed. - """ - partials = {item.relative_path + PARTIAL_SUFFIX for item in planned} - found = [] - for folder, subfolders, names in os.walk(dest): - relative = Path(folder).relative_to(dest) - subfolders[:] = [d for d in subfolders if (relative / d).as_posix() not in partials] - for name in names: - path = (relative / name).as_posix() - if path != LOCK_FILENAME and path not in partials: - found.append(path) - return sorted(found) - - -class _ConversionFailed(Exception): - """A copy could not be written; carries the error for the user.""" - - def __init__(self, issue: ValidationIssue) -> None: - super().__init__(issue.message) - self.issue = issue - - -_FAILURES = (PlaneReadError, CopyWriteError, OSError, ValueError) -"""What reading a source or writing a copy raises when it fails (``tifffile`` raises -:class:`ValueError` for a file it cannot write); anything else is re-raised, after the same -clean-up.""" - - -def _write_all( - root: Path, - dest: Path, - to_write: list[PlannedCopy], - memory_limit: int, - progress: Callable[[int, int, str], None] | None, -) -> list[CopyEntry]: - """Write every copy as ``.partial``, then move them all into place. - - On any failure, an interruption included, every ``.partial`` file of the conversion, - every copy already moved into place (each where nothing was before) and every folder - the conversion made are removed, so the copy tree is left as it was; a failure to read - or write raises :class:`_ConversionFailed`, anything else is re-raised. - """ - made = _missing_folders(dest, to_write) - written: list[Path] = [] - current = to_write[0].relative_path if to_write else "" - try: - for done, item in enumerate(to_write): - current = item.relative_path - if progress is not None: - progress(done, len(to_write), current) - _write_partial(root, dest, item, memory_limit) - if progress is not None: - progress(len(to_write), len(to_write), "") - entries = [] - for item in to_write: - current, target = item.relative_path, dest / item.relative_path - os.replace(_partial(dest, item), target) - written.append(target) - entries.append(replace(item.entry, copy=Fingerprint.of(target))) - except BaseException as exc: - logger.debug("Conversion of %s failed", current, exc_info=True) - for path in [*written, *(_partial(dest, item) for item in to_write)]: - _remove(path) - _remove_empty(made) - if isinstance(exc, (*_FAILURES, _SourceChanged)): - raise _ConversionFailed(failed_issue(current, exc)) from exc - raise - return entries - - -def _missing_folders(dest: Path, to_write: list[PlannedCopy]) -> list[Path]: - """Return the folders of the copy tree that writing these copies will make.""" - missing = set() - for item in to_write: - folder = (dest / item.relative_path).parent - while folder != dest and not folder.exists(): - missing.add(folder) - folder = folder.parent - return sorted(missing, key=lambda folder: len(folder.parts), reverse=True) - - -def _remove_empty(folders: list[Path]) -> None: - """Remove the folders this conversion made, deepest first, when they are empty.""" - for folder in folders: - try: - folder.rmdir() - except OSError: - logger.debug("Folder %s kept: not empty or gone", folder, exc_info=True) - - -class _SourceChanged(Exception): - """A source image changed since it was checked, or while it was copied.""" - - -def _write_partial(root: Path, dest: Path, item: PlannedCopy, memory_limit: int) -> None: - """Write one copy as ``.partial``, refusing a source that changes meanwhile.""" - source, partial = root / item.relative_path, _partial(dest, item) - partial.parent.mkdir(parents=True, exist_ok=True) - _remove(partial) # left by an interrupted conversion: it is never reused - if current_fingerprint(source) != item.entry.source: - raise _SourceChanged("it changed since it was checked") - if item.layout is not None: - write_named_copy(source, partial, item.layout, item.mapping, memory_limit=memory_limit) - elif source.is_dir(): - shutil.copytree(source, partial, copy_function=shutil.copyfile) - else: - shutil.copyfile(source, partial) - if current_fingerprint(source) != item.entry.source: - raise _SourceChanged("it changed while it was copied") - - -def _partial(dest: Path, item: PlannedCopy) -> Path: - """Return the ``.partial`` path of a copy.""" - return dest / (item.relative_path + PARTIAL_SUFFIX) - - -def _remove(path: Path) -> None: - """Remove a file or folder this conversion wrote (or left earlier) if it is there.""" - if path.is_dir() and not path.is_symlink(): - shutil.rmtree(path) - else: - path.unlink(missing_ok=True) - - def _suggested_axes(files: list[CopyEntry]) -> str | None: """Return the roles for ``run --axes`` on the copies when they all have the same axes.""" axes = { diff --git a/src/morphobricks_preprocessing/conversion_plan.py b/src/morphobricks_preprocessing/conversion_plan.py new file mode 100644 index 00000000..2cdf195d --- /dev/null +++ b/src/morphobricks_preprocessing/conversion_plan.py @@ -0,0 +1,153 @@ +"""Plan a conversion: check the experiment and decide each image's copy (design v0.4 §5). + +Moved from :mod:`.conversion` (PP-11c). :func:`plan_conversion` reads only the headers: the +axis errors and unnamed axes are what the conversion corrects; any other error stops it +before anything is written. +""" + +from __future__ import annotations + +from collections.abc import Iterable, Mapping +from dataclasses import dataclass +from pathlib import Path + +from .axes import AxisMapping +from .axis_check import ( + AXES_NOT_NAMED, + AXIS_ERROR_CODES, + ConfirmedAxes, + header_axis_issue, +) +from .axis_review import review_axes +from .conversion_issues import pixel_type_issue +from .conversion_record import CONVERTED, COPIED, CopyEntry +from .copy_axes import CopyLayout, copy_layout +from .fingerprint import Fingerprint +from .metadata import TiffMetadata +from .ome_tiff_copy import copy_nbytes +from .planes import DEFAULT_MEMORY_LIMIT, check_planes +from .run.preflight import preflight +from .validation import ValidationIssue, has_errors + +_NOT_IN_OME = frozenset({"int64", "uint64"}) +"""Pixel types the brick reads that OME-TIFF cannot hold: they are never changed to fit.""" + + +@dataclass(frozen=True) +class PlannedCopy: + """One image of the copy tree, before it is written. + + Attributes + ---------- + entry + Its conversion-record entry, without the copy's fingerprint. + layout + The named-axis copy of a converted image, else None. + mapping + The roles a converted image is read with, else None. + nbytes + The bytes the copy takes (pixels only for a converted image). + """ + + entry: CopyEntry + layout: CopyLayout | None = None + mapping: AxisMapping | None = None + nbytes: int = 0 + + @property + def relative_path(self) -> str: + """The image, relative to the experiment and to the copy tree.""" + return self.entry.relative_path + + +def plan_conversion( + root: Path, + mapping: AxisMapping | ConfirmedAxes, + destination: Path, + exclude: Iterable[str] = (), + missing_as_singleton: bool = False, + memory_limit: int = DEFAULT_MEMORY_LIMIT, +) -> tuple[tuple[PlannedCopy, ...], list[ValidationIssue]]: + """Check the experiment with the confirmed roles and plan each image's copy. + + Only the headers are read. The axis errors do not stop the conversion: they are what + it corrects. Any other error does, and nothing is planned: so does an image to convert + of a pixel type OME-TIFF cannot hold, or whose planes cannot be read within the memory + limit, not even in blocks (checked as the run checks them, before anything is written). + + Parameters + ---------- + root + The experiment folder (absolute). + mapping + The confirmed mapping. + destination + The copy tree (absolute). + exclude + Images to leave unread. + missing_as_singleton + As :func:`convert_experiment`. + memory_limit + As :func:`convert_experiment`. + + Returns + ------- + tuple + The planned copies, sorted by path (empty when an error stops the conversion), + and the issues: the errors that stop it, and the warnings. + """ + pre = preflight(root, destination, exclude=exclude) + review = review_axes(pre.metadata, mapping, missing_as_singleton=missing_as_singleton) + others = [issue for issue in pre.issues if issue not in pre.metadata.issues] + issues = [*others, *review.combined_issues(pre.metadata)] + if any(i.is_error and i.code not in AXIS_ERROR_CODES for i in issues): + return (), [i for i in issues if not (i.is_error and i.code in AXIS_ERROR_CODES)] + # The axis errors and unnamed axes are what is corrected (each entry records its code). + issues = [i for i in issues if not i.is_error and i.code != AXES_NOT_NAMED] + to_convert = set(review.axis_error_files) | set(review.confirmed_files) + headers = {meta.relative_path: meta for meta in pre.metadata.files} + issues += _cannot_convert(root, to_convert, headers, review.file_mappings, memory_limit) + if has_errors(issues): + return (), issues + planned = [] + for rel in pre.discovery.images: + source = Fingerprint.of(root / rel) + if rel in to_convert: + planned.append(_converted(headers[rel], review.file_mappings[rel], source)) + else: + reason = "excluded" if rel in pre.excluded else None + meta = headers.get(rel) + entry = CopyEntry(rel, COPIED, reason, source, source_axes=meta.axes if meta else "") + planned.append(PlannedCopy(entry, nbytes=source.size)) + return tuple(planned), issues + + +def _cannot_convert( + root: Path, + to_convert: set[str], + headers: dict[str, TiffMetadata], + mappings: Mapping[str, AxisMapping], + memory_limit: int, +) -> list[ValidationIssue]: + """Return why images cannot be converted (errors) and the planes read in blocks.""" + wide = sorted(rel for rel in to_convert if headers[rel].dtype.name in _NOT_IN_OME) + if wide: + return [pixel_type_issue(wide, headers[wide[0]].dtype.name)] + return check_planes(root, {rel: mappings[rel] for rel in to_convert}, memory_limit=memory_limit) + + +def _converted(meta: TiffMetadata, mapping: AxisMapping, source: Fingerprint) -> PlannedCopy: + """Plan the named-axis copy of one image read under its confirmed roles.""" + layout = copy_layout(meta, mapping) + entry = CopyEntry( + relative_path=meta.relative_path, + action=CONVERTED, + reason=header_axis_issue(meta, mapping), + source=source, + source_axes=meta.axes, + roles=mapping.roles, + copy_axes=layout.axes, + pixel_sizes=layout.sizes, + unknown_sizes=layout.unknown, + ) + return PlannedCopy(entry, layout, mapping, copy_nbytes(layout, meta.dtype)) diff --git a/src/morphobricks_preprocessing/conversion_reuse.py b/src/morphobricks_preprocessing/conversion_reuse.py new file mode 100644 index 00000000..efbdad1b --- /dev/null +++ b/src/morphobricks_preprocessing/conversion_reuse.py @@ -0,0 +1,65 @@ +"""Compare a planned conversion with what the copy tree already holds. + +Moved from :mod:`.conversion` (PP-11c). A copy is reused only when its plan and its copy +are unchanged; anything else already there is refused, never overwritten. +""" + +from __future__ import annotations + +import os +from pathlib import Path + +from .conversion_issues import differs_issue, not_empty_issue, other_experiment_issue +from .conversion_plan import PlannedCopy +from .conversion_record import ConversionRecord, CopyEntry +from .conversion_write import PARTIAL_SUFFIX +from .fingerprint import current_fingerprint +from .run.lock import LOCK_FILENAME +from .validation import ValidationIssue + + +def compare( + root: Path, + dest: Path, + planned: tuple[PlannedCopy, ...], + earlier: tuple[ConversionRecord, str] | None, +) -> tuple[list[CopyEntry], list[PlannedCopy], ValidationIssue | None]: + """Split the plan into copies to reuse and to write; refuse anything else there.""" + if earlier is None: + unknown = unknown_content(dest, planned) + return [], list(planned), not_empty_issue(dest, unknown) if unknown else None + record = earlier[0] + if record.source_root != root.as_posix(): + return [], [], other_experiment_issue(dest, record.source_root) + reused, to_write, differing = [], [], [] + for item in planned: + old = record.entry(item.relative_path) + target = dest / item.relative_path + if old is None: + (differing if os.path.lexists(target) else to_write).append(item) + elif old.same_plan(item.entry) and current_fingerprint(target) == old.copy: + reused.append(old) + else: + differing.append(item) + paths = [item.relative_path for item in differing] + names = {item.relative_path for item in planned} + paths += [e.relative_path for e in record.files if e.relative_path not in names] + return reused, to_write, differs_issue(dest, sorted(paths)) if paths else None + + +def unknown_content(dest: Path, planned: tuple[PlannedCopy, ...]) -> list[str]: + """Return the files in a folder without a record that this conversion did not leave. + + Only the lock and the ``.partial`` files of the planned copies (left by an interrupted + conversion, removed before they are written again) are allowed. + """ + partials = {item.relative_path + PARTIAL_SUFFIX for item in planned} + found = [] + for folder, subfolders, names in os.walk(dest): + relative = Path(folder).relative_to(dest) + subfolders[:] = [d for d in subfolders if (relative / d).as_posix() not in partials] + for name in names: + path = (relative / name).as_posix() + if path != LOCK_FILENAME and path not in partials: + found.append(path) + return sorted(found) diff --git a/src/morphobricks_preprocessing/conversion_write.py b/src/morphobricks_preprocessing/conversion_write.py new file mode 100644 index 00000000..90d1fa31 --- /dev/null +++ b/src/morphobricks_preprocessing/conversion_write.py @@ -0,0 +1,137 @@ +"""Write the copies of a conversion as ``.partial`` files, then move them into place. + +Moved from :mod:`.conversion` (PP-11c). On any failure, every file and folder the +conversion wrote is removed, so the copy tree is left as it was. +""" + +from __future__ import annotations + +import logging +import os +import shutil +from collections.abc import Callable +from dataclasses import replace +from pathlib import Path + +from .conversion_issues import failed_issue +from .conversion_plan import PlannedCopy +from .conversion_record import CopyEntry +from .fingerprint import Fingerprint, current_fingerprint +from .ome_tiff_copy import CopyWriteError, write_named_copy +from .planes import PlaneReadError +from .validation import ValidationIssue + +logger = logging.getLogger(__name__) + +PARTIAL_SUFFIX = ".partial" +"""A copy is written as ``.partial`` and renamed once every copy is written.""" + + +class ConversionFailed(Exception): + """A copy could not be written; carries the error for the user.""" + + def __init__(self, issue: ValidationIssue) -> None: + super().__init__(issue.message) + self.issue = issue + + +_FAILURES = (PlaneReadError, CopyWriteError, OSError, ValueError) +"""What reading a source or writing a copy raises when it fails (``tifffile`` raises +:class:`ValueError` for a file it cannot write); anything else is re-raised, after the same +clean-up.""" + + +def write_all( + root: Path, + dest: Path, + to_write: list[PlannedCopy], + memory_limit: int, + progress: Callable[[int, int, str], None] | None, +) -> list[CopyEntry]: + """Write every copy as ``.partial``, then move them all into place. + + On any failure, an interruption included, every ``.partial`` file of the conversion, + every copy already moved into place (each where nothing was before) and every folder + the conversion made are removed, so the copy tree is left as it was; a failure to read + or write raises :class:`ConversionFailed`, anything else is re-raised. + """ + made = _missing_folders(dest, to_write) + written: list[Path] = [] + current = to_write[0].relative_path if to_write else "" + try: + for done, item in enumerate(to_write): + current = item.relative_path + if progress is not None: + progress(done, len(to_write), current) + _write_partial(root, dest, item, memory_limit) + if progress is not None: + progress(len(to_write), len(to_write), "") + entries = [] + for item in to_write: + current, target = item.relative_path, dest / item.relative_path + os.replace(partial_path(dest, item), target) + written.append(target) + entries.append(replace(item.entry, copy=Fingerprint.of(target))) + except BaseException as exc: + logger.debug("Conversion of %s failed", current, exc_info=True) + for path in [*written, *(partial_path(dest, item) for item in to_write)]: + _remove(path) + _remove_empty(made) + if isinstance(exc, (*_FAILURES, _SourceChanged)): + raise ConversionFailed(failed_issue(current, exc)) from exc + raise + return entries + + +def _missing_folders(dest: Path, to_write: list[PlannedCopy]) -> list[Path]: + """Return the folders of the copy tree that writing these copies will make.""" + missing = set() + for item in to_write: + folder = (dest / item.relative_path).parent + while folder != dest and not folder.exists(): + missing.add(folder) + folder = folder.parent + return sorted(missing, key=lambda folder: len(folder.parts), reverse=True) + + +def _remove_empty(folders: list[Path]) -> None: + """Remove the folders this conversion made, deepest first, when they are empty.""" + for folder in folders: + try: + folder.rmdir() + except OSError: + logger.debug("Folder %s kept: not empty or gone", folder, exc_info=True) + + +class _SourceChanged(Exception): + """A source image changed since it was checked, or while it was copied.""" + + +def _write_partial(root: Path, dest: Path, item: PlannedCopy, memory_limit: int) -> None: + """Write one copy as ``.partial``, refusing a source that changes meanwhile.""" + source, partial = root / item.relative_path, partial_path(dest, item) + partial.parent.mkdir(parents=True, exist_ok=True) + _remove(partial) # left by an interrupted conversion: it is never reused + if current_fingerprint(source) != item.entry.source: + raise _SourceChanged("it changed since it was checked") + if item.layout is not None: + write_named_copy(source, partial, item.layout, item.mapping, memory_limit=memory_limit) + elif source.is_dir(): + shutil.copytree(source, partial, copy_function=shutil.copyfile) + else: + shutil.copyfile(source, partial) + if current_fingerprint(source) != item.entry.source: + raise _SourceChanged("it changed while it was copied") + + +def partial_path(dest: Path, item: PlannedCopy) -> Path: + """Return the ``.partial`` path of a copy.""" + return dest / (item.relative_path + PARTIAL_SUFFIX) + + +def _remove(path: Path) -> None: + """Remove a file or folder this conversion wrote (or left earlier) if it is there.""" + if path.is_dir() and not path.is_symlink(): + shutil.rmtree(path) + else: + path.unlink(missing_ok=True) diff --git a/tests/test_convert_run.py b/tests/test_convert_run.py index 6f65586a..fcceb679 100644 --- a/tests/test_convert_run.py +++ b/tests/test_convert_run.py @@ -15,6 +15,7 @@ import tifffile import morphobricks_preprocessing.conversion as conversion_module +import morphobricks_preprocessing.conversion_write as write_module from morphobricks_preprocessing import ( CONVERSION_RECORD_FILENAME, AxisMapping, @@ -151,7 +152,7 @@ def test_a_folder_holding_other_files_or_another_experiments_copies_is_refused(t def test_a_failed_conversion_leaves_the_folder_as_it_was(tmp_path, monkeypatch): exp, copies = swapped_experiment(tmp_path / "exp"), tmp_path / "copies" - original, calls = conversion_module.write_named_copy, [] + original, calls = write_module.write_named_copy, [] def failing(source, target, *args, **kwargs): calls.append(target) @@ -160,7 +161,7 @@ def failing(source, target, *args, **kwargs): raise OSError("disk full") return original(source, target, *args, **kwargs) - monkeypatch.setattr(conversion_module, "write_named_copy", failing) + monkeypatch.setattr(write_module, "write_named_copy", failing) result = convert_experiment(exp, SWAPPED, copies) assert not result.written and codes(result.issues) == ["conversion_failed"] assert result.issues[0].paths == ("ctrl/r1/b.tif",) and "disk full" in result.issues[0].message @@ -179,7 +180,7 @@ def test_any_failure_or_interruption_removes_the_partial_files_and_new_folders( # Review of PP-11: an error other than OSError (tifffile raises ValueError) or an # interruption left a .partial file and the folders behind. exp, copies = swapped_experiment(tmp_path / "exp"), tmp_path / "copies" - original, calls = conversion_module.write_named_copy, [] + original, calls = write_module.write_named_copy, [] def failing(source, target, *args, **kwargs): calls.append(target) @@ -188,7 +189,7 @@ def failing(source, target, *args, **kwargs): raise error return original(source, target, *args, **kwargs) - monkeypatch.setattr(conversion_module, "write_named_copy", failing) + monkeypatch.setattr(write_module, "write_named_copy", failing) if reported: result = convert_experiment(exp, SWAPPED, copies) assert not result.written and codes(result.issues) == ["conversion_failed"] @@ -209,7 +210,7 @@ def failing(source, target): raise OSError("device removed") return original(source, target) - monkeypatch.setattr(conversion_module.os, "replace", failing) + monkeypatch.setattr(write_module.os, "replace", failing) result = convert_experiment(exp, SWAPPED, copies) monkeypatch.undo() assert not result.written and codes(result.issues) == ["conversion_failed"] From 4312b5a73e71931d725ad9c151b79c4e56c63ebb Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 15:51:39 +0000 Subject: [PATCH 2/4] PP-11c: conversion integrity (copy SHA-256, one metadata epoch, exact sizes) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - #96: plan_conversion fingerprints each image before and after its header is read (preflight(bind_headers=True)) and refuses an image whose two fingerprints differ (source_changed_during_check, nothing written); that fingerprint is the source's in the record and is checked before and after the copy is written. The copy's pixel type comes from the same header (write_named_copy no longer reopens it) and a plane of another type is refused, never cast. - #87: the conversion record (version 2) gives each copy's SHA-256 (content_hash, one pass after writing); reuse, review_conversion and RunInputs.open (conversion_check, once at open for the recorded images) verify it. A version 1 record is refused with a message to convert again. - #88: CopyEntry records the copy's channel names and significant bits and same_plan() compares them. - #92: same_plan() compares the unknown sizes' reasons, not only their roles. - #80: parse_size is exact (Fraction): a size that is not a whole number of bytes above 0 ("1.5", "0.5B") is refused while the command line is read. Docs: contract §9.1 and §17, plan question 59 (16)-(18) and §10, user guide, AGENTS.md module map. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- AGENTS.md | 13 +- docs/contract.md | 18 +- docs/plan.md | 30 ++ docs/user_guide.md | 25 +- pyproject.toml | 2 + src/morphobricks_preprocessing/cli_run.py | 22 +- .../content_hash.py | 85 ++++++ .../conversion_check.py | 67 +++++ .../conversion_issues.py | 14 +- .../conversion_plan.py | 56 +++- .../conversion_record.py | 111 +++++-- .../conversion_reuse.py | 15 +- .../conversion_write.py | 8 +- .../ome_tiff_copy.py | 14 +- .../run/preflight.py | 33 +- src/morphobricks_preprocessing/run_inputs.py | 28 +- tests/test_convert_integrity.py | 281 ++++++++++++++++++ 17 files changed, 746 insertions(+), 76 deletions(-) create mode 100644 src/morphobricks_preprocessing/content_hash.py create mode 100644 src/morphobricks_preprocessing/conversion_check.py create mode 100644 tests/test_convert_integrity.py diff --git a/AGENTS.md b/AGENTS.md index c3618a8f..dd0aeddf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -155,12 +155,17 @@ src/morphobricks_preprocessing/ swap, question 59) and OME metadata of a named-axis copy ome_tiff_copy.py write_named_copy: one image as uncompressed OME-TIFF, planes from iter_planes under the confirmed roles, blocks written in place (BigTIFF when large) - conversion_record.py the copy tree's .morphobricks-conversion.json (fingerprints, roles, sizes), - review_conversion (the run's conversion_source_changed and - copy_changed_after_conversion warnings) + conversion_record.py the copy tree's .morphobricks-conversion.json (version 2: fingerprints, + copy SHA-256, roles, sizes, channel names, bits), review_conversion (the + run's conversion_source_changed and copy_changed_after_conversion + warnings), changed_copies + content_hash.py content_sha256: a copy's SHA-256 (file, or OME-Zarr folder without labels) + conversion_check.py conversion_problem: RunInputs.open's check of a copy tree (record SHA-256, + version, each recorded copy's SHA-256, read once at open) conversion.py convert_experiment: the copy tree under its lock, record last; conversion_issues.py its messages (pure) - conversion_plan.py plan_conversion: headers checked with the confirmed roles, planes checked + conversion_plan.py plan_conversion: headers checked with the confirmed roles, each bound to + its image's fingerprint (source_changed_during_check), planes checked against the memory limit first, PlannedCopy per image conversion_reuse.py compare: unchanged copies reused, anything else there refused conversion_write.py write_all: .partial files moved into place together (any failure diff --git a/docs/contract.md b/docs/contract.md index 6284a5c4..989f2824 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -195,10 +195,16 @@ masks_root=None)` (design v0.4 §6). It refuses (`RunInputsError`) a version 1 m `plane_histograms.csv`, `experiment_design.csv` or `calibration.csv` whose SHA-256 is not the manifest's (also a `calibration.csv` added since the run), the conversion record of a run on named-axis copies whose SHA-256 is not the manifest's `conversion_record_sha256` (changed -or gone since the run; §17), an image or mask changed or gone since the run, and a mask recorded without axes (a v0.4.0rc1 run wrote `axes: ""` for a +or gone since the run; §17), a conversion record of version 1 (no copy SHA-256: convert +again), a copy of a recorded image whose fingerprint or SHA-256 is not the record's (or that +the record does not list; PP-11c, issue #87), an image or mask changed or gone since the run, and a mask recorded without axes (a v0.4.0rc1 run wrote `axes: ""` for a mask whose header puts X before Y; the message says to save the mask again with `numpy.swapaxes` and run again, never "changed or missing"); every read checks the file's -fingerprint again. +fingerprint again. The copies' SHA-256 is checked once, when the run is opened, for every +image the run recorded (`conversion_check`): each copy is read whole then, so opening a +run on named-axis copies costs one read of those copies (the excluded images' copies are +not read). Checking on every read instead would read a copy whole for each of its planes. +Reads after `open` check the fingerprint, as for any image. It also refuses an experiment holding files the run neither recorded nor excluded (`UnrecordedFilesError`, a `RunInputsError`, PP-8b): images that preprocessing's own @@ -508,12 +514,12 @@ exclude=, missing_as_singleton=, memory_limit=, progress=)` (`conversion`), the | Rule | Detail | |---|---| | What is converted | Every image with an axis error (`axes_yx_swapped`, `axes_contradict_header`, `axes_x_before_y`, §2) or unnamed axes (`axes_not_named`) under the mapping given; it is never taken from the headers. Every other image is copied byte for byte: its header names its axes as confirmed, it is an OME-Zarr image (with its label images), or it is excluded (`exclude`: never read). So the copy tree is the whole experiment. The command and the API are offered for any experiment; an experiment without these images is copied as it is. | -| Checks first | The experiment is checked with the mapping as a run would check it (folders, headers, `review_axes`), from the headers only. The axis errors are what the conversion corrects; any other error stops it before anything is written, and so do images of a type OME-TIFF cannot hold (`cannot_convert_pixel_type`: `int64`, `uint64`; pixel values are never changed to fit) and images to convert whose planes cannot be read within the memory limit, not even in blocks (`check_planes`: `plane_over_memory_limit`, `unsupported_layout`; `planes_read_in_blocks` is a warning). `memory_limit` must be a whole number of bytes above 0 and `missing_as_singleton` True or False (`ValueError` before anything is read). The axis errors and `axes_not_named` are not reported: each converted image's record entry gives its code. | +| Checks first | The experiment is checked with the mapping as a run would check it (folders, headers, `review_axes`), from the headers only. The axis errors are what the conversion corrects; any other error stops it before anything is written, and so do images of a type OME-TIFF cannot hold (`cannot_convert_pixel_type`: `int64`, `uint64`; pixel values are never changed to fit) and images to convert whose planes cannot be read within the memory limit, not even in blocks (`check_planes`: `plane_over_memory_limit`, `unsupported_layout`; `planes_read_in_blocks` is a warning). `memory_limit` must be a whole number of bytes above 0 and `missing_as_singleton` True or False (`ValueError` before anything is read); the CLI's `--memory-limit` (`run`, `convert`) is parsed exactly (`cli_run.parse_size`, no floating point): a size that is not a whole number of bytes above 0 (`1.5`, `0.5B`) is refused while the command line is read, before anything is read (`1.5GiB` is 1610612736 bytes; issue #80). One metadata epoch (issue #96): each image is fingerprinted before and after its header is read (`preflight(bind_headers=True)`); an image whose two fingerprints differ (or that went missing) is `source_changed_during_check` and nothing is written. The copy's axes, sizes, channel names, significant bits and pixel type come from that header (the header is not read again for them), and the fingerprint taken with it is the one recorded as the source's and checked before and after the copy is written. A plane the reader gives in another pixel type than that header's is refused (`conversion_failed`), never cast. The axis errors and `axes_not_named` are not reported: each converted image's record entry gives its code. | | Copy tree | A folder the user names (`--to`), outside the experiment and not holding it (`copy_folder_overlaps_experiment`; a file is `copy_folder_not_a_folder`): new or empty (else `copy_folder_not_empty`, nothing overwritten), or the copy tree of an earlier conversion of the same experiment. The originals are only read. Each image keeps its relative path and file name (an OME-TIFF may end in `.tif`), so `source_relative_path`, `plane_id` and the FOV key are the originals'. | | Copies | Uncompressed OME-TIFF, BigTIFF when the pixels and 1 KiB per page (for its directory) would take more than 4 GiB less 32 MiB. Axes: every axis with a role in the source's order, then Y and X (`ZCYX`); an axis without a role (length 1) or missing (`missing_as_singleton`) is not added. Each plane is the plane the run reads under the confirmed roles (`iter_planes`), bit for bit: same pixel type (in native byte order), no rescaling, NaN and infinities kept; a Y/X swap and X before Y are written rows (Y) then columns (X). Planes over the memory limit are read in blocks, as the run reads them, and each block is written to its place: no plane is held whole when the run would not hold it. The copy's metadata names every axis, so `check` and `run` read it with `axes_from: "header"`. | | Pixel sizes | They follow the roles; a relabelled axis gets an unknown scale, never a guessed one (`copy_axes`). A size the source header gives is written only for the role the header names it for, and only when the user confirmed that role for that same axis; otherwise it is left out and the record says why. One exception (owner decision of 2 November 2026, plan §12 question 59): a Y or X relabelled as the header's other one of Y and X (a Y/X swap, X before Y) keeps the header's size when the header gives X and Y exactly equal, same value and unit, since nothing can be exchanged; otherwise both are unknown in the copy (state them in `calibration.csv`, §9.3, if known). A Y/X swap therefore never exchanges the sizes of Y and X. A z-step is never written as a time step. A plain stack's Y and X confirmed Y before X keep the header's sizes. Units are written as OME symbols (`µm`, `s`, ...). Channel names are written when the C axis keeps its role and every channel is named; significant bits as they are. | | Safe | The copy tree's lock (`.morphobricks-preprocessing.lock`, §9.2's rule; `--remove-stale-lock`) is held throughout; a second conversion is `output_locked`. Each copy is written as `.partial`, the source's fingerprint checked before and after; once every copy is written they are moved into place, then the record is written (atomic, with the file-mode rule of §9). `not_enough_space` is refused before writing. Any failure or interruption removes the `.partial` files of the conversion, the copies already moved into place (each where nothing was) and the folders it made, so the folder is left as it was; a read or write failure is `conversion_failed`, anything else is raised again; `.partial` files of an interrupted conversion are written again. New files get the mode a plain `open()` gives. | -| Repeat | A copy already in the tree is reused when its source (fingerprint and axes), roles, copy axes, pixel sizes (and unknown ones) and the copy's fingerprint are as recorded; a new image is added; when every copy is reused the record is kept as it was (same SHA-256). Anything else (a source changed, other roles, a copy changed, an image no longer in the experiment, another experiment's copy tree, a file at a new image's place that the record does not list) is `copy_differs`: nothing is overwritten. A conversion record that cannot be read is `conversion_record_unreadable`. | -| Conversion record | `.morphobricks-conversion.json` in the copy tree's root (a dot-file, so discovery skips it), record version 1: `source_root`, `mapping`, `missing_as_singleton`, `written_at`, `versions`, and per image `relative_path`, `action` (`converted` or `copied`), `reason` (the axis code, `excluded`, or null), `source` and `copy` fingerprints (`size`, `mtime_ns`), `source_axes`, `roles`, `copy_axes`, `pixel_sizes` (by role, as §9) and `unknown_sizes` (role: why). `read_conversion_record(folder)` reads it with its SHA-256. | -| The run on the copies | The user runs preprocessing on the copy tree with the same output folder (`--output`): the recorded experiment root becomes the copy tree and the report says so (`experiment_root_changed`, and `conversion` with the record, its SHA-256 and `source_root`; the text says "named-axis copies of ..."); every row is recomputed (the row key includes the read path, §9); the design, image type and calibration are kept (same relative paths). The manifest records `conversion_record_sha256`. The run and `check` warn when a source changed or went missing after the conversion (`conversion_source_changed`) and when a copy changed, went missing or was added since (`copy_changed_after_conversion`); an unreadable record stops the run (`conversion_record_unreadable`). `RunInputs` reads the copies like any images, and refuses a run whose conversion record changed or went missing since (`conversion_record_sha256`). | +| Repeat | A copy already in the tree is reused when its source (fingerprint and axes), roles, copy axes, pixel sizes, unknown sizes (roles and reasons), channel names, significant bits, the copy's fingerprint and the copy's SHA-256 (read whole) are as recorded; a new image is added; when every copy is reused the record is kept as it was (same SHA-256). Anything else (a source changed, other roles, a copy changed, an image no longer in the experiment, another experiment's copy tree, a file at a new image's place that the record does not list) is `copy_differs`: nothing is overwritten. A conversion record that cannot be read, or of version 1, is `conversion_record_unreadable`. | +| Conversion record | `.morphobricks-conversion.json` in the copy tree's root (a dot-file, so discovery skips it), record version 2: `source_root`, `mapping`, `missing_as_singleton`, `written_at`, `versions`, and per image `relative_path`, `action` (`converted` or `copied`), `reason` (the axis code, `excluded`, or null), `source` and `copy` fingerprints (`size`, `mtime_ns`), `copy_sha256` (the copy's SHA-256, computed in one pass after it is written; an OME-Zarr copy's is the SHA-256 of its files' relative paths and SHA-256s in path order, its `labels` group left out as for fingerprints: `content_hash`), `source_axes`, `roles`, `copy_axes`, `pixel_sizes` (by role, as §9), `unknown_sizes` (role: why), `channel_names` (as the copy holds them; `[]` for none) and `significant_bits` (or null). Sources keep the fingerprint rule (design v0.4 §1); the copies' SHA-256 is the owner's decision of 2 November 2026 (plan §12 question 59 (16)). Version 2 (PP-11c) added `copy_sha256`, `channel_names` and `significant_bits`; a version 1 record (v0.4.0rc3 or earlier) is refused everywhere it is read (`convert`, the run's `review_conversion`, `RunInputs`) with a message to convert the originals again into a new folder: its copies cannot be checked. `read_conversion_record(folder)` reads it with its SHA-256. | +| The run on the copies | The user runs preprocessing on the copy tree with the same output folder (`--output`): the recorded experiment root becomes the copy tree and the report says so (`experiment_root_changed`, and `conversion` with the record, its SHA-256 and `source_root`; the text says "named-axis copies of ..."); every row is recomputed (the row key includes the read path, §9); the design, image type and calibration are kept (same relative paths). The manifest records `conversion_record_sha256`. The run and `check` warn when a source changed or went missing after the conversion (`conversion_source_changed`) and when a copy changed (its fingerprint or its SHA-256: each copy is read whole, so `check` and `run` on a copy tree cost one more read of the copies), went missing or was added since (`copy_changed_after_conversion`); an unreadable record stops the run (`conversion_record_unreadable`). `RunInputs` reads the copies like any images, and refuses a run whose conversion record changed or went missing since (`conversion_record_sha256`), is of version 1, or whose recorded images' copies no longer have the recorded fingerprint and SHA-256 (§9.1). | | Notebook | Not yet: the axis-check card names the `convert` command; the notebook card is the next PR (PP-11b, plan §10). | diff --git a/docs/plan.md b/docs/plan.md index 7e63cc48..8cd2d733 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -524,6 +524,14 @@ API, CLI). The notebook and CLI only call the API. for files with errors": a card offered after the axis check when images have an axis error or unnamed axes, calling `convert_experiment` with the confirmed roles and a folder the user names, showing the result and the next run on the copies. +- **PP-11c (conversion integrity)** *(this PR; issues #80, #87, #88, #92, #96; question 59 + (16)-(18))*: `conversion.py` split into `conversion_plan`, `conversion_reuse` and + `conversion_write` (move only); the copies' SHA-256 in the conversion record (version 2, + `content_hash`) verified at reuse, by `review_conversion` and by `RunInputs.open` + (`conversion_check`); channel names, significant bits and the unknown sizes' reasons + compared at reuse; each header bound to its image's fingerprint + (`preflight(bind_headers=True)`, `source_changed_during_check`); the copy's pixel type + from that header; `parse_size` exact. - **Release v0.4.0 (PP-13) checklist**, for every tag of v0.4.0 including release candidates, in the release PR before the tag is pushed: - set `version` in `pyproject.toml`; @@ -1465,4 +1473,26 @@ Raised during review; to be decided before the milestones that need them: disappeared, as for `calibration_sha256` and the design's SHA-256. (15) **Notebook card**: the next PR, PP-11b (§10); the axis-check card names the command meanwhile. + (16) **The copies' SHA-256** *(owner decision of 2 November 2026, after an independent + review of 8db1f4a; implemented in PP-11c, issue #87)*: a fingerprint (size and time) + does not see a change that keeps both, so the record gives each copy's SHA-256, + computed in one pass after it is written; reuse, the run's `review_conversion` and + `RunInputs.open` verify it (`RunInputs` once at open, for the copies of every image the + run recorded: one read of those copies, documented in contract §9.1). Sources keep the + fingerprint rule (design v0.4 §1). The record is version 2; a version 1 record is + refused with a message to convert again (its copies cannot be checked). Reuse also + compares the channel names and significant bits (#88) and the reasons of the unknown + sizes (#92). + (17) **One metadata epoch** *(same review; PP-11c, issue #96)*: each image is + fingerprinted before and after the header read the copy's layout comes from, and is + refused (`source_changed_during_check`) when the two differ; that fingerprint is the + one recorded and checked before and after the copy is written. The copy's pixel type + also comes from that header (the writer no longer opens the header for it), and a + plane of another type is refused, never cast. Still read again, by design: the planes' + memory check (`check_planes`, the run's own check) and the reader that reads the pixels + (it opens the file and parses its header); both are bounded by the same fingerprint. + (18) **Exact memory sizes on the command line** *(same review; PP-11c, issue #80)*: + `--memory-limit` is parsed without floating point and must be a whole number of bytes + above 0 (`1.5GiB` is accepted, `1.5` and `0.5B` are refused before anything is + read). diff --git a/docs/user_guide.md b/docs/user_guide.md index 65d6f434..adddbe05 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -583,7 +583,7 @@ report names the change and its effect. Other options: | `--exclude PATH` | Leave out an image, relative to the experiment folder; repeat for several | | `--bins N` | Bins per plane | | `--histogram-range minmax\|robust`, `--robust-quantiles LOW HIGH` | Histogram range of histograms that are not exact (default minmax; robust quantiles 0.01 99.99) | -| `--memory-limit SIZE` | Working-memory limit per plane, e.g. `2GiB` | +| `--memory-limit SIZE` | Working-memory limit per plane, e.g. `2GiB` or `1.5GiB`: a whole number of bytes above 0 (`1.5` or `0.5B` are refused before anything is read) | | `--missing-as-singleton` | Confirm that images lacking some dimensions have one plane along them | | `--image-type fluorescence\|rgb` | The image type (default: the earlier run's, else the suggested one, printed) | | `--design FILE` | The confirmed experiment design (default: `experiment_design.csv` in the output folder); it is copied to the output folder | @@ -732,13 +732,23 @@ Give `run` the output folder of your runs on the originals: the design, image ty stated pixel sizes are kept (the copies have the same names), the report says the experiment is now the folder of copies, and every row is computed again. The conversion record, **.morphobricks-conversion.json** in the folder of copies, lists every image with -its original's and its copy's size and modification time, the roles and the pixel sizes; -the run records its SHA-256 and warns when an original changes after the conversion -(`conversion_source_changed`) or a copy is changed (`copy_changed_after_conversion`). +its original's and its copy's size and modification time, the copy's SHA-256 (a checksum of +its content), the roles, the pixel sizes, the channel names and the significant bits; the +run records the record's SHA-256 and warns when an original changes after the conversion +(`conversion_source_changed`) or a copy is changed (`copy_changed_after_conversion`, also a +change that keeps the copy's size and time). Checking the copies reads each of them whole, +so `check` and `run` on a folder of copies, and opening their run, take about one read of +the copies longer. An original that changes while `convert` reads its metadata is refused +(`source_changed_during_check`); convert again once nothing writes into the experiment. + +A folder of copies made by v0.4.0rc3 or earlier has a conversion record of version 1, without the +copies' SHA-256: `convert`, `run` and the bricks refuse it (`conversion_record_unreadable`, +naming version 1). Convert the originals again into a new folder. Running `convert` again with the same images and roles reuses the copies and only adds new images; when nothing is added the record is left as it was. Bricks reading the run on the -copies (`RunInputs`) refuse it if the conversion record is changed or removed afterwards. +copies (`RunInputs`) refuse it if the conversion record is changed or removed afterwards, +or if a copy no longer has the SHA-256 the record gives. `convert` never overwrites anything else: a folder holding other files, copies of other images or made with other roles is refused (`copy_folder_not_empty`, `copy_differs`); convert into a new folder then. If it fails (`conversion_failed`), the folder is left as it @@ -889,11 +899,12 @@ continues. | `copy_folder_not_empty` | Error | `convert`: the folder for the copies holds files and no conversion record. Nothing is overwritten; choose a new or empty folder. | | `copy_differs` | Error | `convert`: images in the folder of copies differ from the copies this conversion would write (an original changed, other roles, a copy changed, an image no longer in the experiment), or the folder holds copies of another experiment. Nothing is overwritten; convert into a new folder, or remove those copies first. | | `cannot_convert_pixel_type` | Error | `convert`: images of 64-bit integers cannot be saved as OME-TIFF, and pixel values are never changed to fit. Save them again with named axes in a 32-bit type if their values fit, or correct the roles. | +| `source_changed_during_check` | Error | `convert`: an original changed while its metadata was read, so the metadata may not describe the file that would be copied. Nothing was written; convert again when nothing writes into the experiment. | | `conversion_failed` | Error | `convert`: an image could not be copied (unreadable, changed while copied, or the folder cannot be written). The copies written by this conversion were removed; fix the cause and convert again, or exclude the image. | | `not_enough_space` | Error | `convert`: the copies would not fit on the drive. Free space or choose another drive. | -| `conversion_record_unreadable` | Error | The conversion record of a folder of named-axis copies (**.morphobricks-conversion.json**) cannot be read, so `run` stops and `convert` refuses the folder. Convert again into a new folder. | +| `conversion_record_unreadable` | Error | The conversion record of a folder of named-axis copies (**.morphobricks-conversion.json**) cannot be read, or is of version 1 (made by v0.4.0rc3 or earlier, without the copies' SHA-256), so `run` stops and `convert` refuses the folder. Convert again into a new folder. | | `conversion_source_changed` | Warning | An original changed or went missing after its named-axis copy was made; the copies hold the images as converted. If the originals were corrected, convert again into a new folder. | -| `copy_changed_after_conversion` | Warning | An image of the folder of named-axis copies changed, went missing or was added after the conversion, so the conversion record does not describe it. Convert again, or check it is the image you want. | +| `copy_changed_after_conversion` | Warning | An image of the folder of named-axis copies changed (its size, time or SHA-256), went missing or was added after the conversion, so the conversion record does not describe it. Convert again, or check it is the image you want. | | `design_unreadable` | Error | The `experiment_design.csv` an earlier run wrote cannot be read. Correct it, or delete it to confirm the design again. | | `no_usable_planes` | Error | No plane has finite pixel values. Check the images. | | `tiff_outside_leaf` | Error | Images sit in a folder that also holds subfolders, so they have no condition or repeat of their own. A folder holds either images or subfolders, never both. Move the images into a folder of their own, or out of the experiment folder. | diff --git a/pyproject.toml b/pyproject.toml index f8e77cac..7b3f9faa 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -158,7 +158,9 @@ baseline = "tools/code_rules_baseline.json" "morphobricks_preprocessing.cli_design" = "ui" "morphobricks_preprocessing.cli_convert" = "ui" "morphobricks_preprocessing.conversion" = "orchestration" +"morphobricks_preprocessing.conversion_check" = "io" "morphobricks_preprocessing.conversion_issues" = "pure" +"morphobricks_preprocessing.content_hash" = "io" "morphobricks_preprocessing.conversion_plan" = "orchestration" "morphobricks_preprocessing.conversion_reuse" = "io" "morphobricks_preprocessing.conversion_write" = "io" diff --git a/src/morphobricks_preprocessing/cli_run.py b/src/morphobricks_preprocessing/cli_run.py index 7e6eee9d..2f55eef1 100644 --- a/src/morphobricks_preprocessing/cli_run.py +++ b/src/morphobricks_preprocessing/cli_run.py @@ -14,6 +14,7 @@ import re from dataclasses import replace from datetime import date +from fractions import Fraction from pathlib import Path from typing import Literal @@ -67,20 +68,31 @@ def parse_size(text: str) -> int: - """Parse a memory size such as ``"1GiB"``, ``"512 MiB"`` or ``"1073741824"``. + """Parse a memory size such as ``"1GiB"``, ``"1.5 GiB"`` or ``"1073741824"``. + + The size must be an exact whole number of bytes above 0, computed without rounding + (``"1.5GiB"`` is 1610612736 bytes; ``"1.5"`` and ``"0.5B"`` are refused, issue #80). + ``argparse`` calls it while reading the command line, so a refused size stops the + command before anything is read. Raises ------ argparse.ArgumentTypeError - If the text is not a positive size. + If the text is not a whole positive number of bytes. """ - match = re.fullmatch(r"\s*(\d+(?:\.\d+)?)\s*([A-Za-z]*)\s*", text) + match = re.fullmatch(r"\s*([0-9]+(?:\.[0-9]+)?)\s*([A-Za-z]*)\s*", text, flags=re.ASCII) unit = match.group(2).upper() if match else None - if not match or unit not in _UNITS or float(match.group(1)) <= 0: + if not match or unit not in _UNITS: raise argparse.ArgumentTypeError( f"{text!r} is not a memory size; use for example 1GiB, 512MiB or a number of bytes." ) - return int(float(match.group(1)) * _UNITS[unit]) + size = Fraction(match.group(1)) * _UNITS[unit] + if size.denominator != 1 or size <= 0: + raise argparse.ArgumentTypeError( + f"{text!r} is not a whole number of bytes above 0; use for example 1GiB, 512MiB " + "or a whole number of bytes." + ) + return int(size) def command_preflight(args: argparse.Namespace) -> Preflight: diff --git a/src/morphobricks_preprocessing/content_hash.py b/src/morphobricks_preprocessing/content_hash.py new file mode 100644 index 00000000..8e3bd835 --- /dev/null +++ b/src/morphobricks_preprocessing/content_hash.py @@ -0,0 +1,85 @@ +"""The SHA-256 of a copy's content: a file, or an OME-Zarr folder (contract §17, PP-11c). + +A copy's fingerprint (size and modification time) does not see a change that keeps both, +so the conversion record also gives each copy's SHA-256 (owner decision of 2 November +2026, plan §12 question 59 (16)), and reuse, :func:`.conversion_record.review_conversion` +and :class:`.run_inputs.RunInputs` check it. Sources keep the fingerprint rule (design v0.4 +§1). + +An OME-Zarr copy is hashed as the SHA-256 of its files' relative paths and SHA-256s, in +path order. Its ``labels`` group is left out, as :func:`.manifest.source_fingerprint` +leaves it out: label images are masks, recorded and checked one by one by the run. +""" + +from __future__ import annotations + +import hashlib +import os +from pathlib import Path + +from .manifest import LABELS_GROUP, file_sha256 + + +def content_sha256(path: str | os.PathLike) -> str: + """Return the SHA-256 of a file, or of an OME-Zarr folder's files (``labels`` aside). + + Parameters + ---------- + path + The file or folder. + + Returns + ------- + str + The SHA-256, as 64 hexadecimal digits. + + Raises + ------ + OSError + If the file or folder cannot be read (``FileNotFoundError`` when it is gone). + """ + path = Path(path) + if not path.is_dir(): + return file_sha256(path) + digest = hashlib.sha256() + for relative in _folder_files(path): + digest.update(f"{relative}\0{file_sha256(path / relative)}\n".encode()) + return digest.hexdigest() + + +def _folder_files(folder: Path) -> list[str]: + """Return the files of a folder, relative and sorted, without its ``labels`` group.""" + found = [] + for current, subfolders, names in os.walk(folder, onerror=_raise): + if Path(current) == folder and LABELS_GROUP in subfolders: + subfolders.remove(LABELS_GROUP) + relative = Path(current).relative_to(folder) + found += [(relative / name).as_posix() for name in names] + return sorted(found) + + +def _raise(exc: OSError) -> None: + """Raise a folder that cannot be listed: a copy is never hashed in part.""" + raise exc + + +def unchanged_content(path: str | os.PathLike, expected: str | None) -> bool: + """Return True when a copy is there and its SHA-256 is ``expected``. + + Parameters + ---------- + path + The copy. + expected + The SHA-256 recorded when it was written; None (not recorded) is never unchanged. + + Returns + ------- + bool + """ + if expected is None: + return False + try: + return content_sha256(path) == expected + except FileNotFoundError: + return False diff --git a/src/morphobricks_preprocessing/conversion_check.py b/src/morphobricks_preprocessing/conversion_check.py new file mode 100644 index 00000000..f8917f27 --- /dev/null +++ b/src/morphobricks_preprocessing/conversion_check.py @@ -0,0 +1,67 @@ +"""Check a copy tree for :class:`.run_inputs.RunInputs` (contract §9.1, §17, PP-11c). + +A run on named-axis copies records its conversion record's SHA-256 +(``conversion_record_sha256``). When the run is opened, the record must be the same file, +of the current record version, and each copy the run recorded must have the fingerprint +and the SHA-256 the record gives (issue #87). Each copy is read whole, once, when the run is +opened: one read of the copies the run recorded, never skipped. Reads after that check the +fingerprint, as for any image. +""" + +from __future__ import annotations + +from collections.abc import Iterable +from pathlib import Path + +from .conversion_record import ( + RECORD_FILENAME, + ConversionRecordError, + changed_copies, + read_record, +) +from .manifest import file_sha256 +from .validation import agree, plural + + +def conversion_problem(root: Path, expected: str, paths: Iterable[str]) -> str | None: + """Return why a run on named-axis copies cannot be used, or None when it can. + + Parameters + ---------- + root + The copy tree (the run's experiment folder now). + expected + The record's SHA-256 the run recorded. + paths + The images the run recorded, relative to ``root``. + + Returns + ------- + str or None + The message for the user, or None. + """ + path = root / RECORD_FILENAME + if not path.exists() or file_sha256(path) != expected: + return ( + f"The conversion record {path.name} in {root} is not the one the run read " + "(missing, or changed since), so it is not known how these named-axis copies " + "were made. Run preprocessing again on the copies (or, if the copies were only " + "moved, give experiment_root= where they are now)." + ) + try: + found = read_record(root) + except ConversionRecordError as exc: + return ( + f"The conversion record {path.name} in {root} cannot be used ({exc}). Convert the " + "originals again into a new folder and run preprocessing on that one." + ) + changed = changed_copies(root, found[0], paths) if found is not None else [] + if not changed: + return None + them = agree(changed, "it", "them") + return ( + f"{plural(len(changed), 'named-axis copy', 'named-axis copies')} changed since the " + f"conversion wrote {them} (or the conversion record does not list {them}): " + f"{', '.join(changed[:5])}{' and more' if len(changed) > 5 else ''}. Convert the " + "originals again into a new folder and run preprocessing on that one." + ) diff --git a/src/morphobricks_preprocessing/conversion_issues.py b/src/morphobricks_preprocessing/conversion_issues.py index d6d4ff8a..76f5ac3e 100644 --- a/src/morphobricks_preprocessing/conversion_issues.py +++ b/src/morphobricks_preprocessing/conversion_issues.py @@ -10,7 +10,7 @@ from typing import TYPE_CHECKING from .conversion_record import RECORD_FILENAME -from .validation import ValidationIssue, error, plural +from .validation import ValidationIssue, agree, error, plural if TYPE_CHECKING: from .conversion_record import ConversionRecordError @@ -64,6 +64,18 @@ def pixel_type_issue(paths: list[str], dtype: str) -> ValidationIssue: ) +def header_changed_issue(paths: list[str]) -> ValidationIssue: + """Refuse images that changed while their headers were read (issue #96).""" + return error( + "source_changed_during_check", + f"{plural(len(paths), 'image')} changed while {agree(paths, 'its', 'their')} " + "header was read, so the header may not describe the file that would be copied. " + "Nothing was written.", + paths, + "Wait until nothing is writing into the experiment folder, then convert again.", + ) + + def not_empty_issue(dest: Path, paths: list[str]) -> ValidationIssue: """Refuse a folder that holds files and no conversion record.""" return error( diff --git a/src/morphobricks_preprocessing/conversion_plan.py b/src/morphobricks_preprocessing/conversion_plan.py index 2cdf195d..5ddccda7 100644 --- a/src/morphobricks_preprocessing/conversion_plan.py +++ b/src/morphobricks_preprocessing/conversion_plan.py @@ -3,6 +3,12 @@ Moved from :mod:`.conversion` (PP-11c). :func:`plan_conversion` reads only the headers: the axis errors and unnamed axes are what the conversion corrects; any other error stops it before anything is written. + +One metadata epoch (issue #96): each image is fingerprinted before and after its header is +read (``preflight(bind_headers=True)``), and an image whose fingerprint differs is refused +(``source_changed_during_check``). The copy's axes, sizes, channel names and pixel type +come from that header, and the fingerprint taken with it is the one the copy's source must +still have before and after it is copied (:mod:`.conversion_write`). """ from __future__ import annotations @@ -11,6 +17,8 @@ from dataclasses import dataclass from pathlib import Path +import numpy as np + from .axes import AxisMapping from .axis_check import ( AXES_NOT_NAMED, @@ -19,7 +27,7 @@ header_axis_issue, ) from .axis_review import review_axes -from .conversion_issues import pixel_type_issue +from .conversion_issues import header_changed_issue, pixel_type_issue from .conversion_record import CONVERTED, COPIED, CopyEntry from .copy_axes import CopyLayout, copy_layout from .fingerprint import Fingerprint @@ -47,12 +55,15 @@ class PlannedCopy: The roles a converted image is read with, else None. nbytes The bytes the copy takes (pixels only for a converted image). + dtype + The pixel type a converted image's header gives (native byte order), else None. """ entry: CopyEntry layout: CopyLayout | None = None mapping: AxisMapping | None = None nbytes: int = 0 + dtype: np.dtype | None = None @property def relative_path(self) -> str: @@ -70,10 +81,12 @@ def plan_conversion( ) -> tuple[tuple[PlannedCopy, ...], list[ValidationIssue]]: """Check the experiment with the confirmed roles and plan each image's copy. - Only the headers are read. The axis errors do not stop the conversion: they are what - it corrects. Any other error does, and nothing is planned: so does an image to convert - of a pixel type OME-TIFF cannot hold, or whose planes cannot be read within the memory - limit, not even in blocks (checked as the run checks them, before anything is written). + Only the headers are read, each between two fingerprints of its image (an image that + changed meanwhile stops the conversion). The axis errors do not stop the conversion: + they are what it corrects. Any other error does, and nothing is planned: so does an + image to convert of a pixel type OME-TIFF cannot hold, or whose planes cannot be read + within the memory limit, not even in blocks (checked as the run checks them, before + anything is written). Parameters ---------- @@ -96,7 +109,10 @@ def plan_conversion( The planned copies, sorted by path (empty when an error stops the conversion), and the issues: the errors that stop it, and the warnings. """ - pre = preflight(root, destination, exclude=exclude) + pre = preflight(root, destination, exclude=exclude, bind_headers=True) + unsettled = sorted(rel for rel, bound in pre.header_fingerprints.items() if bound is None) + if unsettled: + return (), [header_changed_issue(unsettled)] review = review_axes(pre.metadata, mapping, missing_as_singleton=missing_as_singleton) others = [issue for issue in pre.issues if issue not in pre.metadata.issues] issues = [*others, *review.combined_issues(pre.metadata)] @@ -111,14 +127,12 @@ def plan_conversion( return (), issues planned = [] for rel in pre.discovery.images: - source = Fingerprint.of(root / rel) + # An excluded image is never read: its fingerprint is taken now, for the byte copy. + source = pre.header_fingerprints.get(rel) or Fingerprint.of(root / rel) if rel in to_convert: planned.append(_converted(headers[rel], review.file_mappings[rel], source)) else: - reason = "excluded" if rel in pre.excluded else None - meta = headers.get(rel) - entry = CopyEntry(rel, COPIED, reason, source, source_axes=meta.axes if meta else "") - planned.append(PlannedCopy(entry, nbytes=source.size)) + planned.append(_copied(rel, headers.get(rel), source, rel in pre.excluded)) return tuple(planned), issues @@ -149,5 +163,23 @@ def _converted(meta: TiffMetadata, mapping: AxisMapping, source: Fingerprint) -> copy_axes=layout.axes, pixel_sizes=layout.sizes, unknown_sizes=layout.unknown, + channel_names=layout.channel_names, + significant_bits=layout.significant_bits, + ) + return PlannedCopy(entry, layout, mapping, copy_nbytes(layout, meta.dtype), meta.dtype) + + +def _copied( + rel: str, meta: TiffMetadata | None, source: Fingerprint, excluded: bool +) -> PlannedCopy: + """Plan the byte-for-byte copy of one image (its header as it is, or excluded unread).""" + entry = CopyEntry( + relative_path=rel, + action=COPIED, + reason="excluded" if excluded else None, + source=source, + source_axes=meta.axes if meta else "", + channel_names=meta.channel_names if meta else (), + significant_bits=meta.significant_bits if meta else None, ) - return PlannedCopy(entry, layout, mapping, copy_nbytes(layout, meta.dtype)) + return PlannedCopy(entry, nbytes=source.size) diff --git a/src/morphobricks_preprocessing/conversion_record.py b/src/morphobricks_preprocessing/conversion_record.py index cbbfc317..a531fc44 100644 --- a/src/morphobricks_preprocessing/conversion_record.py +++ b/src/morphobricks_preprocessing/conversion_record.py @@ -3,10 +3,15 @@ :func:`.conversion.convert_experiment` writes it last, into the root of the copy tree, as :data:`RECORD_FILENAME` (a dot-file: discovery skips it, so it is never taken for an image). It lists every image of the tree with its source's and its copy's fingerprints, -relative paths, roles, axes and pixel sizes, and the package versions. A run on the copy -tree records the record's SHA-256 in its manifest (``conversion_record_sha256``) and +the copy's SHA-256 (:mod:`.content_hash`), relative paths, roles, axes, pixel sizes, +channel names and significant bits, and the package versions. A run on the copy tree +records the record's SHA-256 in its manifest (``conversion_record_sha256``) and :func:`review_conversion` warns when a source changed after the conversion, or a copy -since it was written. +since it was written (its fingerprint or its SHA-256). + +Record version 2 (PP-11c) added the copies' SHA-256, channel names and significant bits; +a version 1 record is refused with a message to convert again +(:class:`ConversionRecordError`), since its copies cannot be checked. """ from __future__ import annotations @@ -19,6 +24,7 @@ from ._io import write_text_atomic from .axes import AxisMapping +from .content_hash import unchanged_content from .fingerprint import Fingerprint, current_fingerprint from .manifest import file_sha256 from .recorded_sizes import RecordedSize, sizes_from_data, sizes_to_data @@ -27,7 +33,11 @@ RECORD_FILENAME = ".morphobricks-conversion.json" """The conversion record, in the root of the copy tree.""" -RECORD_VERSION = "1" +RECORD_VERSION = "2" +"""Version 2 (PP-11c): each copy's SHA-256, channel names and significant bits.""" + +_OLD_VERSIONS = frozenset({"1"}) +"""Record versions written before the copies' SHA-256: refused, convert again.""" CONVERTED = "converted" """An image written as a named-axis OME-TIFF with the confirmed roles.""" @@ -69,6 +79,14 @@ class CopyEntry: The copy's pixel sizes by role (converted images). unknown_sizes ``{role: reason}`` for each size the source header gives that the copy does not. + copy_sha256 + The copy's SHA-256 when it was written (:func:`.content_hash.content_sha256`; + None before it is written). + channel_names + The channel names the copy holds (a converted image's as written, a copied + image's from its header); empty for none. + significant_bits + The significant bits the copy holds, or None. """ relative_path: str @@ -81,13 +99,17 @@ class CopyEntry: copy_axes: str | None = None pixel_sizes: tuple[RecordedSize, ...] = () unknown_sizes: Mapping[str, str] = field(default_factory=dict) + copy_sha256: str | None = None + channel_names: tuple[str | None, ...] = () + significant_bits: int | None = None def same_plan(self, other: CopyEntry) -> bool: """Return True if ``other`` copies the same source the same way (copies aside). The source's fingerprint and axes, the action and reason, the roles, the copy's - axes and its pixel sizes (and which sizes are unknown) must all be the same, so a - copy written under another rule is never reused beside new ones. + axes, its pixel sizes, which sizes are unknown and why, its channel names and + significant bits must all be the same, so a copy written under another rule is + never reused beside new ones. """ return self._plan() == other._plan() @@ -102,7 +124,9 @@ def _plan(self) -> tuple: self.roles, self.copy_axes, self.pixel_sizes, - frozenset(self.unknown_sizes), + tuple(sorted(self.unknown_sizes.items())), + tuple(self.channel_names), + self.significant_bits, ) def to_dict(self) -> dict: @@ -118,13 +142,18 @@ def to_dict(self) -> dict: "copy_axes": self.copy_axes, "pixel_sizes": sizes_to_data(self.pixel_sizes), "unknown_sizes": dict(sorted(self.unknown_sizes.items())), + "copy_sha256": self.copy_sha256, + "channel_names": list(self.channel_names), + "significant_bits": self.significant_bits, } @classmethod def from_dict(cls, data: Mapping) -> CopyEntry: - """Read an entry written by :meth:`to_dict`.""" + """Read an entry written by :meth:`to_dict`; it must give its copy and SHA-256.""" roles = data.get("roles") copy = data.get("copy") + if copy is None or not isinstance(data.get("copy_sha256"), str): + raise ValueError(f"{data.get('relative_path')!r} has no copy fingerprint or SHA-256") return cls( relative_path=str(data["relative_path"]), action=_choice(data["action"], (CONVERTED, COPIED), "action"), @@ -136,6 +165,9 @@ def from_dict(cls, data: Mapping) -> CopyEntry: copy_axes=None if data.get("copy_axes") is None else str(data["copy_axes"]), pixel_sizes=sizes_from_data(data.get("pixel_sizes")), unknown_sizes={str(k): str(v) for k, v in (data.get("unknown_sizes") or {}).items()}, + copy_sha256=str(data["copy_sha256"]), + channel_names=tuple(None if n is None else str(n) for n in data["channel_names"]), + significant_bits=_bits(data["significant_bits"]), ) @@ -185,8 +217,15 @@ def from_dict(cls, data: Mapping) -> ConversionRecord: Raises ------ ConversionRecordError - For another record version or data that is not a record. + For another record version (a version 1 record: convert again) or data that is + not a record. """ + if isinstance(data, Mapping) and data.get("record_version") in _OLD_VERSIONS: + raise ConversionRecordError( + f"it is a record of version {data['record_version']}, written before the " + "copies' SHA-256 was recorded, so the copies cannot be checked; convert the " + "originals again into a new folder" + ) if not isinstance(data, Mapping) or data.get("record_version") != RECORD_VERSION: raise ConversionRecordError(f"not a conversion record of version {RECORD_VERSION}") try: @@ -310,18 +349,47 @@ def review_conversion(root: str | os.PathLike, images: Iterable[str]) -> Convers if current_fingerprint(sources / e.relative_path) != e.source ] listed = {e.relative_path for e in record.files} - copies = sorted( - { - e.relative_path - for e in record.files - if current_fingerprint(Path(root) / e.relative_path) != e.copy - } - | (set(images) - listed) - ) + copies = sorted(set(changed_copies(root, record)) | (set(images) - listed)) issues = [*_source_issue(changed, record.source_root), *_copy_issue(copies)] return ConversionReview(sha256, record.source_root, tuple(issues)) +def changed_copies( + root: str | os.PathLike, record: ConversionRecord, paths: Iterable[str] | None = None +) -> list[str]: + """Return the copies changed or gone since they were written: fingerprint or SHA-256. + + Each copy listed is read whole to compute its SHA-256 (unless its fingerprint already + differs), so the check costs one read of the copies. + + Parameters + ---------- + root + The copy tree. + record + Its conversion record. + paths + The images to check, relative; by default every image the record lists. An image + the record does not list counts as changed. + + Returns + ------- + list of str + The changed, gone or unlisted copies, sorted. + """ + wanted = [e.relative_path for e in record.files] if paths is None else list(paths) + changed = [] + for rel in sorted(set(wanted)): + entry, copy = record.entry(rel), Path(root) / rel + if ( + entry is None + or current_fingerprint(copy) != entry.copy + or not unchanged_content(copy, entry.copy_sha256) + ): + changed.append(rel) + return changed + + def _source_issue(paths: list[str], source_root: str) -> list[ValidationIssue]: """Warn about sources changed or gone since the conversion.""" if not paths: @@ -379,6 +447,15 @@ def _choice(value: object, allowed: tuple[str, ...], name: str) -> str: return str(value) +def _bits(value: object) -> int | None: + """Return recorded significant bits: a whole number, or None.""" + if value is None: + return None + if isinstance(value, bool) or not isinstance(value, int): + raise ValueError(f"significant_bits {value!r} is not a whole number") + return value + + def _flag(value: object) -> bool: """Return a JSON boolean; raise for anything else.""" if not isinstance(value, bool): diff --git a/src/morphobricks_preprocessing/conversion_reuse.py b/src/morphobricks_preprocessing/conversion_reuse.py index efbdad1b..a79e5bbc 100644 --- a/src/morphobricks_preprocessing/conversion_reuse.py +++ b/src/morphobricks_preprocessing/conversion_reuse.py @@ -1,7 +1,8 @@ """Compare a planned conversion with what the copy tree already holds. Moved from :mod:`.conversion` (PP-11c). A copy is reused only when its plan and its copy -are unchanged; anything else already there is refused, never overwritten. +are unchanged: the copy's fingerprint and its SHA-256 (read whole, issue #87) as recorded; +anything else already there is refused, never overwritten. """ from __future__ import annotations @@ -9,6 +10,7 @@ import os from pathlib import Path +from .content_hash import unchanged_content from .conversion_issues import differs_issue, not_empty_issue, other_experiment_issue from .conversion_plan import PlannedCopy from .conversion_record import ConversionRecord, CopyEntry @@ -37,7 +39,7 @@ def compare( target = dest / item.relative_path if old is None: (differing if os.path.lexists(target) else to_write).append(item) - elif old.same_plan(item.entry) and current_fingerprint(target) == old.copy: + elif _unchanged(old, item.entry, target): reused.append(old) else: differing.append(item) @@ -47,6 +49,15 @@ def compare( return reused, to_write, differs_issue(dest, sorted(paths)) if paths else None +def _unchanged(old: CopyEntry, planned: CopyEntry, target: Path) -> bool: + """Return True when a recorded copy has the plan and the content it was written with.""" + return ( + old.same_plan(planned) + and current_fingerprint(target) == old.copy + and unchanged_content(target, old.copy_sha256) + ) + + def unknown_content(dest: Path, planned: tuple[PlannedCopy, ...]) -> list[str]: """Return the files in a folder without a record that this conversion did not leave. diff --git a/src/morphobricks_preprocessing/conversion_write.py b/src/morphobricks_preprocessing/conversion_write.py index 90d1fa31..71f194c8 100644 --- a/src/morphobricks_preprocessing/conversion_write.py +++ b/src/morphobricks_preprocessing/conversion_write.py @@ -13,6 +13,7 @@ from dataclasses import replace from pathlib import Path +from .content_hash import content_sha256 from .conversion_issues import failed_issue from .conversion_plan import PlannedCopy from .conversion_record import CopyEntry @@ -71,7 +72,8 @@ def write_all( current, target = item.relative_path, dest / item.relative_path os.replace(partial_path(dest, item), target) written.append(target) - entries.append(replace(item.entry, copy=Fingerprint.of(target))) + copy, sha256 = Fingerprint.of(target), content_sha256(target) + entries.append(replace(item.entry, copy=copy, copy_sha256=sha256)) except BaseException as exc: logger.debug("Conversion of %s failed", current, exc_info=True) for path in [*written, *(partial_path(dest, item) for item in to_write)]: @@ -115,7 +117,9 @@ def _write_partial(root: Path, dest: Path, item: PlannedCopy, memory_limit: int) if current_fingerprint(source) != item.entry.source: raise _SourceChanged("it changed since it was checked") if item.layout is not None: - write_named_copy(source, partial, item.layout, item.mapping, memory_limit=memory_limit) + write_named_copy( + source, partial, item.layout, item.mapping, dtype=item.dtype, memory_limit=memory_limit + ) elif source.is_dir(): shutil.copytree(source, partial, copy_function=shutil.copyfile) else: diff --git a/src/morphobricks_preprocessing/ome_tiff_copy.py b/src/morphobricks_preprocessing/ome_tiff_copy.py index a0d4eb47..23fb5f58 100644 --- a/src/morphobricks_preprocessing/ome_tiff_copy.py +++ b/src/morphobricks_preprocessing/ome_tiff_copy.py @@ -19,7 +19,7 @@ from .axes import AxisMapping from .copy_axes import CopyLayout -from .planes import DEFAULT_MEMORY_LIMIT, Block, iter_planes, plane_layout +from .planes import DEFAULT_MEMORY_LIMIT, Block, iter_planes BIGTIFF_FROM = 2**32 - 2**25 """Copies whose file would take more bytes than this are written as BigTIFF (a classic @@ -78,6 +78,7 @@ def write_named_copy( layout: CopyLayout, mapping: AxisMapping, *, + dtype: np.dtype, memory_limit: int = DEFAULT_MEMORY_LIMIT, ) -> None: """Write ``source``, read under ``mapping``, to ``target`` as a named-axis OME-TIFF. @@ -92,6 +93,9 @@ def write_named_copy( The copy's axes, shape and metadata (:func:`.copy_axes.copy_layout`). mapping The image's own confirmed mapping. + dtype + The pixel type the inspected header gives (native byte order): the header is not + read again for it (issue #96), and a plane of another type is refused, never cast. memory_limit Working-memory limit per plane, as the run's. @@ -100,11 +104,11 @@ def write_named_copy( PlaneReadError If the source cannot be read under ``mapping`` within the memory limit. CopyWriteError - If the planes do not lie where the file was laid out. + If the planes do not lie where the file was laid out, or are not of ``dtype``. OSError If the file cannot be written. """ - dtype = plane_layout(source, mapping, memory_limit=memory_limit).dtype + dtype = np.dtype(dtype) offset, _ = tifffile.imwrite( target, shape=layout.shape, @@ -140,7 +144,9 @@ def _check_contiguous(path: Path, offset: int, plane_nbytes: int, count: int) -> def _write_block(stream: BinaryIO, start: int, block: Block, cols: int, dtype: np.dtype) -> None: """Write one block of a plane at its place; a block as wide as the plane in one write.""" - pixels = np.ascontiguousarray(block.pixels, dtype=dtype) + if block.pixels.dtype != dtype: + raise CopyWriteError(f"a plane is {block.pixels.dtype}, not {dtype} as its header said") + pixels = np.ascontiguousarray(block.pixels) row_nbytes = cols * dtype.itemsize first = start + block.y * row_nbytes + block.x * dtype.itemsize if pixels.shape[1] == cols: diff --git a/src/morphobricks_preprocessing/run/preflight.py b/src/morphobricks_preprocessing/run/preflight.py index a12d7021..105a08ba 100644 --- a/src/morphobricks_preprocessing/run/preflight.py +++ b/src/morphobricks_preprocessing/run/preflight.py @@ -24,9 +24,10 @@ from ..conversion_record import ConversionReview, review_conversion from ..dataset import DatasetReport, check_dataset from ..discovery import DiscoveryResult, default_output_folder, discover_experiment +from ..fingerprint import Fingerprint, current_fingerprint from ..mask_names import MASK_NAME_CODES, mask_name_issues from ..masks import MaskInventory, find_masks -from ..metadata import MetadataReport, inspect_images +from ..metadata import MetadataReport, check_metadata, inspect_image, inspect_images from ..pixel_size_checks import PIXEL_SIZE_CODES, pixel_size_issues from ..planes import ( DEFAULT_MEMORY_LIMIT, @@ -70,6 +71,11 @@ class Preflight: conversion The conversion record of a tree of named-axis copies (design v0.4 §5), checked against the sources and copies; None for an ordinary experiment. + header_fingerprints + With ``bind_headers``: each inspected image's fingerprint, taken before and after + its header was read, or None when the two differ (or it went missing): the + header read is then not known to describe the file as fingerprinted. Empty + otherwise. """ discovery: DiscoveryResult @@ -81,6 +87,7 @@ class Preflight: masks: MaskInventory | None = None calibration: CalibrationReview = field(default_factory=CalibrationReview) conversion: ConversionReview | None = None + header_fingerprints: Mapping[str, Fingerprint | None] = field(default_factory=dict) @property def issues(self) -> tuple[ValidationIssue, ...]: @@ -165,6 +172,7 @@ def preflight( exclude: Iterable[str] = (), masks_folder: str | os.PathLike | None = None, mask_axes: Mapping[str, str] | None = None, + bind_headers: bool = False, ) -> Preflight: """Pass 1: check the folder and the image headers without reading pixels. @@ -184,6 +192,10 @@ def preflight( always used as masks, also without a masks folder (design v0.4 §2). mask_axes The axes stated per mask type, ``{type: roles}`` (:func:`.masks.find_masks`). + bind_headers + Fingerprint each image before and after its header is read + (:attr:`Preflight.header_fingerprints`), so a caller that writes from the headers + (:func:`.conversion_plan.plan_conversion`) never mixes a header with a later file. Returns ------- @@ -195,7 +207,8 @@ def preflight( excluded = tuple(sorted({normalise_relative_path(path) for path in exclude})) discovery = _without_excluded_masks(discovery, excluded) images = [path for path in discovery.images if path not in excluded] - metadata, calibration = _calibrated(inspect_images(root, images), discovery.images, output) + inspected, bound = _inspected(root, images, bind_headers) + metadata, calibration = _calibrated(inspected, discovery.images, output) return Preflight( discovery=discovery, metadata=metadata, @@ -206,9 +219,25 @@ def preflight( masks=_masks(discovery, metadata, masks_folder, mask_axes, output), calibration=calibration, conversion=review_conversion(root, discovery.images), + header_fingerprints=bound, ) +def _inspected( + root: Path, images: list[str], bind: bool +) -> tuple[MetadataReport, dict[str, Fingerprint | None]]: + """Inspect the headers; with ``bind``, fingerprint each image before and after.""" + if not bind: + return inspect_images(root, images), {} + files, bound = [], {} + for rel in sorted(images): + before = current_fingerprint(root / rel) + files.append(inspect_image(root / rel, rel)) + after = current_fingerprint(root / rel) + bound[rel] = before if before is not None and before == after else None + return MetadataReport(files=tuple(files), issues=tuple(check_metadata(files))), bound + + def remasked( pre: Preflight, masks_folder: str | os.PathLike | None, diff --git a/src/morphobricks_preprocessing/run_inputs.py b/src/morphobricks_preprocessing/run_inputs.py index d7ff1bc7..1222cb90 100644 --- a/src/morphobricks_preprocessing/run_inputs.py +++ b/src/morphobricks_preprocessing/run_inputs.py @@ -7,7 +7,8 @@ - :meth:`RunInputs.open` refuses a version 1 manifest (a run before the axis check), a table, design or ``calibration.csv`` whose SHA-256 is not the manifest's (also a ``calibration.csv`` added since the run), a conversion record changed or gone since a - run on named-axis copies (``conversion_record_sha256``), an image or mask changed or gone + run on named-axis copies (``conversion_record_sha256``) or one of its copies changed since + it was written (the copies' SHA-256, read whole once at open), an image or mask changed or gone since the run, a mask recorded without axes (a v0.4.0rc1 run of a mask whose header puts X before Y), and an image or mask in the experiment the run neither recorded nor excluded (:class:`UnrecordedFilesError`, from :func:`.unrecorded.unrecorded_files`: folders are @@ -35,7 +36,7 @@ from .axes import AxisMapping from .calibration import CALIBRATION_FILENAME -from .conversion_record import RECORD_FILENAME as CONVERSION_RECORD +from .conversion_check import conversion_problem from .design import DESIGN_FILENAME, ExperimentDesign, read_experiment_design from .fingerprint import Fingerprint from .manifest import ( @@ -169,7 +170,8 @@ def open( ------ RunInputsError For a run before v0.4.0, a table, design, calibration or conversion record (of a - run on named-axis copies) changed or gone since the run, + run on named-axis copies) changed or gone since the run, a named-axis copy whose + SHA-256 is not its record's (or a record of version 1, without them), an image or mask changed or gone, a mask recorded without axes, OME-Zarr images without the ``zarr`` extra (``morphobricks-preprocessing[zarr]``), or an OME-Zarr image whose ``labels`` group cannot be read. @@ -184,7 +186,7 @@ def open( _check_sha(output / DESIGN_FILENAME, manifest.experiment_design_sha256, "design") _check_calibration(output / CALIBRATION_FILENAME, manifest.calibration_sha256) root = Path(experiment_root or manifest.experiment_root).resolve() - _check_conversion(root, manifest.conversion_record_sha256) + _check_conversion(root, manifest) recorded_masks = (manifest.masks or {}).get("folder") masks = masks_root or recorded_masks inputs = cls( @@ -474,16 +476,14 @@ def _check_calibration(path: Path, expected: str | None) -> None: ) -def _check_conversion(root: Path, expected: str | None) -> None: - """Refuse a conversion record changed or gone since a run on named-axis copies.""" - path = root / CONVERSION_RECORD - if expected is not None and (not path.exists() or file_sha256(path) != expected): - raise RunInputsError( - f"The conversion record {path.name} in {root} is not the one the run read " - "(missing, or changed since), so it is not known how these named-axis copies " - "were made. Run preprocessing again on the copies (or, if the copies were only " - "moved, give experiment_root= where they are now)." - ) +def _check_conversion(root: Path, manifest: RunManifest) -> None: + """Refuse a changed or gone conversion record, or a copy changed since it was written.""" + if manifest.conversion_record_sha256 is None: + return + paths = [e.relative_path for e in manifest.files] + problem = conversion_problem(root, manifest.conversion_record_sha256, paths) + if problem is not None: + raise RunInputsError(problem) def _changed_text(paths: list[str], what: str = "image") -> str: diff --git a/tests/test_convert_integrity.py b/tests/test_convert_integrity.py new file mode 100644 index 00000000..b17b429f --- /dev/null +++ b/tests/test_convert_integrity.py @@ -0,0 +1,281 @@ +"""Conversion integrity (PP-11c; issues #80, #87, #88, #92, #96; contract §9.1, §17). + +- One metadata epoch: each image is fingerprinted before and after its header is read, and + an image that changed meanwhile is refused before anything is written (#96). +- Each copy's SHA-256 is recorded, and reuse, the run's conversion review and ``RunInputs`` + check it, so a same-size change with the time put back is found (#87). +- Reuse compares the copy's channel names and significant bits (#88), and the reasons of + the unknown sizes, not only which sizes are unknown (#92). +- The command line takes a memory size only as an exact whole number of bytes (#80). +""" + +import argparse +import json +import os + +import numpy as np +import pytest +import tifffile + +import morphobricks_preprocessing.cli_convert as cli_convert +import morphobricks_preprocessing.cli_run as cli_run +import morphobricks_preprocessing.metadata as metadata_module +import morphobricks_preprocessing.ome_tiff_copy as copy_module +from morphobricks_preprocessing import ( + CONVERSION_RECORD_FILENAME, + AxisMapping, + RunInputs, + RunInputsError, + convert_experiment, + file_sha256, + read_conversion_record, + run_experiment, +) +from morphobricks_preprocessing.cli import main, parse_size +from morphobricks_preprocessing.copy_axes import copy_layout +from morphobricks_preprocessing.metadata import inspect_image +from morphobricks_preprocessing.ome_tiff_copy import CopyWriteError, write_named_copy +from test_convert_run import SWAPPED, codes, snapshot, swapped_experiment + +NAMED = AxisMapping("ZYX", tuple("ZYX")) + + +def change_in_place(path): + """Change one pixel byte of a TIFF file, keeping its size and modification time.""" + stat = path.stat() + with tifffile.TiffFile(path) as tif: + offset = tif.pages[0].dataoffsets[0] + data = bytearray(path.read_bytes()) + data[offset] ^= 0xFF + path.write_bytes(bytes(data)) + os.utime(path, ns=(stat.st_atime_ns, stat.st_mtime_ns)) + assert (path.stat().st_size, path.stat().st_mtime_ns) == (stat.st_size, stat.st_mtime_ns) + + +def edit_record(copies, change): + """Rewrite the conversion record after ``change(data)``; return the new SHA-256.""" + path = copies / CONVERSION_RECORD_FILENAME + data = json.loads(path.read_text(encoding="utf-8")) + change(data) + path.write_bytes((json.dumps(data, indent=2) + "\n").encode()) + return file_sha256(path) + + +# #96: one metadata epoch ------------------------------------------------------------------- + + +def test_an_image_changed_while_its_header_is_read_is_refused_and_nothing_written( + tmp_path, monkeypatch +): + exp, copies = swapped_experiment(tmp_path / "exp"), tmp_path / "copies" + target = exp / "ctrl" / "r1" / "b.tif" + original, changed = metadata_module.inspect_tiff, [] + + def inspect_then_change(path, *args, **kwargs): + meta = original(path, *args, **kwargs) + if os.path.samefile(path, target) and not changed: + changed.append(path) # new pixels and a new calibration, after the header read + tifffile.imwrite( + target, np.ones((2, 6, 9), np.uint8), ome=True, + metadata={"axes": "ZYX", "PhysicalSizeZ": 5.0, "PhysicalSizeX": 0.1}, + ) # fmt: skip + return meta + + monkeypatch.setattr(metadata_module, "inspect_tiff", inspect_then_change) + result = convert_experiment(exp, SWAPPED, copies) + assert changed + assert not result.written and codes(result.issues) == ["source_changed_during_check"] + assert result.issues[0].paths == ("ctrl/r1/b.tif",) + assert "Nothing was written" in result.issues[0].message + assert not copies.exists() or [p for p in copies.rglob("*")] == [] + monkeypatch.undo() + assert convert_experiment(exp, SWAPPED, copies).written # settled now: converted + + +def test_writing_a_copy_does_not_read_the_source_header_again(tmp_path, monkeypatch): + # #96: the pixel type comes from the inspected header; only the reader opens the file. + monkeypatch.setattr( + copy_module, "plane_layout", lambda *a, **k: pytest.fail("header read"), raising=False + ) + exp, copies = swapped_experiment(tmp_path / "exp"), tmp_path / "copies" + assert convert_experiment(exp, SWAPPED, copies).written + + +def test_a_plane_of_another_pixel_type_than_the_header_said_is_refused(tmp_path): + exp = swapped_experiment(tmp_path / "exp", names=("a.tif",)) + meta = inspect_image(exp / "a.tif", "a.tif") + layout = copy_layout(meta, SWAPPED) + with pytest.raises(CopyWriteError, match="not uint16"): + write_named_copy(exp / "a.tif", tmp_path / "a.tif", layout, SWAPPED, dtype=np.uint16) + + +# #87: the copies' SHA-256 ------------------------------------------------------------------ + + +def test_each_copys_sha256_is_recorded(tmp_path): + exp, copies = swapped_experiment(tmp_path / "exp"), tmp_path / "copies" + assert convert_experiment(exp, SWAPPED, copies).written + data = json.loads((copies / CONVERSION_RECORD_FILENAME).read_text()) + assert data["record_version"] == "2" + for entry in data["files"]: + assert entry["copy_sha256"] == file_sha256(copies / entry["relative_path"]) + + +def test_a_same_size_change_with_the_time_put_back_is_found_at_reuse(tmp_path): + exp, copies = swapped_experiment(tmp_path / "exp"), tmp_path / "copies" + assert convert_experiment(exp, SWAPPED, copies).written + change_in_place(copies / "ctrl" / "r1" / "b.tif") + before = snapshot(copies) + again = convert_experiment(exp, SWAPPED, copies) + assert not again.written and codes(again.issues) == ["copy_differs"] + assert again.issues[0].paths == ("ctrl/r1/b.tif",) + assert snapshot(copies) == before + + +def test_the_run_on_the_copies_warns_about_a_same_size_change(tmp_path): + exp, copies = swapped_experiment(tmp_path / "exp"), tmp_path / "copies" + assert convert_experiment(exp, SWAPPED, copies).written + change_in_place(copies / "drug" / "r1" / "a.tif") + run = run_experiment(copies, NAMED, output_folder=tmp_path / "out") + found = {issue.code: issue for issue in run.issues} + assert found["copy_changed_after_conversion"].paths == ("drug/r1/a.tif",) + + +def test_run_inputs_refuses_a_copy_changed_since_it_was_written(tmp_path): + exp, copies, out = swapped_experiment(tmp_path / "exp"), tmp_path / "copies", tmp_path / "o" + assert convert_experiment(exp, SWAPPED, copies).written + assert run_experiment(copies, NAMED, output_folder=out).written + RunInputs.open(out) + change_in_place(copies / "ctrl" / "r1" / "a.tif") + with pytest.raises(RunInputsError, match="1 named-axis copy changed .*ctrl/r1/a.tif"): + RunInputs.open(out) + + +def test_an_ome_zarr_copy_changed_in_place_is_not_reused(tmp_path): + from zarr_tree import write_ngff + + exp, copies = swapped_experiment(tmp_path / "exp", names=("c/a.tif",)), tmp_path / "copies" + write_ngff(exp / "c" / "z.zarr", np.arange(2 * 9 * 6, dtype=np.uint8).reshape(2, 9, 6), "zyx") + assert convert_experiment(exp, SWAPPED, copies).written + chunk = max( + (p for p in (copies / "c" / "z.zarr").rglob("*") if p.is_file()), + key=lambda p: p.stat().st_size, + ) + stat = chunk.stat() + data = bytearray(chunk.read_bytes()) + data[-1] ^= 0xFF + chunk.write_bytes(bytes(data)) + os.utime(chunk, ns=(stat.st_atime_ns, stat.st_mtime_ns)) + again = convert_experiment(exp, SWAPPED, copies) + assert not again.written and again.issues[0].paths == ("c/z.zarr",) + + +def test_a_version_1_record_is_refused_with_a_reason(tmp_path): + exp, copies, out = swapped_experiment(tmp_path / "exp"), tmp_path / "copies", tmp_path / "o" + assert convert_experiment(exp, SWAPPED, copies).written + assert run_experiment(copies, NAMED, output_folder=out).written + + def version_1(data): + data["record_version"] = "1" + for entry in data["files"]: + for key in ("copy_sha256", "channel_names", "significant_bits"): + del entry[key] + + sha256 = edit_record(copies, version_1) + again = convert_experiment(exp, SWAPPED, copies) + assert not again.written and codes(again.issues) == ["conversion_record_unreadable"] + assert "version 1" in again.issues[0].message and "convert" in again.issues[0].message + # A run made on such a copy tree before PP-11c: RunInputs refuses, never skips the check. + manifest = json.loads((out / "run_manifest.json").read_text()) + manifest["conversion_record_sha256"] = sha256 + (out / "run_manifest.json").write_bytes(json.dumps(manifest, indent=2).encode()) + with pytest.raises(RunInputsError, match="version 1.*Convert the originals again"): + RunInputs.open(out) + + +# #88 and #92: what reuse compares ---------------------------------------------------------- + + +def channel_experiment(root, sizes=None): + path = root / "c" / "img.tif" + path.parent.mkdir(parents=True) + tifffile.imwrite( + path, np.zeros((2, 6, 9), np.uint16), ome=True, photometric="minisblack", + metadata={"axes": "CYX", "Channel": {"Name": ["DAPI", "Actin"]}, "SignificantBits": 12, + **(sizes or {})}, + ) # fmt: skip + return root + + +CHANNELS = AxisMapping("CYX", ("C", "X", "Y")) + + +@pytest.mark.parametrize( + "change", + [ + lambda entry: entry.update(channel_names=["DAPI", "Tubulin"]), + lambda entry: entry.update(channel_names=[]), + lambda entry: entry.update(significant_bits=16), + lambda entry: entry.update(significant_bits=None), + ], + ids=["channel name changed", "channel names missing", "bits changed", "bits missing"], +) +def test_a_copy_recorded_with_other_channel_names_or_bits_is_not_reused(tmp_path, change): + exp, copies = channel_experiment(tmp_path / "exp"), tmp_path / "copies" + assert convert_experiment(exp, CHANNELS, copies).written + entry = read_conversion_record(copies)[0].entry("c/img.tif") + assert entry.channel_names == ("DAPI", "Actin") and entry.significant_bits == 12 + edit_record(copies, lambda data: change(data["files"][0])) + again = convert_experiment(exp, CHANNELS, copies) + assert not again.written and codes(again.issues) == ["copy_differs"] + + +def test_a_copy_recorded_with_other_reasons_for_its_unknown_sizes_is_not_reused(tmp_path): + sizes = {"PhysicalSizeX": 0.5, "PhysicalSizeY": 0.7} # not equal: unknown after the swap + exp, copies = channel_experiment(tmp_path / "exp", sizes), tmp_path / "copies" + assert convert_experiment(exp, CHANNELS, copies).written + unknown = read_conversion_record(copies)[0].files[0].unknown_sizes + assert set(unknown) == {"X", "Y"} + assert convert_experiment(exp, CHANNELS, copies).reused == ("c/img.tif",) + + def other_reasons(data): + data["files"][0]["unknown_sizes"] = {role: "another reason" for role in unknown} + + edit_record(copies, other_reasons) + again = convert_experiment(exp, CHANNELS, copies) + assert not again.written and codes(again.issues) == ["copy_differs"] + + +# #80: exact memory sizes ------------------------------------------------------------------- + + +@pytest.mark.parametrize( + ("text", "size"), + [("1.5GiB", 3 * 2**29), ("4096.5KiB", 4096 * 1024 + 512), ("0.5KiB", 512)] + + [("9007199254740993", 2**53 + 1), ("9007199254740993B", 2**53 + 1)], +) +def test_parse_size_is_exact(text, size): + assert parse_size(text) == size and isinstance(parse_size(text), int) + + +@pytest.mark.parametrize("text", ["1.5", "0.5B", "0.5", "1.0000000001KiB", "0GiB", "0.0", "１"]) +def test_parse_size_refuses_a_size_that_is_not_whole_bytes(text): + with pytest.raises(argparse.ArgumentTypeError): + parse_size(text) + + +@pytest.mark.parametrize("command", ["run", "convert"]) +def test_the_commands_refuse_a_bad_memory_size_before_reading_anything( + tmp_path, monkeypatch, capsys, command +): + exp = swapped_experiment(tmp_path / "exp") + for module in (cli_run, cli_convert): + monkeypatch.setattr(module, "preflight", lambda *a, **k: pytest.fail("read")) + args = [command, str(exp), "--axes", "ZXY", "--memory-limit", "1.5"] + if command == "convert": + args += ["--to", str(tmp_path / "copies")] + with pytest.raises(SystemExit) as exit_info: + main(args) + assert exit_info.value.code == 2 + assert "not a whole number of bytes" in capsys.readouterr().err + assert not (tmp_path / "copies").exists() From 792a6c1ae304c5c6fc3b8564118f92081487caf8 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 15:57:01 +0000 Subject: [PATCH 3/4] PP-11c: a copy that cannot be read counts as changed; version check without hashing A copy whose SHA-256 cannot be computed (permission denied, say) is not known to be unchanged, so reuse, review_conversion and RunInputs treat it as changed instead of raising. The record-version check no longer hashes the recorded value (a list there raised TypeError instead of ConversionRecordError). Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- src/morphobricks_preprocessing/content_hash.py | 6 ++++-- src/morphobricks_preprocessing/conversion_record.py | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/morphobricks_preprocessing/content_hash.py b/src/morphobricks_preprocessing/content_hash.py index 8e3bd835..578dcf3c 100644 --- a/src/morphobricks_preprocessing/content_hash.py +++ b/src/morphobricks_preprocessing/content_hash.py @@ -64,7 +64,9 @@ def _raise(exc: OSError) -> None: def unchanged_content(path: str | os.PathLike, expected: str | None) -> bool: - """Return True when a copy is there and its SHA-256 is ``expected``. + """Return True when a copy is there, readable, and its SHA-256 is ``expected``. + + A copy that cannot be read is not known to be unchanged, so it counts as changed. Parameters ---------- @@ -81,5 +83,5 @@ def unchanged_content(path: str | os.PathLike, expected: str | None) -> bool: return False try: return content_sha256(path) == expected - except FileNotFoundError: + except OSError: return False diff --git a/src/morphobricks_preprocessing/conversion_record.py b/src/morphobricks_preprocessing/conversion_record.py index a531fc44..afe92235 100644 --- a/src/morphobricks_preprocessing/conversion_record.py +++ b/src/morphobricks_preprocessing/conversion_record.py @@ -36,7 +36,7 @@ RECORD_VERSION = "2" """Version 2 (PP-11c): each copy's SHA-256, channel names and significant bits.""" -_OLD_VERSIONS = frozenset({"1"}) +_OLD_VERSIONS = ("1",) """Record versions written before the copies' SHA-256: refused, convert again.""" CONVERTED = "converted" From 418bbbd89e87f586eb4138f1aed555377564e44c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 2 Nov 2026 16:53:33 +0000 Subject: [PATCH 4/4] Review PP-11c: never hash a copy in part; the run's warning says the bricks will refuse MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - content_hash: os.walk does not enter a link to a folder, so an OME-Zarr copy holding one was hashed without the files behind it (a change there kept the SHA-256). Such a folder now raises OSError, so the copy counts as changed (the conversion writes no links). - copy_changed_after_conversion: the run goes on, but RunInputs now refuses a run on a changed or added copy, so the warning no longer suggests analysing it; it says the bricks will refuse it and to convert again (contract §17, user guide). Regression tests fail on 792a6c1. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01G3qp9AxjpGPw9xfWAUJNqc --- docs/contract.md | 2 +- docs/user_guide.md | 2 +- .../content_hash.py | 14 ++++++-- .../conversion_record.py | 6 ++-- tests/test_convert_integrity.py | 34 +++++++++++++++++++ 5 files changed, 51 insertions(+), 7 deletions(-) diff --git a/docs/contract.md b/docs/contract.md index 989f2824..c50de113 100644 --- a/docs/contract.md +++ b/docs/contract.md @@ -521,5 +521,5 @@ exclude=, missing_as_singleton=, memory_limit=, progress=)` (`conversion`), the | Safe | The copy tree's lock (`.morphobricks-preprocessing.lock`, §9.2's rule; `--remove-stale-lock`) is held throughout; a second conversion is `output_locked`. Each copy is written as `.partial`, the source's fingerprint checked before and after; once every copy is written they are moved into place, then the record is written (atomic, with the file-mode rule of §9). `not_enough_space` is refused before writing. Any failure or interruption removes the `.partial` files of the conversion, the copies already moved into place (each where nothing was) and the folders it made, so the folder is left as it was; a read or write failure is `conversion_failed`, anything else is raised again; `.partial` files of an interrupted conversion are written again. New files get the mode a plain `open()` gives. | | Repeat | A copy already in the tree is reused when its source (fingerprint and axes), roles, copy axes, pixel sizes, unknown sizes (roles and reasons), channel names, significant bits, the copy's fingerprint and the copy's SHA-256 (read whole) are as recorded; a new image is added; when every copy is reused the record is kept as it was (same SHA-256). Anything else (a source changed, other roles, a copy changed, an image no longer in the experiment, another experiment's copy tree, a file at a new image's place that the record does not list) is `copy_differs`: nothing is overwritten. A conversion record that cannot be read, or of version 1, is `conversion_record_unreadable`. | | Conversion record | `.morphobricks-conversion.json` in the copy tree's root (a dot-file, so discovery skips it), record version 2: `source_root`, `mapping`, `missing_as_singleton`, `written_at`, `versions`, and per image `relative_path`, `action` (`converted` or `copied`), `reason` (the axis code, `excluded`, or null), `source` and `copy` fingerprints (`size`, `mtime_ns`), `copy_sha256` (the copy's SHA-256, computed in one pass after it is written; an OME-Zarr copy's is the SHA-256 of its files' relative paths and SHA-256s in path order, its `labels` group left out as for fingerprints: `content_hash`), `source_axes`, `roles`, `copy_axes`, `pixel_sizes` (by role, as §9), `unknown_sizes` (role: why), `channel_names` (as the copy holds them; `[]` for none) and `significant_bits` (or null). Sources keep the fingerprint rule (design v0.4 §1); the copies' SHA-256 is the owner's decision of 2 November 2026 (plan §12 question 59 (16)). Version 2 (PP-11c) added `copy_sha256`, `channel_names` and `significant_bits`; a version 1 record (v0.4.0rc3 or earlier) is refused everywhere it is read (`convert`, the run's `review_conversion`, `RunInputs`) with a message to convert the originals again into a new folder: its copies cannot be checked. `read_conversion_record(folder)` reads it with its SHA-256. | -| The run on the copies | The user runs preprocessing on the copy tree with the same output folder (`--output`): the recorded experiment root becomes the copy tree and the report says so (`experiment_root_changed`, and `conversion` with the record, its SHA-256 and `source_root`; the text says "named-axis copies of ..."); every row is recomputed (the row key includes the read path, §9); the design, image type and calibration are kept (same relative paths). The manifest records `conversion_record_sha256`. The run and `check` warn when a source changed or went missing after the conversion (`conversion_source_changed`) and when a copy changed (its fingerprint or its SHA-256: each copy is read whole, so `check` and `run` on a copy tree cost one more read of the copies), went missing or was added since (`copy_changed_after_conversion`); an unreadable record stops the run (`conversion_record_unreadable`). `RunInputs` reads the copies like any images, and refuses a run whose conversion record changed or went missing since (`conversion_record_sha256`), is of version 1, or whose recorded images' copies no longer have the recorded fingerprint and SHA-256 (§9.1). | +| The run on the copies | The user runs preprocessing on the copy tree with the same output folder (`--output`): the recorded experiment root becomes the copy tree and the report says so (`experiment_root_changed`, and `conversion` with the record, its SHA-256 and `source_root`; the text says "named-axis copies of ..."); every row is recomputed (the row key includes the read path, §9); the design, image type and calibration are kept (same relative paths). The manifest records `conversion_record_sha256`. The run and `check` warn when a source changed or went missing after the conversion (`conversion_source_changed`) and when a copy changed (its fingerprint or its SHA-256: each copy is read whole, so `check` and `run` on a copy tree cost one more read of the copies), went missing or was added since (`copy_changed_after_conversion`; the run goes on, and its message says that `RunInputs` will refuse the run for a changed or added copy); an unreadable record stops the run (`conversion_record_unreadable`). `RunInputs` reads the copies like any images, and refuses a run whose conversion record changed or went missing since (`conversion_record_sha256`), is of version 1, or whose recorded images' copies no longer have the recorded fingerprint and SHA-256 (§9.1). | | Notebook | Not yet: the axis-check card names the `convert` command; the notebook card is the next PR (PP-11b, plan §10). | diff --git a/docs/user_guide.md b/docs/user_guide.md index adddbe05..b84de9d5 100644 --- a/docs/user_guide.md +++ b/docs/user_guide.md @@ -904,7 +904,7 @@ continues. | `not_enough_space` | Error | `convert`: the copies would not fit on the drive. Free space or choose another drive. | | `conversion_record_unreadable` | Error | The conversion record of a folder of named-axis copies (**.morphobricks-conversion.json**) cannot be read, or is of version 1 (made by v0.4.0rc3 or earlier, without the copies' SHA-256), so `run` stops and `convert` refuses the folder. Convert again into a new folder. | | `conversion_source_changed` | Warning | An original changed or went missing after its named-axis copy was made; the copies hold the images as converted. If the originals were corrected, convert again into a new folder. | -| `copy_changed_after_conversion` | Warning | An image of the folder of named-axis copies changed (its size, time or SHA-256), went missing or was added after the conversion, so the conversion record does not describe it. Convert again, or check it is the image you want. | +| `copy_changed_after_conversion` | Warning | An image of the folder of named-axis copies changed (its size, time or SHA-256), went missing or was added after the conversion, so the conversion record does not describe it. The run goes on, but the bricks (`RunInputs`) refuse a run on such copies: convert the originals again into a new folder. | | `design_unreadable` | Error | The `experiment_design.csv` an earlier run wrote cannot be read. Correct it, or delete it to confirm the design again. | | `no_usable_planes` | Error | No plane has finite pixel values. Check the images. | | `tiff_outside_leaf` | Error | Images sit in a folder that also holds subfolders, so they have no condition or repeat of their own. A folder holds either images or subfolders, never both. Move the images into a folder of their own, or out of the experiment folder. | diff --git a/src/morphobricks_preprocessing/content_hash.py b/src/morphobricks_preprocessing/content_hash.py index 578dcf3c..6d35629a 100644 --- a/src/morphobricks_preprocessing/content_hash.py +++ b/src/morphobricks_preprocessing/content_hash.py @@ -8,7 +8,10 @@ An OME-Zarr copy is hashed as the SHA-256 of its files' relative paths and SHA-256s, in path order. Its ``labels`` group is left out, as :func:`.manifest.source_fingerprint` -leaves it out: label images are masks, recorded and checked one by one by the run. +leaves it out: label images are masks, recorded and checked one by one by the run, and +a brick may add a label image to an OME-Zarr image without changing the image. A folder +holding a link to a folder is not hashed (:class:`OSError`): the files behind the link +would be left out, and a copy is never hashed in part (the conversion writes no links). """ from __future__ import annotations @@ -36,7 +39,8 @@ def content_sha256(path: str | os.PathLike) -> str: Raises ------ OSError - If the file or folder cannot be read (``FileNotFoundError`` when it is gone). + If the file or folder cannot be read (``FileNotFoundError`` when it is gone), or + the folder holds a link to a folder. """ path = Path(path) if not path.is_dir(): @@ -54,6 +58,12 @@ def _folder_files(folder: Path) -> list[str]: if Path(current) == folder and LABELS_GROUP in subfolders: subfolders.remove(LABELS_GROUP) relative = Path(current).relative_to(folder) + # os.walk does not enter a link to a folder, so its files would not be hashed. + linked = [name for name in subfolders if (Path(current) / name).is_symlink()] + if linked: + raise OSError( + f"{folder} holds a link to a folder ({(relative / linked[0]).as_posix()})" + ) found += [(relative / name).as_posix() for name in names] return sorted(found) diff --git a/src/morphobricks_preprocessing/conversion_record.py b/src/morphobricks_preprocessing/conversion_record.py index afe92235..f86d8cc9 100644 --- a/src/morphobricks_preprocessing/conversion_record.py +++ b/src/morphobricks_preprocessing/conversion_record.py @@ -416,10 +416,10 @@ def _copy_issue(paths: list[str]) -> list[ValidationIssue]: "copy_changed_after_conversion", f"{plural(len(paths), 'image')} of this folder of named-axis copies changed, went " "missing or were added after the conversion, so the conversion record does not " - "describe them.", + "describe them. The bricks will refuse a run on them (RunInputs checks every " + "copy the run records against the record).", paths, - "Convert the originals again into a new folder, or check that these images are " - "the ones you want analysed.", + "Convert the originals again into a new folder and run preprocessing on that one.", ) ] diff --git a/tests/test_convert_integrity.py b/tests/test_convert_integrity.py index b17b429f..569d068e 100644 --- a/tests/test_convert_integrity.py +++ b/tests/test_convert_integrity.py @@ -279,3 +279,37 @@ def test_the_commands_refuse_a_bad_memory_size_before_reading_anything( assert exit_info.value.code == 2 assert "not a whole number of bytes" in capsys.readouterr().err assert not (tmp_path / "copies").exists() + + +def test_an_ome_zarr_copy_holding_a_folder_link_counts_as_changed(tmp_path): + # Review of PP-11c: os.walk does not enter a link to a folder, so the files behind it + # would be left out of the SHA-256; such a copy is never hashed in part. + from morphobricks_preprocessing.content_hash import content_sha256, unchanged_content + + zarr = tmp_path / "z.zarr" + (zarr / "0").mkdir(parents=True) + (zarr / "0" / "c").write_bytes(b"a") + (zarr / "zarr.json").write_text("{}") + expected = content_sha256(zarr) + (zarr / "0").rename(tmp_path / "elsewhere") + try: + (zarr / "0").symlink_to(tmp_path / "elsewhere", target_is_directory=True) + except OSError: + pytest.skip("folder links cannot be made here") + with pytest.raises(OSError, match="link to a folder"): + content_sha256(zarr) + assert not unchanged_content(zarr, expected) + + +def test_the_run_warning_about_an_added_copy_says_the_bricks_will_refuse_it(tmp_path): + # Review of PP-11c: the run goes on with the warning, but RunInputs refuses the run, so + # the warning must not suggest that analysing such a copy is an option. + exp, copies, out = swapped_experiment(tmp_path / "exp"), tmp_path / "copies", tmp_path / "o" + assert convert_experiment(exp, SWAPPED, copies).written + (copies / "ctrl" / "r1" / "n.tif").write_bytes((copies / "ctrl" / "r1" / "a.tif").read_bytes()) + run = run_experiment(copies, NAMED, output_folder=out) + found = {issue.code: issue for issue in run.issues}["copy_changed_after_conversion"] + assert found.paths == ("ctrl/r1/n.tif",) and "refuse" in found.message + assert "Convert the originals again" in found.action + with pytest.raises(RunInputsError, match="ctrl/r1/n.tif"): + RunInputs.open(out)