From d4768661ba0ea67aa81efff20e676a082e4574d6 Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Fri, 13 Nov 2026 08:20:37 +0000 Subject: [PATCH 1/2] feat: a file is identified by its catalogue identifier, or else by its path relative to the folder FolderFiles gave every file the identity path:, so moving a workspace's folder changed every identity: a trigger loop reduced its runs again, and a table that a batch wrote named files that no longer resolved. The same raw file seen through two file sources had two identities, and provenance named raw files by path, not as the catalogue does. A row with a pid is identified by it, pid:20.500.12269/. Any other file is identified by its path, relative to the given root if it is inside it, absolute otherwise. folder_files passes the workspace's folder as the root; a FolderFiles without one, as with local(), gives absolute paths. LoKI's read_metadata gives the pid of a raw file that carries entry/entry_identifier_uuid, through catalogue_identifier in ess.apps.files. Records hold file identities, so the log's format is 4. Co-Authored-By: Claude Fable 5.2 --- docs/developer/README.md | 3 +- ...a-record-of-another-spec-stays-readable.md | 2 +- ...-identified-by-its-catalogue-identifier.md | 2 +- packages/essapps/docs/batch.md | 4 +- packages/essapps/src/ess/apps/batch/export.py | 3 +- packages/essapps/src/ess/apps/batch/text.py | 7 +- packages/essapps/src/ess/apps/files.py | 77 ++++++++++++++----- packages/essapps/src/ess/apps/folder.py | 9 ++- packages/essapps/src/ess/apps/log.py | 9 ++- packages/essapps/src/ess/apps/protocol.py | 9 ++- packages/essapps/tests/folder_files_test.py | 71 +++++++++++++++++ packages/essapps/tests/folder_test.py | 72 +++++++++++++++-- .../src/ess/instruments/loki/files.py | 14 +++- .../essinstruments/tests/loki/files_test.py | 17 ++++ 14 files changed, 249 insertions(+), 50 deletions(-) diff --git a/docs/developer/README.md b/docs/developer/README.md index 8e040e68..d755c52c 100644 --- a/docs/developer/README.md +++ b/docs/developer/README.md @@ -1,6 +1,6 @@ # ESS data-reduction framework -**Status: `ess.apps.Workspace`, which `local()` and `open_folder()` make in the caller's process, implements this design, including freeze, `select=`, and persist to a fake store or to scipp HDF5 files in a folder (`ess.apps.folder`). A record of another spec than the one bound reads as stored scipp data ([ADR 0009](adr/0009-a-record-of-another-spec-stays-readable.md)). Not implemented: a hosted workspace reached with `connect()`, `client.publish`, software versions in provenance, and catalogue identifiers as file identities ([ADR 0010](adr/0010-a-file-is-identified-by-its-catalogue-identifier.md)). The first release combines no runs and tunes no parameters interactively, so it needs neither stages nor accumulators.** +**Status: `ess.apps.Workspace`, which `local()` and `open_folder()` make in the caller's process, implements this design, including freeze, `select=`, and persist to a fake store or to scipp HDF5 files in a folder (`ess.apps.folder`). A record of another spec than the one bound reads as stored scipp data ([ADR 0009](adr/0009-a-record-of-another-spec-stays-readable.md)). A file is identified by its catalogue identifier, or else by its path ([ADR 0010](adr/0010-a-file-is-identified-by-its-catalogue-identifier.md)). Not implemented: a hosted workspace reached with `connect()`, `client.publish`, and software versions in provenance. The first release combines no runs and tunes no parameters interactively, so it needs neither stages nor accumulators.** This page gives an overview of the framework: its components, what each promises the others, and the ideas that client code is built from. The details live one level down: @@ -113,6 +113,7 @@ It keeps nothing: every client of a workspace reads, references, and releases th Every call that starts work returns once the workspace has *logged* it in its history; the work happens later. **File source.** The workspace finds files, and their metadata, through its *file source*, which is given when the workspace is made. +At submission it resolves a name, such as `file(run=60339)`, to the file's *identity*, which the record holds: `pid:20.500.12269/` for a file the catalogue lists, and else its path, `path:masks/main.xml` relative to the workspace's folder if the file is in it ([ADR 0010](adr/0010-a-file-is-identified-by-its-catalogue-identifier.md)). A client sees only the files the workspace's proposal may read: ```python diff --git a/docs/developer/adr/0009-a-record-of-another-spec-stays-readable.md b/docs/developer/adr/0009-a-record-of-another-spec-stays-readable.md index 30b13aaa..c53dc1f4 100644 --- a/docs/developer/adr/0009-a-record-of-another-spec-stays-readable.md +++ b/docs/developer/adr/0009-a-record-of-another-spec-stays-readable.md @@ -1,6 +1,6 @@ # ADR 0009: A record names its spec by name, version, and digest, and a record of another spec stays readable -- Status: accepted, implemented (`ess.apps.workspace`, log format 3), but for what Open names. +- Status: accepted, implemented (`ess.apps.workspace`, with a new format of the log), but for what Open names. - Deciders: Simon - Date: 2026-11-13 diff --git a/docs/developer/adr/0010-a-file-is-identified-by-its-catalogue-identifier.md b/docs/developer/adr/0010-a-file-is-identified-by-its-catalogue-identifier.md index 6c8531e7..2c226586 100644 --- a/docs/developer/adr/0010-a-file-is-identified-by-its-catalogue-identifier.md +++ b/docs/developer/adr/0010-a-file-is-identified-by-its-catalogue-identifier.md @@ -1,6 +1,6 @@ # ADR 0010: A file is identified by its catalogue identifier, or else by its path -- Status: accepted, not implemented. On `main`, `FolderFiles` gives every file the identity `path:`. +- Status: accepted, implemented (`ess.apps.FolderFiles`, and LoKI's `read_metadata`, which forms the identifier from a raw file), but for what Open names. No file source asks the catalogue: `FolderFiles` takes a file's `pid` from its table. - Deciders: Simon - Date: 2026-11-13 diff --git a/packages/essapps/docs/batch.md b/packages/essapps/docs/batch.md index 9bfcced9..c98d358c 100644 --- a/packages/essapps/docs/batch.md +++ b/packages/essapps/docs/batch.md @@ -202,7 +202,7 @@ A cell that is not text is its value, and `None` or `NaN` is an empty cell, so o **The table.** The first column holds the key of each row; it may also be a blank, as `sample` above. A column named as a blank holds its cells. A column of any other name is info, which comes back as it is; a column named `key`, as `frames.table_of` writes the key, is not info. Two mistakes are refused: a blank that is no column, such as a misspelt header, and a column named as a parameter that is not a blank, whose cells no row would use. An empty cell takes the parameter's default. -**A cell** is read as JSON if it is JSON, so `1.5`, `null`, and `{"x": 0, "y": 1}` are values, and anything else is text. Text that is JSON is written in quotes, such as `"12"` for a sample name at a string parameter; a `.csv` file doubles them, `"""12"""`. At a parameter that is a file field, or a list of them, a whole number names a run, `pid:...` a file by its kind, and other text a path, relative to the working directory. A number with a leading zero, such as `0342`, is not JSON, so it is read as a path: write `342`. The same holds for the values of the template's file. A file field inside a group takes the JSON form, `{"@ref": "file", "file": "run:60339"}`. Every file is one that the workspace's file source knows. +**A cell** is read as JSON if it is JSON, so `1.5`, `null`, and `{"x": 0, "y": 1}` are values, and anything else is text. Text that is JSON is written in quotes, such as `"12"` for a sample name at a string parameter; a `.csv` file doubles them, `"""12"""`. At a parameter that is a file field, or a list of them, a whole number names a run, text with a kind, such as `pid:...` or `path:...`, a file as the record holds it, and other text a path, relative to the working directory. A table that the batch writes holds each file as the record does, by its identity: `pid:20.500.12269/` for a raw file the catalogue lists, and `path:masks/mask.xml`, relative to the workspace's folder, for any other file in it. A number with a leading zero, such as `0342`, is not JSON, so it is read as a path: write `342`. The same holds for the values of the template's file. A file field inside a group takes the JSON form, `{"@ref": "file", "file": "run:60339"}`. Every file is one that the workspace's file source knows. **`reduce`** returns the table with three columns, which it replaces if the table has them: `status`, the ID of the `record`, and `failure`, the reason. So its result can be its next table. With `--remaining` it leaves out the rows whose newest record under the label completed. It compares neither cells nor settings: after changing either, reduce without `--remaining`, or under another label. The `status` of a row is `completed`, `failed`, or `cancelled`. @@ -227,7 +227,7 @@ log.jsonl history store/ every output of every row, /.h5 ``` -`files.csv` is the table of `FolderFiles`, a stand-in for a catalogue: a row per file with its path, relative to the folder, and its metadata as text. A run number in a template or a table is looked up here. +`files.csv` is the table of `FolderFiles`, a stand-in for a catalogue: a row per file with its path, relative to the folder, and its metadata as text. A run number in a template or a table is looked up here. A file with a `pid`, which `read_metadata` gives a raw file that carries the catalogue's identifier, is identified by it, and any other file in the folder by its path relative to the folder. So a folder that is moved, with all it holds, keeps its records, its tables, and its results. ``` file,run,kind,role diff --git a/packages/essapps/src/ess/apps/batch/export.py b/packages/essapps/src/ess/apps/batch/export.py index 9b56c727..a469a51f 100644 --- a/packages/essapps/src/ess/apps/batch/export.py +++ b/packages/essapps/src/ess/apps/batch/export.py @@ -32,7 +32,8 @@ def write_outputs(client: Client, record: Record, directory: Path) -> dict[str, """ Write each output that a completed ``record`` returned to ``directory``, and the record itself, which names the files and the values they were computed - from:: + from. It names a file by its identity: its catalogue identifier, or its + path, relative to the workspace's folder if the file is in it:: iofq.h5 what scipp.io.load_hdf5 reads (see ess.apps.hdf5) _record.json the record diff --git a/packages/essapps/src/ess/apps/batch/text.py b/packages/essapps/src/ess/apps/batch/text.py index a21ecee6..bcd394b1 100644 --- a/packages/essapps/src/ess/apps/batch/text.py +++ b/packages/essapps/src/ess/apps/batch/text.py @@ -16,9 +16,10 @@ A file has no JSON form that a person would write, so at a parameter that is a file field, or a list of them, a file is named by text (see :func:`named_file`):: - 60339 the run 60339 - /data/mask.xml a path - pid:20.500/abc anything else by its kind, as ``FileRef.file`` holds it + 60339 the run 60339 + /data/mask.xml a path + pid:20.500.12269/abc anything else by its kind, as ``FileRef.file`` holds it + path:masks/mask.xml a file of the workspace's folder, by its identity Only a parameter is read this way, not a file field inside a group, which takes the JSON form of a file, as the record holds it. diff --git a/packages/essapps/src/ess/apps/files.py b/packages/essapps/src/ess/apps/files.py index f307bcda..5aba370f 100644 --- a/packages/essapps/src/ess/apps/files.py +++ b/packages/essapps/src/ess/apps/files.py @@ -6,10 +6,13 @@ A person names a file by what they know: a run number, a path, or a PID. The workspace asks its file source to resolve such a name to the file's identity when a request is submitted, so a record names the file itself. +A file the catalogue lists is identified by its catalogue identifier, any +other file by its path (see :class:`FolderFiles`). """ from __future__ import annotations +import os from collections.abc import Iterable, Mapping from pathlib import Path from typing import Any @@ -24,7 +27,9 @@ def file( A file by its run number, its path, or the PID of the SciCat dataset that holds it, as a request writes it. The workspace resolves it to the file's identity when the request is submitted, and the record holds that - identity in place of the name. Exactly one of the three is given:: + identity in place of the name: ``pid:...`` for a file the catalogue lists, + and ``path:...`` for any other (see :class:`FolderFiles`). Exactly one of + the three is given:: file(run=60339) file(path='/home/user/data/run1.h5') @@ -86,6 +91,14 @@ def __repr__(self) -> str: return f'Selector({self.fields})' +def catalogue_identifier(uuid: str) -> str: + """ + The PID under which the catalogue lists the raw file that carries ``uuid`` + in ``entry/entry_identifier_uuid``: ``20.500.12269/``. + """ + return f'20.500.12269/{uuid}' + + def readable(metadata: dict[str, Any], proposal: str) -> bool: """Whether requests of ``proposal`` may read a file with this metadata.""" return metadata.get('proposal', proposal) == proposal @@ -96,17 +109,28 @@ class FolderFiles: A file source over the files of a folder, with a table a person fills. The table has a row per file: ``file``, its name in the folder, and its - metadata, such as ``run``, ``kind``, and ``role``. A row without ``kind`` is - a raw file (see :class:`Selector`), and without ``proposal`` is open to all. - The order of the rows is the order of measurement:: + metadata, such as ``run``, ``kind``, ``role``, and ``pid``. A row without + ``kind`` is a raw file (see :class:`Selector`), and without ``proposal`` is + open to all. The order of the rows is the order of measurement:: FolderFiles('/data/test', [ {'file': 'run-1.nxs', 'run': 1, 'role': 'sample'}, - {'file': 'mask.xml', 'kind': 'mask'}, - ]) + {'file': 'masks/main.xml', 'kind': 'mask'}, + ], root='/data/test') + + A file with a ``pid``, one the catalogue lists, is identified by it, written + in full, so every source that lists the file gives it one identity. Any + other file is identified by its path: relative to ``root`` if it is inside + it, and absolute if not or without ``root``:: + + pid:20.500.12269/ a row with pid 20.500.12269/ + path:masks/main.xml a file inside root + path:/data/other/run-1.nxs a file outside root - A file is identified by its path, and named by its path, its ``run``, or its - ``pid``. + ``root`` is the folder of a workspace (see :func:`ess.apps.folder.folder_files`), + so that moving the folder keeps the identity of each file in it. A file is + named by its identity, its ``run``, its ``pid``, or its path, absolute or + relative to ``root``. Raises ------ @@ -117,33 +141,44 @@ class FolderFiles: """ def __init__( - self, directory: Path | str, table: Iterable[Mapping[str, Any]] + self, + directory: Path | str, + table: Iterable[Mapping[str, Any]], + *, + root: Path | str | None = None, ) -> None: + root = None if root is None else Path(os.path.abspath(root)) + self._paths: dict[FileRef, Path] = {} self._metadata: dict[FileRef, dict[str, Any]] = {} self._names: dict[FileRef, FileRef] = {} for row in table: metadata = dict(row) - path = Path(directory).absolute() / metadata.pop('file') + path = Path(os.path.abspath(Path(directory, metadata.pop('file')))) if not path.is_file(): raise FileNotFoundError(path) - ref = FileRef.at(path) - if ref in self._metadata: + if FileRef.at(path) in self._names: raise ValueError(f'two rows name the file {path}') + if root is not None and path.is_relative_to(root): + local = FileRef.at(path.relative_to(root)) + else: + local = FileRef.at(path) + pid = metadata.get('pid') + ref = local if pid is None else FileRef(file=f'pid:{pid}') + names = {ref, local, FileRef.at(path)} + if 'run' in metadata: + names.add(FileRef(file=f'run:{metadata["run"]}')) + for name in names: + if name in self._names: + raise ValueError(f'two rows give {name}') + self._names[name] = ref + self._paths[ref] = path self._metadata[ref] = metadata - for kind in ('run', 'pid'): - if kind in metadata: - name = FileRef(file=f'{kind}:{metadata[kind]}') - if name in self._names: - raise ValueError(f'two rows give {name}') - self._names[name] = ref def resolve(self, name: FileRef) -> FileRef: - if name in self._metadata: - return name return self._names[name] def path(self, ref: FileRef) -> Path: - return Path(ref.file.removeprefix('path:')) + return self._paths[ref] def metadata(self, ref: FileRef) -> dict[str, Any]: return dict(self._metadata[ref]) diff --git a/packages/essapps/src/ess/apps/folder.py b/packages/essapps/src/ess/apps/folder.py index a2088fc1..f94b884d 100644 --- a/packages/essapps/src/ess/apps/folder.py +++ b/packages/essapps/src/ess/apps/folder.py @@ -6,7 +6,7 @@ The folder holds what a workspace needs to go on after its process ended:: - files.csv the table of FolderFiles: file, run, kind, role, ... + files.csv the table of FolderFiles: file, run, pid, kind, role, ... (written by hand, or started with scan_folder) log.jsonl history: a header naming the format of the log and of the store, then one event per line (see ess.apps.log) @@ -120,11 +120,14 @@ def folder_files(directory: Path) -> FolderFiles: """ The files that ``files.csv`` in ``directory`` lists. A path in it is relative to ``directory``, and an empty cell is metadata that the file - lacks. Raises ``FileNotFoundError`` without ``files.csv``. + lacks. A file in ``directory`` without ``pid`` is identified by its path + relative to it, so moving the folder keeps every identity (see + :class:`~ess.apps.FolderFiles`). Raises ``FileNotFoundError`` without + ``files.csv``. """ with (directory / 'files.csv').open(newline='') as f: table = [{k: v for k, v in row.items() if v != ''} for row in csv.DictReader(f)] - return FolderFiles(directory, table) + return FolderFiles(directory, table, root=directory) def open_folder( diff --git a/packages/essapps/src/ess/apps/log.py b/packages/essapps/src/ess/apps/log.py index a9db8c2b..c6b6dd33 100644 --- a/packages/essapps/src/ess/apps/log.py +++ b/packages/essapps/src/ess/apps/log.py @@ -30,7 +30,7 @@ format. A log file starts with a header line that names its format, such as -``{"format": 3}``, and holds one event per line after it. A log reads only +``{"format": 4}``, and holds one event per line after it. A log reads only files of its own format, :data:`FORMAT`, and leaves a file it refuses as it was. Stages and accumulators are not history, and neither are output values or @@ -163,9 +163,10 @@ class Pushed(BaseModel, frozen=True, extra='forbid'): _event: TypeAdapter[Event] = TypeAdapter(Event) # The format of a log file, named by its first line. Bumped on any change to -# the events, to what a spec's digest covers, or to the files of the store -# (ess.apps.hdf5), since stored values are found through the log. -FORMAT = 3 +# the events, to the identities of files that requests hold (ADR 0010), to +# what a spec's digest covers, or to the files of the store (ess.apps.hdf5), +# since stored values are found through the log. +FORMAT = 4 _HEADER = json.dumps({'format': FORMAT}).encode() + b'\n' diff --git a/packages/essapps/src/ess/apps/protocol.py b/packages/essapps/src/ess/apps/protocol.py index d68277c7..d2d71973 100644 --- a/packages/essapps/src/ess/apps/protocol.py +++ b/packages/essapps/src/ess/apps/protocol.py @@ -456,7 +456,14 @@ class FileSource(Protocol): """ def resolve(self, name: FileRef) -> FileRef: - """The identity of the named file; raises ``KeyError`` if unknown.""" + """ + The identity of the named file; raises ``KeyError`` if unknown. + + A file the catalogue lists is identified by its catalogue identifier, + ``pid:20.500.12269/``, the same in every file source that knows + the file. Any other file is identified by its path (see + :class:`~.files.FolderFiles`). + """ ... def path(self, ref: FileRef) -> Path: diff --git a/packages/essapps/tests/folder_files_test.py b/packages/essapps/tests/folder_files_test.py index d0d7b693..50f730b5 100644 --- a/packages/essapps/tests/folder_files_test.py +++ b/packages/essapps/tests/folder_files_test.py @@ -82,3 +82,74 @@ def test_two_rows_of_one_file_are_refused(folder: Path) -> None: def test_two_rows_of_one_run_or_pid_are_refused(folder: Path, field: str) -> None: with pytest.raises(ValueError, match='two rows give'): FolderFiles(folder, [{'file': 'a.nxs', field: 1}, {'file': 'b.nxs', field: 1}]) + + +def test_without_a_root_a_file_is_identified_by_its_absolute_path( + files: FolderFiles, folder: Path +) -> None: + assert files.resolve(file(run=1)) == FileRef.at(folder / 'a.nxs') + + +@pytest.fixture +def root(tmp_path: Path) -> Path: + """The folder of a workspace, with a run and a mask, next to another folder.""" + root = tmp_path / 'workspace' + (root / 'masks').mkdir(parents=True) + (tmp_path / 'other').mkdir() + for path in ('workspace/run-1.nxs', 'workspace/masks/main.xml', 'other/run-2.nxs'): + (tmp_path / path).touch() + return root + + +@pytest.fixture +def rooted(root: Path) -> FolderFiles: + return FolderFiles( + root, + [ + {'file': 'run-1.nxs', 'run': 1, 'pid': '20.500.12269/abc'}, + {'file': 'masks/main.xml', 'kind': 'mask'}, + {'file': '../other/run-2.nxs', 'run': 2}, + ], + root=root, + ) + + +def test_a_file_is_identified_by_its_pid_or_else_by_its_path_relative_to_the_root( + rooted: FolderFiles, root: Path +) -> None: + assert rooted.list(Selector()) == [ + FileRef(file='pid:20.500.12269/abc'), + FileRef.at(root.parent / 'other' / 'run-2.nxs'), + ] + assert rooted.list(Selector(kind='mask')) == [FileRef(file='path:masks/main.xml')] + + +@pytest.mark.parametrize( + ('name', 'identity', 'path'), + [ + (file(run=1), 'pid:20.500.12269/abc', 'run-1.nxs'), + (file(pid='20.500.12269/abc'), 'pid:20.500.12269/abc', 'run-1.nxs'), + (file(path='run-1.nxs'), 'pid:20.500.12269/abc', 'run-1.nxs'), + (file(path='masks/main.xml'), 'path:masks/main.xml', 'masks/main.xml'), + ], +) +def test_a_name_resolves_to_the_identity_whose_path_is_absolute( + rooted: FolderFiles, root: Path, name: FileRef, identity: str, path: str +) -> None: + ref = rooted.resolve(name) + + assert ref == FileRef(file=identity) + assert rooted.path(ref) == root / path + assert rooted.resolve(file(path=root / path)) == ref + + +def test_one_raw_file_in_two_sources_has_one_identity(tmp_path: Path) -> None: + identities = set() + for name in ('a', 'b'): + (tmp_path / name).mkdir() + (tmp_path / name / 'run-1.nxs').touch() + row = {'file': 'run-1.nxs', 'run': 1, 'pid': '20.500.12269/abc'} + files = FolderFiles(tmp_path / name, [row], root=tmp_path / name) + identities.add(files.resolve(file(run=1))) + + assert identities == {FileRef(file='pid:20.500.12269/abc')} diff --git a/packages/essapps/tests/folder_test.py b/packages/essapps/tests/folder_test.py index 32ba27e8..218e416e 100644 --- a/packages/essapps/tests/folder_test.py +++ b/packages/essapps/tests/folder_test.py @@ -5,14 +5,17 @@ from collections.abc import Callable, Sequence from pathlib import Path +import pandas as pd import pydantic import pytest import scipp as sc -from ess.apps import Client, file +from ess.apps import Client, Record, Rule, Selector, TriggerLoop, file +from ess.apps.batch import Batch, FileInfo, Row +from ess.apps.batch.frames import batch_table from ess.apps.folder import FolderStore, open_folder, scan_folder from ess.apps.testing import read_file -from ess.spec import Binding, WorkflowSpec, function_binding +from ess.spec import Binding, FileRef, WorkflowSpec, function_binding from .batch import toys @@ -50,13 +53,18 @@ def _plain(params: toys.Params) -> toys.Outputs: @pytest.fixture def folder(tmp_path: Path) -> Path: - """A folder of runs 1 to 3, each holding its number.""" - lines = ['file,run,role'] + """ + A folder of runs 1 to 3, each holding its number; the catalogue lists run 3. + """ + folder = tmp_path / 'workspace' + folder.mkdir() + lines = ['file,run,role,pid'] for run in (1, 2, 3): - (tmp_path / f'run-{run}.json').write_text(str(run)) - lines.append(f'run-{run}.json,{run},sample') - (tmp_path / 'files.csv').write_text('\n'.join(lines)) - return tmp_path + (folder / f'run-{run}.json').write_text(str(run)) + pid = '20.500.12269/abc' if run == 3 else '' + lines.append(f'run-{run}.json,{run},sample,{pid}') + (folder / 'files.csv').write_text('\n'.join(lines)) + return folder @pytest.fixture @@ -144,6 +152,54 @@ def test_a_request_of_a_second_session_references_a_persisted_output( assert second.output(total, 'total') == 4.0 +def _find_each(client: Client, ref: FileRef) -> Record: + """Find the offset of a run, as the member of its run under ``found``.""" + (metadata,) = client.files.metadata([ref]) + return client.submit( + toys.FIND, + params={'sample': ref}, + label='found', + member=metadata['run'], + persist=True, + ) + + +def _found(client: Client) -> pd.DataFrame: + """The batch of a row per run, with its result under ``found``, as a table.""" + + def rows(files: Sequence[FileInfo]) -> list[Row]: + return [Row(f.metadata['run'], {'sample': f.ref}) for f in files] + + batch = Batch(client, toys.FIND, columns=['sample'], label='found', source=rows) + batch.scan() + return batch_table(batch, batch.state()) + + +def test_a_moved_folder_keeps_the_identity_of_its_files_and_what_was_done_with_them( + folder: Path, bindings: list[Binding] +) -> None: + rule = Rule('find', _find_each, selector=Selector(), label='found', at='sample') + with open_folder(folder, bindings) as first: + found = TriggerLoop(first, [rule]).step() + first.wait(found) + table = _found(first) + files = [r.request.params['sample'] for r in found] + assert files == [ + FileRef(file='path:run-1.json'), + FileRef(file='path:run-2.json'), + FileRef(file='pid:20.500.12269/abc'), + ] + + with open_folder(folder.rename(folder.with_name('moved')), bindings) as second: + assert second.files.list(Selector()) == files + assert [second.output(r, 'offset').value for r in found] == [1, 2, 3] + assert TriggerLoop(second, [rule]).step() == [] + assert _found(second).equals(table) + again = second.submit(found[0].request) + second.wait([again]) + assert second.output(again, 'offset').value == 1 + + def test_a_folder_held_by_a_client_cannot_be_opened_again( open_client: Callable[[], Client], ) -> None: diff --git a/packages/essinstruments/src/ess/instruments/loki/files.py b/packages/essinstruments/src/ess/instruments/loki/files.py index fcaf03eb..93ddbc8b 100644 --- a/packages/essinstruments/src/ess/instruments/loki/files.py +++ b/packages/essinstruments/src/ess/instruments/loki/files.py @@ -2,7 +2,7 @@ # Copyright (c) 2026 Scipp contributors (https://github.com/scipp) """ What a file of LoKI says of itself, for the table of a workspace in a folder -(see :func:`ess.apps.folder.scan_folder`). It needs the extra ``[loki]``. +(see :func:`ess.apps.folder.scan_folder`). It needs the extra ``[loki-app]``. No file says what a run is for, so ``role`` is left for a person to fill in, and with it the ``role`` of a calibration file, such as ``direct_beam``. @@ -15,6 +15,8 @@ import h5py import numpy as np +from ess.apps.files import catalogue_identifier + _LEADING_RUN = re.compile(r'\d+') @@ -31,11 +33,13 @@ def read_metadata(path: Path) -> dict[str, Any] | None: The metadata of a file, or ``None`` if it is not one of LoKI's:: *.xml {'kind': 'mask'} - HDF5 with an NXentry {'kind': 'raw', 'run': ..., 'sample': ..., 'title': ...} + HDF5 with an NXentry {'kind': 'raw', 'run': ..., 'pid': ..., 'sample': ...} HDF5 written by scipp {'kind': 'calibration'} - The run of a raw file is its ``entry_identifier``. The files of the detector - test at Larmor have none, and are named by their run, ``60339-....nxs``. + The run of a raw file is its ``entry_identifier``, and its ``pid`` the + catalogue identifier formed from its ``entry_identifier_uuid`` (see + :func:`ess.apps.files.catalogue_identifier`). The files of the detector test + at Larmor have neither, and are named by their run, ``60339-....nxs``. """ if path.suffix == '.xml': return {'kind': 'mask'} @@ -46,8 +50,10 @@ def read_metadata(path: Path) -> dict[str, Any] | None: if entry is None or entry.attrs.get('NX_class') != 'NXentry': return {'kind': 'calibration'} if 'scipp-type' in file.attrs else None named = _LEADING_RUN.match(path.name) + uuid = _text(entry, 'entry_identifier_uuid') found = { 'run': _text(entry, 'entry_identifier') or (named and named.group()), + 'pid': uuid and catalogue_identifier(uuid), 'sample': _text(entry, 'sample/name'), 'title': _text(entry, 'title'), } diff --git a/packages/essinstruments/tests/loki/files_test.py b/packages/essinstruments/tests/loki/files_test.py index 0db500fd..b947b456 100644 --- a/packages/essinstruments/tests/loki/files_test.py +++ b/packages/essinstruments/tests/loki/files_test.py @@ -47,6 +47,23 @@ def test_an_entry_identifier_that_is_a_number_gives_the_run_as_its_text( assert read_metadata(path) == {'kind': 'raw', 'run': '342'} +def test_a_file_with_an_entry_uuid_has_the_pid_the_catalogue_lists_it_by( + tmp_path: Path, +) -> None: + path = tmp_path / 'raw.hdf' + with h5py.File(path, 'w') as file: + entry = file.create_group('entry') + entry.attrs['NX_class'] = 'NXentry' + entry['entry_identifier'] = '342' + entry['entry_identifier_uuid'] = '2d1f-7a3c' + + assert read_metadata(path) == { + 'kind': 'raw', + 'run': '342', + 'pid': '20.500.12269/2d1f-7a3c', + } + + def test_an_xml_file_is_a_mask(mask_file: Path) -> None: assert read_metadata(mask_file) == {'kind': 'mask'} From 256f6618f48c7d71c1263422ccd8d3686382216e Mon Sep 17 00:00:00 2001 From: Simon Heybrock Date: Fri, 13 Nov 2026 08:31:37 +0000 Subject: [PATCH 2/2] fix: a written relative path is relative to the working directory; real paths; a pid is written in full file(path=...) and a Path in params are made absolute against the working directory, as the batch's text form reads them; the folder-relative form is only the identity a record holds. FolderFiles identifies a file by its real path, so a folder reached through a symbolic link resolves '../' rows, and looks up an absolute name by its real path. A pid not of the form / is refused. path() and metadata() raise KeyError naming an identity the source does not know, and a record that reads such a file fails with that sentence. Co-Authored-By: Claude Fable 5.2 --- ...-identified-by-its-catalogue-identifier.md | 2 + packages/essapps/docs/batch.md | 2 +- packages/essapps/src/ess/apps/batch/text.py | 2 +- packages/essapps/src/ess/apps/files.py | 54 ++++++++++++++---- packages/essapps/src/ess/apps/protocol.py | 6 +- packages/essapps/src/ess/apps/workspace.py | 14 ++++- packages/essapps/tests/folder_files_test.py | 57 +++++++++++++++++-- packages/essspec/src/ess/spec/validation.py | 5 +- 8 files changed, 119 insertions(+), 23 deletions(-) diff --git a/docs/developer/adr/0010-a-file-is-identified-by-its-catalogue-identifier.md b/docs/developer/adr/0010-a-file-is-identified-by-its-catalogue-identifier.md index 2c226586..16de0328 100644 --- a/docs/developer/adr/0010-a-file-is-identified-by-its-catalogue-identifier.md +++ b/docs/developer/adr/0010-a-file-is-identified-by-its-catalogue-identifier.md @@ -40,8 +40,10 @@ A person still writes `file(run=60339)`. - A moved folder, and a second file source, give a raw file the same identity. - Provenance lists raw files as the catalogue names them. - A file source reads the identifier from a raw file, or gets it from the catalogue. +- A file first listed without its catalogue identifier and later with it changes identity: a trigger loop reduces it again, and the records made before name an identity the file source no longer knows. - Identities are in the log, so changing them changes the log's format ([ADR 0004](0004-history-is-append-only-lists.md#decision)). ## Open - Whether every raw file in user operation carries the identifier, and whether a file the catalogue lists can still be unreadable. To be asked of the teams that run the file writer and the catalogue. +- A catalogue dataset that holds several files cannot be represented, since two rows with one `pid` are refused. Raw files have one file per dataset. diff --git a/packages/essapps/docs/batch.md b/packages/essapps/docs/batch.md index c98d358c..7f52c333 100644 --- a/packages/essapps/docs/batch.md +++ b/packages/essapps/docs/batch.md @@ -249,7 +249,7 @@ run,role,sample ,direct_beam, <- with a column file: a file that is no run ``` -`read_metadata` is a function of the instrument package, from the path of a file to its metadata, or `None` for a file that is not the instrument's (`ess.apps.folder.scan_folder`). The runs come first, by their number, since the order of the rows is the order of measurement. A file that cannot be read is left out, with a warning. Two files of one run are refused. +`read_metadata` is a function of the instrument package, from the path of a file to its metadata, or `None` for a file that is not the instrument's (`ess.apps.folder.scan_folder`). The runs come first, by their number, since the order of the rows is the order of measurement. A file that cannot be read is left out, with a warning. Two files of one run are refused. Running `files` again after files gained a `pid` changes their identity: a trigger loop reduces them again, and earlier records name files the folder no longer knows. A file says its kind, its run, and perhaps its sample, but not what a run is for. A row source that pairs runs reads the `role`, and a field such as the direct beam requires it. The run table gives it (`frames.described`): a row describes the file of its `run`, or the file at its `file` if it has no run. A run of the table that has no file in the folder is ignored, so the table may cover more than the folder holds. A file that no row describes keeps what it says of itself, and where both have a column, a cell of the run table that is not empty wins. Two rows for one run are refused. Without `--with`, the roles are typed into `files.csv`, so `files` then refuses to write over a table that exists. diff --git a/packages/essapps/src/ess/apps/batch/text.py b/packages/essapps/src/ess/apps/batch/text.py index bcd394b1..c9ae32b0 100644 --- a/packages/essapps/src/ess/apps/batch/text.py +++ b/packages/essapps/src/ess/apps/batch/text.py @@ -88,7 +88,7 @@ def named_file(name: Any) -> Any: return name if _KIND.match(name): return FileRef(file=name) - return file(path=Path(name).absolute()) + return file(path=name) def _takes_files(annotation: Any, metadata: tuple[Any, ...]) -> bool: diff --git a/packages/essapps/src/ess/apps/files.py b/packages/essapps/src/ess/apps/files.py index 5aba370f..f8f0c1ea 100644 --- a/packages/essapps/src/ess/apps/files.py +++ b/packages/essapps/src/ess/apps/files.py @@ -35,6 +35,10 @@ def file( file(path='/home/user/data/run1.h5') file(pid='20.500.12269/vanadium') + A relative path is relative to the working directory, and is made absolute + here. A path relative to the workspace's folder is written only as the + identity a record holds, ``FileRef(file='path:masks/main.xml')``. + A string where a file field is expected is refused: write ``file(path=...)``. The record holds no metadata; read it with :meth:`ess.apps.Files.metadata`. """ @@ -43,7 +47,9 @@ def file( if len(named) != 1: raise ValueError(f'name a file by exactly one of {list(given)}') ((kind, value),) = named.items() - return FileRef.at(value) if kind == 'path' else FileRef(file=f'{kind}:{value}') + if kind == 'path': + return FileRef.at(Path(value).absolute()) + return FileRef(file=f'{kind}:{value}') class Selector: @@ -119,9 +125,10 @@ class FolderFiles: ], root='/data/test') A file with a ``pid``, one the catalogue lists, is identified by it, written - in full, so every source that lists the file gives it one identity. Any - other file is identified by its path: relative to ``root`` if it is inside - it, and absolute if not or without ``root``:: + in full, ``/``, so every source that lists the file gives it + one identity. Any other file is identified by its real path, with symbolic + links resolved: relative to the real path of ``root`` if it is inside it, + and absolute if not or without ``root``:: pid:20.500.12269/ a row with pid 20.500.12269/ path:masks/main.xml a file inside root @@ -129,15 +136,16 @@ class FolderFiles: ``root`` is the folder of a workspace (see :func:`ess.apps.folder.folder_files`), so that moving the folder keeps the identity of each file in it. A file is - named by its identity, its ``run``, its ``pid``, or its path, absolute or - relative to ``root``. + named by its identity, its ``run``, its ``pid``, or its absolute path, also + one through a symbolic link. Raises ------ FileNotFoundError If the folder lacks a file of the table. ValueError - If two rows name one file, or give one run or one PID. + If two rows name one file, give one run or one PID, or a PID is not + written in full. """ def __init__( @@ -147,13 +155,13 @@ def __init__( *, root: Path | str | None = None, ) -> None: - root = None if root is None else Path(os.path.abspath(root)) + root = None if root is None else Path(os.path.realpath(root)) self._paths: dict[FileRef, Path] = {} self._metadata: dict[FileRef, dict[str, Any]] = {} self._names: dict[FileRef, FileRef] = {} for row in table: metadata = dict(row) - path = Path(os.path.abspath(Path(directory, metadata.pop('file')))) + path = Path(os.path.realpath(Path(directory, metadata.pop('file')))) if not path.is_file(): raise FileNotFoundError(path) if FileRef.at(path) in self._names: @@ -163,6 +171,12 @@ def __init__( else: local = FileRef.at(path) pid = metadata.get('pid') + if pid is not None and not _written_in_full(str(pid)): + raise ValueError( + f'the pid {pid!r} of {path} is not written in full: a catalogue ' + 'identifier is written with its prefix, such as ' + '20.500.12269/' + ) ref = local if pid is None else FileRef(file=f'pid:{pid}') names = {ref, local, FileRef.at(path)} if 'run' in metadata: @@ -175,13 +189,31 @@ def __init__( self._metadata[ref] = metadata def resolve(self, name: FileRef) -> FileRef: + kind, _, value = name.file.partition(':') + if kind == 'path' and os.path.isabs(value): + name = FileRef.at(os.path.realpath(value)) return self._names[name] def path(self, ref: FileRef) -> Path: - return self._paths[ref] + return self._paths[self._known(ref)] def metadata(self, ref: FileRef) -> dict[str, Any]: - return dict(self._metadata[ref]) + return dict(self._metadata[self._known(ref)]) + + def _known(self, ref: FileRef) -> FileRef: + """ + ``ref``, if it is the identity of a file of the table. A record may hold + one that is not, of a file moved or given a ``pid`` since it was made. + """ + if ref not in self._paths: + raise KeyError(f'the file source does not know the file {ref.file}') + return ref def list(self, selector: Selector) -> list[FileRef]: return [r for r, m in self._metadata.items() if selector.matches(m)] + + +def _written_in_full(pid: str) -> bool: + """Whether ``pid`` has the form ``/`` of a catalogue identifier.""" + prefix, slash, suffix = pid.partition('/') + return bool(prefix and slash and suffix) diff --git a/packages/essapps/src/ess/apps/protocol.py b/packages/essapps/src/ess/apps/protocol.py index d2d71973..0749ca9a 100644 --- a/packages/essapps/src/ess/apps/protocol.py +++ b/packages/essapps/src/ess/apps/protocol.py @@ -462,14 +462,16 @@ def resolve(self, name: FileRef) -> FileRef: A file the catalogue lists is identified by its catalogue identifier, ``pid:20.500.12269/``, the same in every file source that knows the file. Any other file is identified by its path (see - :class:`~.files.FolderFiles`). + :class:`~.files.FolderFiles`). An identity is not a path: only + :meth:`path` gives the path of a file. """ ... def path(self, ref: FileRef) -> Path: """ A local path to the file, for a workflow to read: a lookup by - identity that never raises. + identity. Raises ``KeyError`` for an identity the source does not know, + such as one a record holds of a file moved or given a ``pid`` since. The files are on a network mount, and the workspace calls ``path`` for each file of a request when the request is submitted, so that the diff --git a/packages/essapps/src/ess/apps/workspace.py b/packages/essapps/src/ess/apps/workspace.py index 43f8e850..c8f5b851 100644 --- a/packages/essapps/src/ess/apps/workspace.py +++ b/packages/essapps/src/ess/apps/workspace.py @@ -168,6 +168,7 @@ from concurrent.futures import ThreadPoolExecutor from dataclasses import replace from datetime import UTC, datetime +from pathlib import Path from typing import Any from pydantic import BaseModel @@ -1145,13 +1146,24 @@ def _typed(self, spec_id: SpecId, values: Mapping[str, Any]) -> dict[str, Any]: driver takes them by name. """ spec = self._bindings[spec_id].spec - paths = validation.local_paths(values, self._files.path) + paths = validation.local_paths(values, self._path) context = {'read': self._reader(values), 'paths': paths} return { n: validation.validated(spec.params, n, v, n in spec.tables, context) for n, v in values.items() } + def _path(self, ref: FileRef) -> Path: + """ + The local path of a file; no lock. A record may hold the identity of a + file the source no longer knows, moved or given a ``pid`` since the + record was made, which fails the record. + """ + try: + return self._files.path(ref) + except KeyError: + raise _Failure(f'the file source does not know the file {ref}') from None + def _staged(self, stage: _Stage, make: Callable[[], StageCall]) -> StageCall: """ The stage's callable; ``make`` stages the binding, outside the diff --git a/packages/essapps/tests/folder_files_test.py b/packages/essapps/tests/folder_files_test.py index 50f730b5..5f5169f8 100644 --- a/packages/essapps/tests/folder_files_test.py +++ b/packages/essapps/tests/folder_files_test.py @@ -78,10 +78,35 @@ def test_two_rows_of_one_file_are_refused(folder: Path) -> None: FolderFiles(folder, [{'file': 'a.nxs'}, {'file': 'a.nxs'}]) -@pytest.mark.parametrize('field', ['run', 'pid']) -def test_two_rows_of_one_run_or_pid_are_refused(folder: Path, field: str) -> None: +@pytest.mark.parametrize(('field', 'value'), [('run', 1), ('pid', 'x/1')]) +def test_two_rows_of_one_run_or_pid_are_refused( + folder: Path, field: str, value: object +) -> None: + rows = [{'file': 'a.nxs', field: value}, {'file': 'b.nxs', field: value}] with pytest.raises(ValueError, match='two rows give'): - FolderFiles(folder, [{'file': 'a.nxs', field: 1}, {'file': 'b.nxs', field: 1}]) + FolderFiles(folder, rows) + + +@pytest.mark.parametrize('pid', ['2d1f-7a3c', '/2d1f-7a3c', '20.500.12269/']) +def test_a_pid_not_written_in_full_is_refused(folder: Path, pid: str) -> None: + with pytest.raises(ValueError, match=r'a\.nxs is not written in full'): + FolderFiles(folder, [{'file': 'a.nxs', 'pid': pid}]) + + +def test_a_relative_path_is_relative_to_the_working_directory( + files: FolderFiles, folder: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.chdir(folder) + assert file(path='mask.xml') == FileRef.at(folder / 'mask.xml') + assert files.path(files.resolve(file(path='mask.xml'))) == folder / 'mask.xml' + + +@pytest.mark.parametrize('lookup', ['path', 'metadata']) +def test_an_identity_the_source_does_not_know_raises_keyerror( + files: FolderFiles, lookup: str +) -> None: + with pytest.raises(KeyError, match=r'does not know the file path:gone\.nxs'): + getattr(files, lookup)(FileRef(file='path:gone.nxs')) def test_without_a_root_a_file_is_identified_by_its_absolute_path( @@ -129,8 +154,8 @@ def test_a_file_is_identified_by_its_pid_or_else_by_its_path_relative_to_the_roo [ (file(run=1), 'pid:20.500.12269/abc', 'run-1.nxs'), (file(pid='20.500.12269/abc'), 'pid:20.500.12269/abc', 'run-1.nxs'), - (file(path='run-1.nxs'), 'pid:20.500.12269/abc', 'run-1.nxs'), - (file(path='masks/main.xml'), 'path:masks/main.xml', 'masks/main.xml'), + (FileRef.at('run-1.nxs'), 'pid:20.500.12269/abc', 'run-1.nxs'), + (FileRef.at('masks/main.xml'), 'path:masks/main.xml', 'masks/main.xml'), ], ) def test_a_name_resolves_to_the_identity_whose_path_is_absolute( @@ -153,3 +178,25 @@ def test_one_raw_file_in_two_sources_has_one_identity(tmp_path: Path) -> None: identities.add(files.resolve(file(run=1))) assert identities == {FileRef(file='pid:20.500.12269/abc')} + + +@pytest.fixture +def linked(tmp_path: Path) -> Path: + """A workspace's folder reached through a symbolic link, next to another.""" + (tmp_path / 'real' / 'workspace').mkdir(parents=True) + (tmp_path / 'real' / 'other').mkdir() + (tmp_path / 'real' / 'workspace' / 'run-1.nxs').touch() + (tmp_path / 'real' / 'other' / 'run-2.nxs').touch() + (tmp_path / 'link').symlink_to(tmp_path / 'real' / 'workspace') + return tmp_path / 'link' + + +def test_a_path_through_a_symbolic_link_is_the_real_path(linked: Path) -> None: + rows = [{'file': 'run-1.nxs', 'run': 1}, {'file': '../other/run-2.nxs', 'run': 2}] + files = FolderFiles(linked, rows, root=linked) + other = linked.resolve().parent / 'other' / 'run-2.nxs' + + assert files.resolve(file(run=1)) == FileRef(file='path:run-1.nxs') + assert files.resolve(file(run=2)) == FileRef.at(other) + assert files.path(files.resolve(file(run=2))) == other + assert files.resolve(file(path=linked / 'run-1.nxs')) == files.resolve(file(run=1)) diff --git a/packages/essspec/src/ess/spec/validation.py b/packages/essspec/src/ess/spec/validation.py index 4b23d798..2722d426 100644 --- a/packages/essspec/src/ess/spec/validation.py +++ b/packages/essspec/src/ess/spec/validation.py @@ -171,10 +171,11 @@ def validate_request_value( def _paths_as_files(value: Any) -> Any: """ ``value``, plain data, with each ``Path`` in it the file named by that - path, ``file(path=...)`` (see :meth:`~ess.spec.refs.FileRef.at`). + path, ``file(path=...)`` (see :meth:`~ess.spec.refs.FileRef.at`). A + relative path is relative to the working directory. """ if isinstance(value, PurePath): - return FileRef.at(value).model_dump(by_alias=True) + return FileRef.at(Path(value).absolute()).model_dump(by_alias=True) if isinstance(value, dict): return {k: _paths_as_files(v) for k, v in value.items()} if isinstance(value, list | tuple):