From 991cf92e5b8132cf78832f028a74636f5aa63448 Mon Sep 17 00:00:00 2001 From: Adrian Blumer Date: Tue, 3 Nov 2026 21:43:35 +0100 Subject: [PATCH 1/4] refactor(coordinator): name plan steps Step, with their dependencies The planner now calls its step type Step. It also adds a Plan type that holds the steps. A step has a release field instead of reference. The action "fetch" is now "copy". Each step lists in depends_on the releases of the plan that must be in place before it runs. Part of TNET-157. --- .../src/timenet_connectors/builder/cli.py | 14 +-- .../tests/test_build_plan.py | 43 ++++++--- .../timenet/src/timenet/coordinator/plan.py | 94 ++++++++++++------- .../tests/coordinator/test_plan_types.py | 18 ++++ 4 files changed, 111 insertions(+), 58 deletions(-) create mode 100644 packages/timenet/tests/coordinator/test_plan_types.py diff --git a/packages/timenet-connectors/src/timenet_connectors/builder/cli.py b/packages/timenet-connectors/src/timenet_connectors/builder/cli.py index ffed1d74f..7224fdf55 100644 --- a/packages/timenet-connectors/src/timenet_connectors/builder/cli.py +++ b/packages/timenet-connectors/src/timenet_connectors/builder/cli.py @@ -28,7 +28,7 @@ from timenet.cli.runner import run_cli from timenet.cli.ui import console from timenet.config import settings -from timenet.coordinator.plan import ParentStep, prepare_parents +from timenet.coordinator.plan import Step, prepare_parents from timenet.engine import publish_pipeline, run_pipeline from timenet.errors import ( TimeFValidationError, @@ -219,14 +219,14 @@ def build_one( # ruff: ignore[too-many-arguments] override = None if isolation is None else ("on" if isolation else "off") isolated = settings(isolation=override).isolation == "on" - def build_parent(step: ParentStep) -> object: - builder = find_builder(step.reference.dataset_id) + def build_parent(step: Step) -> object: + builder = find_builder(step.release.dataset_id) if builder is not None and not _in_repo(builder): return _build_with(builder, step, target) # Each parent builds like the dataset itself. An isolated child makes the plan again. The # child finds every parent in the output, so it only opens the parents. return _build( - step.reference.dataset_id, + step.release.dataset_id, target, output, isolated=isolated, @@ -282,7 +282,7 @@ def _in_repo(builder: BuilderBackend) -> bool: return isinstance(builder, ConnectorBuilder) and type(builder).build is ConnectorBuilder.build -def _build_with(builder: BuilderBackend, step: ParentStep, target: WritableRegistry | Path) -> Path: +def _build_with(builder: BuilderBackend, step: Step, target: WritableRegistry | Path) -> Path: """Build a parent through the builder backend that claims it, with the pins of its step. The plan already prepared the parents of this parent. So the backend gets no options for @@ -297,10 +297,10 @@ def _build_with(builder: BuilderBackend, step: ParentStep, target: WritableRegis """ if not isinstance(target, Path): raise TimeFValidationError( - f"parent release {step.reference} builds through another builder backend, which writes to a " + f"parent release {step.release} builds through another builder backend, which writes to a " "local directory; build it into a local output and publish it first" ) - return builder.build(step.reference.dataset_id, target, pinned_by=step.pinned_by or None) + return builder.build(step.release.dataset_id, target, pinned_by=step.pinned_by or None) def _committed(dataset_id: str, target: WritableRegistry | Path, registry: WritableRegistry) -> str | None: diff --git a/packages/timenet-connectors/tests/test_build_plan.py b/packages/timenet-connectors/tests/test_build_plan.py index 1747b5722..5a701ac70 100644 --- a/packages/timenet-connectors/tests/test_build_plan.py +++ b/packages/timenet-connectors/tests/test_build_plan.py @@ -7,7 +7,7 @@ from timenet import builders from timenet.connectors import BaseConnector -from timenet.coordinator.plan import ParentStep, plan_parents, prepare_parents +from timenet.coordinator.plan import Step, plan_parents, prepare_parents from timenet.dataset import Record, RegularAxis, Signal, Source, TimeFDataset from timenet.engine import open_parents, run_pipeline from timenet.errors import ( @@ -93,8 +93,8 @@ def _cards(**parents: dict[str, DatasetRef]) -> dict: return cards -def _no_build(step: ParentStep) -> None: - pytest.fail(f"the plan built {step.reference}") +def _no_build(step: Step) -> None: + pytest.fail(f"the plan built {step.release}") def _install_cards(monkeypatch, **parents: dict[str, DatasetRef]) -> dict: @@ -111,9 +111,9 @@ def _install_cards(monkeypatch, **parents: dict[str, DatasetRef]) -> dict: @pytest.mark.parametrize( ("grand_in", "expected"), [ - ("output", [ParentStep(PARENT, "build")]), - ("origin", [ParentStep(GRAND, "fetch"), ParentStep(PARENT, "build")]), - ("nowhere", [ParentStep(GRAND, "build"), ParentStep(PARENT, "build")]), + ("output", [Step(PARENT, "build")]), + ("origin", [Step(GRAND, "copy"), Step(PARENT, "build", depends_on=(GRAND,))]), + ("nowhere", [Step(GRAND, "build"), Step(PARENT, "build", depends_on=(GRAND,))]), ], ) def test_plan_uses_fetches_or_builds_each_parent_once_before_its_children(tmp_path, monkeypatch, grand_in, expected): @@ -213,7 +213,7 @@ def test_a_declared_parent_needs_only_its_direct_payload_sources(tmp_path, monke # DECLARED opens with SOURCE, its direct payload source, which a fetch of DECLARED brings; GRAND, # SOURCE's own source, is not needed. - assert steps == ([] if where == "output" else [ParentStep(DECLARED, "fetch")]) + assert steps == ([] if where == "output" else [Step(DECLARED, "copy")]) prepare_parents(CHILD.dataset_id, output, build=_no_build, fetch_from=origin) for reference in (DECLARED, SOURCE): assert output.exists(reference.dataset_id, str(reference.version)) @@ -230,7 +230,7 @@ def test_a_payload_source_that_is_later_a_declared_parent_brings_its_own_payload steps = plan_parents(CHILD.dataset_id, output, build_parents=True) - assert steps == [ParentStep(GRAND, "build", pinned_by={SOURCE: pins})] + assert steps == [Step(GRAND, "build", pinned_by={SOURCE: pins})] def test_the_plan_reads_each_card_from_the_backend_that_claims_the_dataset(tmp_path, monkeypatch): @@ -243,7 +243,7 @@ def test_the_plan_reads_each_card_from_the_backend_that_claims_the_dataset(tmp_p steps = plan_parents(CHILD.dataset_id, LocalRegistry(tmp_path), build_parents=True) - assert steps == [ParentStep(GRAND, "build"), ParentStep(PARENT, "build")] + assert steps == [Step(GRAND, "build"), Step(PARENT, "build", depends_on=(GRAND,))] cards[PARENT.dataset_id] = cards[PARENT.dataset_id].model_copy(update={"dataset_version": "2.0.0"}) with pytest.raises(TimeFValidationError, match=r"test/parent@1\.0\.0 is missing .* builds version 2\.0\.0"): plan_parents(CHILD.dataset_id, LocalRegistry(tmp_path), build_parents=True) @@ -306,7 +306,7 @@ def test_a_pinned_source_is_rebuilt_in_staging_and_published_only_if_it_matches( steps = plan_parents(CHILD.dataset_id, output, build_parents=True) - assert steps == [ParentStep(GRAND, "build", pinned_by={PARENT: pins})] + assert steps == [Step(GRAND, "build", pinned_by={PARENT: pins})] def build(step): run_pipeline(_Grand(), output.root, pinned_by=step.pinned_by) @@ -338,7 +338,7 @@ def test_a_payload_source_that_two_releases_read_is_built_against_the_pins_of_bo steps = plan_parents(CHILD.dataset_id, output, build_parents=True) - assert steps == [ParentStep(GRAND, "build", pinned_by=pins)] + assert steps == [Step(GRAND, "build", pinned_by=pins)] def test_a_built_release_needs_only_its_payload_sources(tmp_path, monkeypatch): @@ -351,14 +351,14 @@ def test_a_built_release_needs_only_its_payload_sources(tmp_path, monkeypatch): plan_parents(PARENT.dataset_id, output) # The source that PARENT reads builds against its pins. assert plan_parents(PARENT.dataset_id, output, build_parents=True) == [ - ParentStep(GRAND, "build", pinned_by={PARENT: pins}) + Step(GRAND, "build", pinned_by={PARENT: pins}) ] # A rebuild of PARENT needs its declared parents instead, and pins the new files. - assert plan_parents(PARENT.dataset_id, output, build_parents=True, force_build=True) == [ParentStep(GRAND, "build")] + assert plan_parents(PARENT.dataset_id, output, build_parents=True, force_build=True) == [Step(GRAND, "build")] # A registry that holds the source fills the gap: the local PARENT is reused and GRAND is copied. origin = LocalRegistry(tmp_path / "origin") origin.put_dataset(make_dataset()) - assert plan_parents(PARENT.dataset_id, output, fetch_from=origin) == [ParentStep(PARENT, "fetch")] + assert plan_parents(PARENT.dataset_id, output, fetch_from=origin) == [Step(PARENT, "copy")] prepare_parents(PARENT.dataset_id, output, build=_no_build, fetch_from=origin) assert output.exists(GRAND.dataset_id, str(GRAND.version)) @@ -374,7 +374,7 @@ def test_a_damaged_parent_is_fetched_again_but_never_built_over(tmp_path, monkey plan_parents(CHILD.dataset_id, output, build_parents=True) assert caught.value.release == PARENT - assert plan_parents(CHILD.dataset_id, output, fetch_from=origin) == [ParentStep(PARENT, "fetch")] + assert plan_parents(CHILD.dataset_id, output, fetch_from=origin) == [Step(PARENT, "copy")] prepare_parents(CHILD.dataset_id, output, build=_no_build, fetch_from=origin) assert plan_parents(CHILD.dataset_id, output) == [] @@ -791,3 +791,16 @@ def test_the_cli_hint_names_its_flags(tmp_path, monkeypatch, case): error, hint = hints[case] assert isinstance(result.exception, error) assert str(result.exception).endswith(hint) + + +def test_plan_steps_name_their_dependencies(tmp_path, monkeypatch): + cards = _cards(child={"parent": PARENT}, parent={"grand": GRAND}) + del cards[GRAND.dataset_id] + entries = [_Entry(ConnectorBuilder()), _Entry(_CardBackend(cards))] + monkeypatch.setattr(builders, "entry_points", lambda group: entries) + + steps = plan_parents(CHILD.dataset_id, LocalRegistry(tmp_path), build_parents=True) + + by_release = {step.release: step for step in steps} + assert by_release[GRAND].depends_on == () + assert by_release[PARENT].depends_on == (GRAND,) diff --git a/packages/timenet/src/timenet/coordinator/plan.py b/packages/timenet/src/timenet/coordinator/plan.py index 0cdc59012..1fda73e14 100644 --- a/packages/timenet/src/timenet/coordinator/plan.py +++ b/packages/timenet/src/timenet/coordinator/plan.py @@ -7,7 +7,8 @@ :class:`graphlib.TopologicalSorter` puts each release after the releases that it depends on, and finds cycles. A cycle, or a parent that the plan cannot get, fails here, before any download. If the steps of the plan run in this order, each release that the build opens is in place before the -build starts. The build then only opens these releases. +build starts. The build then only opens these releases. Each step is a :class:`Step`. Its action is +``"copy"`` or ``"build"``, and it names the releases of the plan that it depends on. A payload source is a release whose stored files another release reads. The release that reads them records each file by path, size, and checksum. These records are the pins. If the plan builds @@ -43,19 +44,29 @@ @dataclass(frozen=True) -class ParentStep: - """One release that the plan fetches or builds before the build of the requested release. +class Step: + """One release that the plan copies or builds before the build of the requested release. The output does not hold the release, or it does not hold all payload sources of the release. A - fetch copies the release, and each payload source in its manifest that the output does not hold. + copy takes the release, and each payload source in its manifest that the output does not hold, + from another registry. """ - reference: DatasetRef - action: Literal["fetch", "build"] + release: DatasetRef + action: Literal["copy", "build"] + depends_on: tuple[DatasetRef, ...] = () + """The releases of the plan that must be in place before this step runs.""" pinned_by: Mapping[DatasetRef, PayloadSource] = field(default_factory=dict, hash=False) """For a build: each release that already reads files of this release, with its pins on them.""" +@dataclass(frozen=True) +class Plan: + """The steps that get each release that a build needs, in the order in which they run.""" + + steps: tuple[Step, ...] + + @dataclass(frozen=True) class _Release: """One node of the graph: a release that the build needs, and what the plan does with it.""" @@ -63,8 +74,8 @@ class _Release: opened: bool """True if the build opens the release with its payload sources. False if the release is only a payload source of another release, and the build reads only its pinned files.""" - action: Literal["fetch", "build"] | None - """``"fetch"`` or ``"build"``, or ``None`` if the output holds the release.""" + action: Literal["copy", "build"] | None + """``"copy"`` or ``"build"``, or ``None`` if the output holds the release.""" dependencies: tuple[DatasetRef, ...] """The releases that must be in place before this release. A release to build depends on the parents on its card. An opened release that the plan does not build depends on its payload @@ -83,7 +94,7 @@ class _Graph: releases: dict[DatasetRef, _Release] = field(default_factory=dict) """The nodes, in the order in which the walk found them.""" fetched: set[DatasetRef] = field(default_factory=set) - """The payload sources that the plan fetches only as part of a fetch of the release that reads them.""" + """The payload sources that the plan fetches only as part of a copy of the release that reads them.""" pins: dict[DatasetRef, dict[DatasetRef, PayloadSource]] = field(default_factory=dict) """For each payload source: each release that reads its files, with its pins on them.""" @@ -131,8 +142,8 @@ def visit(self, reference: DatasetRef, needed_by: DatasetRef, pinned: PayloadSou self.visit(source.release, reference, source) else: self.releases[reference] = _Release(opened, action, ()) - # A fetch of the release that reads this payload source also copies the payload source. - if action == "fetch": + # A copy of the release that reads this payload source also copies the payload source. + if action == "copy": self.fetched.add(reference) def order(self) -> list[DatasetRef]: @@ -160,34 +171,43 @@ def order(self) -> list[DatasetRef]: path = " -> ".join(str(reference) for reference in (*cycle, cycle[0])) raise TimeFValidationError(f"dataset parent cycle: {path}") from error - def steps(self, requested: DatasetRef) -> list[ParentStep]: - """Make one step for each release that the plan fetches or builds, in the order of :meth:`order`. + def steps(self, requested: DatasetRef) -> list[Step]: + """Make one step for each release that the plan copies or builds, in the order of :meth:`order`. Args: requested: The requested release. The caller builds it, so it gets no "build" step. If - the output holds it, it gets a "fetch" step under the same rule as an opened parent. + the output holds it, it gets a "copy" step under the same rule as an opened parent. Returns: - The steps, each after the steps of the releases that it depends on. + The steps, each after the steps of the releases that it depends on. ``depends_on`` of a + step holds only the dependencies that have a step in the plan. Raises: TimeFValidationError: If the releases depend on each other in a cycle. """ # ruff: ignore[docstring-extraneous-exception] - order() raises the cycle error - steps: list[ParentStep] = [] + chosen: dict[DatasetRef, Literal["copy", "build"]] = {} for reference in self.order(): release = self.releases[reference] if reference == requested and release.action == "build": continue if release.action == "build": - steps.append(ParentStep(reference, "build", pinned_by=self.pins.get(reference, {}))) - # A fetch of an opened release also copies each payload source that the output does not - # hold. So if the plan fetches a payload source only with the release that reads it, the - # plan fetches that release again, even if the output holds it. + chosen[reference] = "build" + # A copy of an opened release also copies each payload source that the output does not + # hold. So if the plan copies a payload source only with the release that reads it, the + # plan copies that release again, even if the output holds it. elif release.opened and ( - release.action == "fetch" or any(source in self.fetched for source in release.dependencies) + release.action == "copy" or any(source in self.fetched for source in release.dependencies) ): - steps.append(ParentStep(reference, "fetch")) - return steps + chosen[reference] = "copy" + return [ + Step( + reference, + action, + depends_on=tuple(need for need in self.releases[reference].dependencies if need in chosen), + pinned_by=self.pins.get(reference, {}) if action == "build" else {}, + ) + for reference, action in chosen.items() + ] def plan_parents( @@ -197,8 +217,8 @@ def plan_parents( fetch_from: BaseRegistry | None = None, build_parents: bool = False, force_build: bool = False, -) -> list[ParentStep]: - """Plan how to fetch or build the releases a dataset needs. +) -> list[Step]: + """Plan how to get each parent release that a build of ``dataset_id`` needs. Prefer complete copies, then ``fetch_from``, then builds if ``build_parents`` is true. An existing release needs only its payload sources unless ``force_build`` is set. @@ -211,7 +231,8 @@ def plan_parents( force_build: Plan the requested release's parents even if the output already holds the release. Returns: - Fetch or build steps, ordered with dependencies first. + The steps to copy or build parents. Each step comes after the steps of the releases that it + depends on. Raises: LookupError: If no installed connector declares a dataset card for ``dataset_id``. @@ -260,7 +281,7 @@ def prepare_parents( # ruff: ignore[too-many-arguments] dataset_id: str, output: WritableRegistry, *, - build: Callable[[ParentStep], object], + build: Callable[[Step], object], fetch_from: BaseRegistry | None = None, build_parents: bool = False, force_build: bool = False, @@ -285,17 +306,17 @@ def prepare_parents( # ruff: ignore[too-many-arguments] dataset_id, output, fetch_from=fetch_from, build_parents=build_parents, force_build=force_build ) for step in steps: - if step.action == "fetch": - prepare_local_copy(output.local_root, step.reference, fetch_from=fetch_from) + if step.action == "copy": + prepare_local_copy(output.local_root, step.release, fetch_from=fetch_from) else: build(step) # After a successful build, the output must hold the exact release with its payload sources. if isinstance(output, LocalRegistry): - prepare_local_copy(output.root, step.reference) + prepare_local_copy(output.root, step.release) else: - if _manifest(output, step.reference) is None: - raise TimeNetDatasetNotFoundError(f"builder did not publish parent release {step.reference}") - output.open_version(step.reference.dataset_id, str(step.reference.version)) + if _manifest(output, step.release) is None: + raise TimeNetDatasetNotFoundError(f"builder did not publish parent release {step.release}") + output.open_version(step.release.dataset_id, str(step.release.version)) def _manifest(registry: BaseRegistry, reference: DatasetRef) -> Manifest | None: @@ -320,7 +341,7 @@ def _manifest(registry: BaseRegistry, reference: DatasetRef) -> Manifest | None: def _locate( output: WritableRegistry, fetch_from: BaseRegistry | None, reference: DatasetRef -) -> tuple[Manifest | None, Literal["fetch"] | None]: +) -> tuple[Manifest | None, Literal["copy"] | None]: # The damage is raised from a variable, the access error by require_open_access. # ruff: ignore[docstring-extraneous-exception] """Find a release in ``output`` or ``fetch_from``. @@ -328,7 +349,8 @@ def _locate( A local copy is intact when every file has its recorded size. Returns: - ``(manifest, None)`` for an output copy, ``(manifest, "fetch")`` for a fetch, or ``(None, None)`` if absent. + The manifest and ``None`` if the output holds an intact copy. The manifest and ``"copy"`` if + the plan fetches the release. ``None`` and ``None`` if the plan builds it. Raises: TimeNetFileError: If the local copy has a missing or resized file and ``fetch_from`` lacks the release. @@ -356,7 +378,7 @@ def _locate( # So the plan refuses another release before any download. if fetch_from is not None and fetch_from.requires_open_access: require_open_access(fetched.metadata, fetched.lineage) - return fetched, "fetch" + return fetched, "copy" if damage is not None: raise damage return None, None diff --git a/packages/timenet/tests/coordinator/test_plan_types.py b/packages/timenet/tests/coordinator/test_plan_types.py new file mode 100644 index 000000000..684627913 --- /dev/null +++ b/packages/timenet/tests/coordinator/test_plan_types.py @@ -0,0 +1,18 @@ +from dataclasses import FrozenInstanceError + +import pytest + +from timenet.coordinator.plan import Plan, Step +from timenet.types.references import DatasetRef + + +def test_a_step_is_frozen_and_names_its_dependencies(): + step = Step(DatasetRef.parse("a/b@1.0.0"), "copy", depends_on=(DatasetRef.parse("a/c@1.0.0"),)) + with pytest.raises(FrozenInstanceError): + step.action = "build" # ty: ignore[invalid-assignment] + assert step.depends_on == (DatasetRef.parse("a/c@1.0.0"),) + + +def test_a_plan_holds_its_steps_in_order(): + steps = (Step(DatasetRef.parse("a/b@1.0.0"), "copy"), Step(DatasetRef.parse("a/c@1.0.0"), "build")) + assert Plan(steps).steps == steps From 7b1300cd2a720a75bf6482a245caa1d52293548c Mon Sep 17 00:00:00 2001 From: Adrian Blumer Date: Wed, 4 Nov 2026 17:57:41 +0100 Subject: [PATCH 2/4] docs: shorten the docstrings and docs of the plan steps Say only what a caller of the plan steps needs to know. Part of TNET-157. --- .../timenet/src/timenet/coordinator/plan.py | 67 +++++++++---------- 1 file changed, 30 insertions(+), 37 deletions(-) diff --git a/packages/timenet/src/timenet/coordinator/plan.py b/packages/timenet/src/timenet/coordinator/plan.py index 1fda73e14..600f6e928 100644 --- a/packages/timenet/src/timenet/coordinator/plan.py +++ b/packages/timenet/src/timenet/coordinator/plan.py @@ -1,20 +1,7 @@ """Plan and prepare the parent releases that a build needs, before any connector runs. -Each connector card lists its exact parent releases under ``parents``. The planner works in two -phases. First, it walks from the requested release through the cards and the manifests. The walk -finds a graph with one node for each release that the build needs. Each node records what the plan -does with the release, and the releases that it depends on. Then -:class:`graphlib.TopologicalSorter` puts each release after the releases that it depends on, and -finds cycles. A cycle, or a parent that the plan cannot get, fails here, before any download. If -the steps of the plan run in this order, each release that the build opens is in place before the -build starts. The build then only opens these releases. Each step is a :class:`Step`. Its action is -``"copy"`` or ``"build"``, and it names the releases of the plan that it depends on. - -A payload source is a release whose stored files another release reads. The release that reads -them records each file by path, size, and checksum. These records are the pins. If the plan builds -a missing payload source, the build must write the same files. The plan gives the pins to the -build. The build publishes the release only if the files match. Otherwise, it deletes the new files -and raises an error. +A plan is a list of :class:`Step` objects. Each step copies or builds one release, after the steps +of the releases that it depends on. """ from collections.abc import Callable, Mapping @@ -47,9 +34,8 @@ class Step: """One release that the plan copies or builds before the build of the requested release. - The output does not hold the release, or it does not hold all payload sources of the release. A - copy takes the release, and each payload source in its manifest that the output does not hold, - from another registry. + A copy takes the release from another registry, with each payload source in its manifest that + the output does not hold. """ release: DatasetRef @@ -57,7 +43,8 @@ class Step: depends_on: tuple[DatasetRef, ...] = () """The releases of the plan that must be in place before this step runs.""" pinned_by: Mapping[DatasetRef, PayloadSource] = field(default_factory=dict, hash=False) - """For a build: each release that already reads files of this release, with its pins on them.""" + """For a build: each release that already reads files of this release, with its pins on them. + The build must publish the release only if it writes each pinned file again.""" @dataclass(frozen=True) @@ -94,7 +81,7 @@ class _Graph: releases: dict[DatasetRef, _Release] = field(default_factory=dict) """The nodes, in the order in which the walk found them.""" fetched: set[DatasetRef] = field(default_factory=set) - """The payload sources that the plan fetches only as part of a copy of the release that reads them.""" + """The payload sources that the plan copies only with the release that reads them.""" pins: dict[DatasetRef, dict[DatasetRef, PayloadSource]] = field(default_factory=dict) """For each payload source: each release that reads its files, with its pins on them.""" @@ -175,12 +162,12 @@ def steps(self, requested: DatasetRef) -> list[Step]: """Make one step for each release that the plan copies or builds, in the order of :meth:`order`. Args: - requested: The requested release. The caller builds it, so it gets no "build" step. If - the output holds it, it gets a "copy" step under the same rule as an opened parent. + requested: The requested release, which gets no "build" step. If the output holds it, it + gets a "copy" step under the same rule as an opened parent. Returns: The steps, each after the steps of the releases that it depends on. ``depends_on`` of a - step holds only the dependencies that have a step in the plan. + step names only releases that have a step too. Raises: TimeFValidationError: If the releases depend on each other in a cycle. @@ -220,19 +207,19 @@ def plan_parents( ) -> list[Step]: """Plan how to get each parent release that a build of ``dataset_id`` needs. - Prefer complete copies, then ``fetch_from``, then builds if ``build_parents`` is true. - An existing release needs only its payload sources unless ``force_build`` is set. + A build needs each declared parent with its payload sources. The plan copies a release that the + output does not hold from ``fetch_from`` if it can, and otherwise builds it with the installed + connector that declares that exact version. Args: dataset_id: The dataset to build. - output: The registry to build into. - fetch_from: The registry to fetch missing or damaged parents from, or ``None``. + output: The registry that the build writes to. + fetch_from: The registry to copy missing parents from, or ``None`` to copy nothing. build_parents: Build missing parents with their installed connectors. force_build: Plan the requested release's parents even if the output already holds the release. Returns: - The steps to copy or build parents. Each step comes after the steps of the releases that it - depends on. + The steps to copy or build parents, each after the steps of the releases that it depends on. Raises: LookupError: If no installed connector declares a dataset card for ``dataset_id``. @@ -240,8 +227,11 @@ def plan_parents( TimeFValidationError: If parents cycle, a connector builds the wrong version, or fetching uses non-local output. TimeNetMissingParentError: If a parent or payload source is missing and ``build_parents`` is false. TimeNetDatasetNotFoundError: If a parent is missing and no installed connector builds it. - TimeNetFileError: If a local parent copy has a missing or resized file and cannot be fetched again. - TimeNetAccessError: If a release or its lineage lacks open access where a registry requires it. + TimeNetFileError: If a local output holds a damaged copy of a parent, and the plan cannot + copy the parent again. + TimeNetAccessError: If the output is not local and the plan visits a release whose card, or + a card in its lineage, does not declare ``access: open``. Also if ``fetch_from`` is not + local and the plan would copy such a release from it. """ # ruff: ignore[docstring-extraneous-exception] - raised by the helpers of the walk if fetch_from is not None and not isinstance(output, LocalRegistry): # A registry that is not local accepts a child only if it already holds the parents. Also, a @@ -344,17 +334,20 @@ def _locate( ) -> tuple[Manifest | None, Literal["copy"] | None]: # The damage is raised from a variable, the access error by require_open_access. # ruff: ignore[docstring-extraneous-exception] - """Find a release in ``output`` or ``fetch_from``. + """Find an intact copy of a release in the output, or else in ``fetch_from``. - A local copy is intact when every file has its recorded size. + In a local output, a copy with a missing file or a file of another size is damaged. Returns: - The manifest and ``None`` if the output holds an intact copy. The manifest and ``"copy"`` if - the plan fetches the release. ``None`` and ``None`` if the plan builds it. + The manifest and ``None`` if the output holds an intact copy, the manifest and ``"copy"`` if + ``fetch_from`` holds the release, or ``None`` and ``None`` if neither holds it. Raises: - TimeNetFileError: If the local copy has a missing or resized file and ``fetch_from`` lacks the release. - TimeNetAccessError: If the release or its lineage lacks open access where its registry requires it. + TimeNetFileError: If a local output holds a damaged copy, and ``fetch_from`` does not hold + the release. + TimeNetAccessError: If the card of the release, or a card in its lineage, does not declare + ``access: open``, and the output that holds the release, or the ``fetch_from`` registry + to copy it from, is not local. """ manifest = _manifest(output, reference) damage = None From 035949e7219dba25bb90db7731b0c19d7942125b Mon Sep 17 00:00:00 2001 From: Adrian Blumer Date: Wed, 4 Nov 2026 22:31:30 +0100 Subject: [PATCH 3/4] refactor(coordinator): drop the unused Plan type and ban connector imports No code on this branch returns a Plan, so the type goes away until the build coordinator returns one. A ruff rule (banned-api) now refuses an import of timenet_connectors in the code of the timenet package. So the subprocess test that the planner imports nothing from timenet_connectors goes away. The test of the Plan and Step dataclasses and a test that repeated a case of the parametrized plan test go away too. The error for a registry source with an output that is not local now says "copying parents from a registry", the term of the plan steps. Part of TNET-157. --- .../tests/test_build_plan.py | 13 ------------- .../timenet/src/timenet/coordinator/plan.py | 15 +++++---------- .../tests/coordinator/test_plan_imports.py | 7 ------- .../tests/coordinator/test_plan_types.py | 18 ------------------ pyproject.toml | 7 +++++++ 5 files changed, 12 insertions(+), 48 deletions(-) delete mode 100644 packages/timenet/tests/coordinator/test_plan_imports.py delete mode 100644 packages/timenet/tests/coordinator/test_plan_types.py diff --git a/packages/timenet-connectors/tests/test_build_plan.py b/packages/timenet-connectors/tests/test_build_plan.py index 5a701ac70..8b0deaa37 100644 --- a/packages/timenet-connectors/tests/test_build_plan.py +++ b/packages/timenet-connectors/tests/test_build_plan.py @@ -791,16 +791,3 @@ def test_the_cli_hint_names_its_flags(tmp_path, monkeypatch, case): error, hint = hints[case] assert isinstance(result.exception, error) assert str(result.exception).endswith(hint) - - -def test_plan_steps_name_their_dependencies(tmp_path, monkeypatch): - cards = _cards(child={"parent": PARENT}, parent={"grand": GRAND}) - del cards[GRAND.dataset_id] - entries = [_Entry(ConnectorBuilder()), _Entry(_CardBackend(cards))] - monkeypatch.setattr(builders, "entry_points", lambda group: entries) - - steps = plan_parents(CHILD.dataset_id, LocalRegistry(tmp_path), build_parents=True) - - by_release = {step.release: step for step in steps} - assert by_release[GRAND].depends_on == () - assert by_release[PARENT].depends_on == (GRAND,) diff --git a/packages/timenet/src/timenet/coordinator/plan.py b/packages/timenet/src/timenet/coordinator/plan.py index 600f6e928..17e112bb1 100644 --- a/packages/timenet/src/timenet/coordinator/plan.py +++ b/packages/timenet/src/timenet/coordinator/plan.py @@ -47,13 +47,6 @@ class Step: The build must publish the release only if it writes each pinned file again.""" -@dataclass(frozen=True) -class Plan: - """The steps that get each release that a build needs, in the order in which they run.""" - - steps: tuple[Step, ...] - - @dataclass(frozen=True) class _Release: """One node of the graph: a release that the build needs, and what the plan does with it.""" @@ -234,9 +227,11 @@ def plan_parents( local and the plan would copy such a release from it. """ # ruff: ignore[docstring-extraneous-exception] - raised by the helpers of the walk if fetch_from is not None and not isinstance(output, LocalRegistry): - # A registry that is not local accepts a child only if it already holds the parents. Also, a - # fetch copies a parent into the local cache, not into the output. - raise TimeFValidationError("fetching parents needs a local output; publish the parents to the registry first") + # A registry that is not local accepts a child only if it already holds the parents. A copy + # step writes a parent only to a local directory, never to such a registry. + raise TimeFValidationError( + "copying parents from a registry needs a local output; publish the parents to the registry first" + ) card = find_card(dataset_id) if card is None: raise LookupError(f"no installed connector declares a card for {dataset_id!r}") diff --git a/packages/timenet/tests/coordinator/test_plan_imports.py b/packages/timenet/tests/coordinator/test_plan_imports.py deleted file mode 100644 index c0f08c575..000000000 --- a/packages/timenet/tests/coordinator/test_plan_imports.py +++ /dev/null @@ -1,7 +0,0 @@ -import subprocess -import sys - - -def test_the_planner_imports_nothing_from_timenet_connectors(): - code = "import sys, timenet.coordinator.plan; assert 'timenet_connectors' not in sys.modules" - subprocess.run([sys.executable, "-c", code], check=True) diff --git a/packages/timenet/tests/coordinator/test_plan_types.py b/packages/timenet/tests/coordinator/test_plan_types.py deleted file mode 100644 index 684627913..000000000 --- a/packages/timenet/tests/coordinator/test_plan_types.py +++ /dev/null @@ -1,18 +0,0 @@ -from dataclasses import FrozenInstanceError - -import pytest - -from timenet.coordinator.plan import Plan, Step -from timenet.types.references import DatasetRef - - -def test_a_step_is_frozen_and_names_its_dependencies(): - step = Step(DatasetRef.parse("a/b@1.0.0"), "copy", depends_on=(DatasetRef.parse("a/c@1.0.0"),)) - with pytest.raises(FrozenInstanceError): - step.action = "build" # ty: ignore[invalid-assignment] - assert step.depends_on == (DatasetRef.parse("a/c@1.0.0"),) - - -def test_a_plan_holds_its_steps_in_order(): - steps = (Step(DatasetRef.parse("a/b@1.0.0"), "copy"), Step(DatasetRef.parse("a/c@1.0.0"), "build")) - assert Plan(steps).steps == steps diff --git a/pyproject.toml b/pyproject.toml index 7a846ab8a..10c0d2eb3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -66,6 +66,7 @@ select = [ "D", # pydocstyle — docstring presence and formatting "DOC", # pydoclint — args/returns match signatures "PL", # Pylint — error/warning/convention rules (opinionated metrics dropped in `ignore`) + "banned-api", # flake8-tidy-imports (TID251) — the imports banned below ] ignore = [ "line-too-long", # Handled by the formatter @@ -108,10 +109,16 @@ ignore = [ ] # timenet.testing is a test-support subpackage; assert-based comparison helpers are its purpose. "**/src/timenet/testing/*.py" = ["assert"] +# The ban on importing timenet_connectors applies only to the code of the timenet package. +"!packages/timenet/src/**" = ["banned-api"] [tool.ruff.lint.pydocstyle] convention = "google" +[tool.ruff.lint.flake8-tidy-imports.banned-api] +# timenet-connectors depends on timenet, so timenet never imports it and works without it. +"timenet_connectors".msg = "timenet must work without timenet-connectors" + [tool.ruff.lint.flake8-bugbear] # Typer expects Option/Argument descriptors as parameter defaults. extend-immutable-calls = ["typer.Option", "typer.Argument"] From 32ee397e02b16866d2d29df7d99fd5dc6d41a26e Mon Sep 17 00:00:00 2001 From: Adrian Blumer Date: Thu, 5 Nov 2026 00:12:40 +0100 Subject: [PATCH 4/4] docs: shorten the docstrings of the plan steps Shorter docstrings for the code that this change adds or changes. Part of TNET-157. --- .../src/timenet_connectors/builder/cli.py | 22 ++-- .../timenet/src/timenet/coordinator/plan.py | 107 ++++++++---------- 2 files changed, 57 insertions(+), 72 deletions(-) diff --git a/packages/timenet-connectors/src/timenet_connectors/builder/cli.py b/packages/timenet-connectors/src/timenet_connectors/builder/cli.py index 7224fdf55..c5260dc6f 100644 --- a/packages/timenet-connectors/src/timenet_connectors/builder/cli.py +++ b/packages/timenet-connectors/src/timenet_connectors/builder/cli.py @@ -195,21 +195,21 @@ def build_one( # ruff: ignore[too-many-arguments] narrate: bool = False, pinned_by: Mapping[DatasetRef, PayloadSource] | None = None, ) -> str: - """Prepare a dataset's parent releases, then build the dataset. + """Prepare a dataset's parents, then build or reuse its release. - A missing parent that ``fetch_from`` or ``build_parents`` does not provide fails the build before any download. - The build reuses an existing version unless ``force`` is true. + The build fails before downloading if it cannot copy or build a missing parent. + A new release must match every file record in ``pinned_by`` before publication. Args: - dataset_id: The dataset id. - output: The output directory or a ``timenet://``, ``http(s)://``, or ``s3://`` registry URL. + dataset_id: The dataset ID. + output: A local directory or a ``timenet://``, ``http(s)://``, or ``s3://`` registry URL. fetch_from: The registry, or its URI, to copy missing parent releases from. build_parents: Build missing parent releases from their installed connectors. force: Rebuild an existing version. keep_cache: Keep the raw download cache after building. - isolation: Use isolation if true, this interpreter if false, or ``TIMENET_ISOLATION`` if ``None``. + isolation: Use a separate environment if true, this interpreter if false, or ``TIMENET_ISOLATION`` if ``None``. narrate: Print status and download progress when isolation is off. - pinned_by: Releases that read this dataset's files, with file pins the build must match. + pinned_by: The expected file records for this release, grouped by the releases that read them. Returns: The committed version directory for a local output, or the published version. @@ -283,17 +283,15 @@ def _in_repo(builder: BuilderBackend) -> bool: def _build_with(builder: BuilderBackend, step: Step, target: WritableRegistry | Path) -> Path: - """Build a parent through the builder backend that claims it, with the pins of its step. + """Build a parent release with its file pins through the given backend. - The plan already prepared the parents of this parent. So the backend gets no options for - parents. + ``target`` must already hold the parents of ``step.release``. Returns: The committed version directory. Raises: - TimeFValidationError: If the output is not local. A builder backend builds into a local - directory. + TimeFValidationError: If ``target`` is not a local ``Path``. """ if not isinstance(target, Path): raise TimeFValidationError( diff --git a/packages/timenet/src/timenet/coordinator/plan.py b/packages/timenet/src/timenet/coordinator/plan.py index 17e112bb1..fef1a385a 100644 --- a/packages/timenet/src/timenet/coordinator/plan.py +++ b/packages/timenet/src/timenet/coordinator/plan.py @@ -1,7 +1,6 @@ -"""Plan and prepare the parent releases that a build needs, before any connector runs. +"""Plan and prepare the parent releases needed to build a dataset. -A plan is a list of :class:`Step` objects. Each step copies or builds one release, after the steps -of the releases that it depends on. +Each :class:`Step` copies or builds one release after the releases it needs. """ from collections.abc import Callable, Mapping @@ -32,30 +31,31 @@ @dataclass(frozen=True) class Step: - """One release that the plan copies or builds before the build of the requested release. + """A plan step that copies or builds one release. - A copy takes the release from another registry, with each payload source in its manifest that - the output does not hold. + A copy includes the release and its missing payload sources. """ release: DatasetRef action: Literal["copy", "build"] depends_on: tuple[DatasetRef, ...] = () - """The releases of the plan that must be in place before this step runs.""" + """The releases with steps that must finish before this step runs.""" pinned_by: Mapping[DatasetRef, PayloadSource] = field(default_factory=dict, hash=False) - """For a build: each release that already reads files of this release, with its pins on them. - The build must publish the release only if it writes each pinned file again.""" + """The releases that read this release's files, mapped to their expected file records. + + A build must reproduce every recorded file before it publishes the release. + """ @dataclass(frozen=True) class _Release: - """One node of the graph: a release that the build needs, and what the plan does with it.""" + """A required release, its dependencies, and its planned action.""" opened: bool """True if the build opens the release with its payload sources. False if the release is only a payload source of another release, and the build reads only its pinned files.""" action: Literal["copy", "build"] | None - """``"copy"`` or ``"build"``, or ``None`` if the output holds the release.""" + """The planned action, or ``None`` if the output already holds the release.""" dependencies: tuple[DatasetRef, ...] """The releases that must be in place before this release. A release to build depends on the parents on its card. An opened release that the plan does not build depends on its payload @@ -64,7 +64,7 @@ class _Release: @dataclass class _Graph: - """The graph of the releases that a build needs, which the walk of :meth:`visit` finds.""" + """A graph of required releases and their dependencies.""" output: WritableRegistry fetch_from: BaseRegistry | None @@ -74,22 +74,17 @@ class _Graph: releases: dict[DatasetRef, _Release] = field(default_factory=dict) """The nodes, in the order in which the walk found them.""" fetched: set[DatasetRef] = field(default_factory=set) - """The payload sources that the plan copies only with the release that reads them.""" + """The payload sources that the plan copies as part of another release.""" pins: dict[DatasetRef, dict[DatasetRef, PayloadSource]] = field(default_factory=dict) """For each payload source: each release that reads its files, with its pins on them.""" def visit(self, reference: DatasetRef, needed_by: DatasetRef, pinned: PayloadSource | None = None) -> None: - """Add a release to the graph, then visit the releases that it depends on. - - The walk does not look for cycles. It stops at a release that it already found as an opened - release, and at a payload source that it already found. + """Add a release and its dependencies to the graph. Args: - reference: The release. + reference: The release to add. needed_by: The release that needs ``reference``. - pinned: The pins of ``needed_by`` on the files of ``reference``, if ``reference`` is a - payload source of ``needed_by``. ``None`` if the build opens ``reference`` as a - parent. + pinned: The expected file records for a payload source, or ``None`` to open it as a parent. """ opened = pinned is None if pinned is not None: @@ -152,18 +147,18 @@ def order(self) -> list[DatasetRef]: raise TimeFValidationError(f"dataset parent cycle: {path}") from error def steps(self, requested: DatasetRef) -> list[Step]: - """Make one step for each release that the plan copies or builds, in the order of :meth:`order`. + """Create copy and build steps with dependencies first. + + Each step names only dependencies that also have steps. Args: - requested: The requested release, which gets no "build" step. If the output holds it, it - gets a "copy" step under the same rule as an opened parent. + requested: The requested release, which gets a copy step if needed but no build step. Returns: - The steps, each after the steps of the releases that it depends on. ``depends_on`` of a - step names only releases that have a step too. + The steps in dependency order. Raises: - TimeFValidationError: If the releases depend on each other in a cycle. + TimeFValidationError: If the releases form a dependency cycle. """ # ruff: ignore[docstring-extraneous-exception] - order() raises the cycle error chosen: dict[DatasetRef, Literal["copy", "build"]] = {} for reference in self.order(): @@ -198,33 +193,29 @@ def plan_parents( build_parents: bool = False, force_build: bool = False, ) -> list[Step]: - """Plan how to get each parent release that a build of ``dataset_id`` needs. + """Plan the parent releases needed to build a dataset. - A build needs each declared parent with its payload sources. The plan copies a release that the - output does not hold from ``fetch_from`` if it can, and otherwise builds it with the installed - connector that declares that exact version. + Each parent needs its payload sources as well. The plan copies missing releases from + ``fetch_from`` or builds them if ``build_parents`` is true. Args: dataset_id: The dataset to build. - output: The registry that the build writes to. + output: The destination registry. fetch_from: The registry to copy missing parents from, or ``None`` to copy nothing. build_parents: Build missing parents with their installed connectors. - force_build: Plan the requested release's parents even if the output already holds the release. + force_build: Plan a rebuild's parents even if ``output`` holds the requested release. Returns: - The steps to copy or build parents, each after the steps of the releases that it depends on. + The steps to copy or build parents, with dependencies first. Raises: - LookupError: If no installed connector declares a dataset card for ``dataset_id``. - TimeNetInvalidCardError: If an installed connector's dataset card cannot be read. - TimeFValidationError: If parents cycle, a connector builds the wrong version, or fetching uses non-local output. - TimeNetMissingParentError: If a parent or payload source is missing and ``build_parents`` is false. + LookupError: If no installed connector declares a card for ``dataset_id``. + TimeNetInvalidCardError: If reading an installed connector's card fails. + TimeFValidationError: If parents form a cycle, a connector version differs, or copying targets a remote output. + TimeNetMissingParentError: If a required release is missing and ``build_parents`` is false. TimeNetDatasetNotFoundError: If a parent is missing and no installed connector builds it. - TimeNetFileError: If a local output holds a damaged copy of a parent, and the plan cannot - copy the parent again. - TimeNetAccessError: If the output is not local and the plan visits a release whose card, or - a card in its lineage, does not declare ``access: open``. Also if ``fetch_from`` is not - local and the plan would copy such a release from it. + TimeNetFileError: If a local parent copy is damaged and ``fetch_from`` has no replacement. + TimeNetAccessError: If a release or its lineage lacks open access required by a registry. """ # ruff: ignore[docstring-extraneous-exception] - raised by the helpers of the walk if fetch_from is not None and not isinstance(output, LocalRegistry): # A registry that is not local accepts a child only if it already holds the parents. A copy @@ -271,18 +262,18 @@ def prepare_parents( # ruff: ignore[too-many-arguments] build_parents: bool = False, force_build: bool = False, ) -> None: - """Fetch or build each parent release that a build of ``dataset_id`` needs, before the build starts. + """Copy or build a dataset's required parent releases. + + The ``build`` callback must publish each release only if its files match the step's + ``pinned_by`` records. Args: dataset_id: The dataset to build. - output: The registry that the build writes to. - build: Builds one parent into ``output``. For a step with ``pinned_by``, it must publish the - release only if the new files match those pins. - fetch_from: The registry to fetch missing parents from. If it is ``None``, the plan fetches - nothing. + output: The destination registry. + build: The function that builds one parent into ``output``. + fetch_from: The registry to copy missing parents from, or ``None`` to copy nothing. build_parents: Build missing parents with their installed connectors. - force_build: Build the requested release again, even if the output holds it, and plan its - parents. + force_build: Prepare a rebuild's parents even if ``output`` holds the requested release. Raises: TimeNetDatasetNotFoundError: If ``build`` does not publish the release of its step. @@ -329,20 +320,16 @@ def _locate( ) -> tuple[Manifest | None, Literal["copy"] | None]: # The damage is raised from a variable, the access error by require_open_access. # ruff: ignore[docstring-extraneous-exception] - """Find an intact copy of a release in the output, or else in ``fetch_from``. + """Find a release in ``output`` or ``fetch_from``. - In a local output, a copy with a missing file or a file of another size is damaged. + A local output copy is valid only if its files exist and match their recorded sizes. Returns: - The manifest and ``None`` if the output holds an intact copy, the manifest and ``"copy"`` if - ``fetch_from`` holds the release, or ``None`` and ``None`` if neither holds it. + ``(manifest, None)`` from ``output``, ``(manifest, "copy")`` from ``fetch_from``, or ``(None, None)`` if absent. Raises: - TimeNetFileError: If a local output holds a damaged copy, and ``fetch_from`` does not hold - the release. - TimeNetAccessError: If the card of the release, or a card in its lineage, does not declare - ``access: open``, and the output that holds the release, or the ``fetch_from`` registry - to copy it from, is not local. + TimeNetFileError: If a local copy is damaged and ``fetch_from`` has no replacement. + TimeNetAccessError: If a release or its lineage lacks open access required by a registry. """ manifest = _manifest(output, reference) damage = None