Refactor introduce runners

airflow_src/plugins/jobs/docker_job_handler.py

@@ -14,7 +14,7 @@
     - requires the optional requirements in `requirements_docker_job_engine.txt`.
     - the image must already be present on the host, it is never pulled, cf. `_get_image`.
     - requires the bind mount of the docker socket in docker-compose.yaml (cf. `group_add`).
-    - requires key 'locations.general.mounts_path' in alphakraken.{env}.yaml to point to the
+    - requires the environment variable `MOUNTS_PATH` (cf. envs/{env}.env) to point to the
       mounts folder as seen by the docker host.
     - `_SLURM_TIME` is not honored: docker has no wall clock limit.
 

airflow_src/plugins/jobs/job_handler.py

@@ -10,7 +10,7 @@
 from airflow.exceptions import AirflowFailException
 from common.quanting_env import QuantingEnv
 
-from shared.keys import JobEngines
+from shared.keys import EnvVars, JobEngines
 from shared.path_views import CLUSTER_VIEW, DOCKER_HOST_VIEW, Locations
 
 
@@ -33,8 +33,8 @@ def _get_job_handler(engine: str) -> "JobHandler":
 
         if not DOCKER_HOST_VIEW.has(Locations.OUTPUT):
             raise AirflowFailException(
-                f"The '{JobEngines.DOCKER}' job engine requires the key "
-                f"`locations.general.mounts_path` in alphakraken.yaml."
+                f"The '{JobEngines.DOCKER}' job engine requires the environment variable "
+                f"`{EnvVars.MOUNTS_PATH}`, cf. envs/<env>.env."
             )
 
         logging.info("Using DockerJobHandler")

airflow_src/tests/conftest.py

@@ -3,6 +3,7 @@
 import os
 
 os.environ["ENV_NAME"] = "_test_"
+os.environ["MOUNTS_PATH"] = "./tmp/test/mounts"
 
 from collections.abc import Callable
 

airflow_src/tests/plugins/jobs/test_job_handler.py

@@ -49,13 +49,13 @@ def test_get_job_handler_injects_docker_host_view(
 
 @pytest.mark.skipif(not HAS_DOCKER, reason="`docker` not installed")
 @patch("jobs.docker_job_handler.docker.from_env")
-def test_get_job_handler_docker_without_mounts_path(
+def test_get_job_handler_docker_without_mounts_env(
     mock_from_env: MagicMock,  # noqa: ARG001
 ) -> None:
-    """Test that a docker host view without locations points to the missing yaml key."""
+    """Test that a docker host view without locations points to the missing environment variable."""
     with (
         patch.object(DOCKER_HOST_VIEW, "_locations", {}),
-        pytest.raises(AirflowFailException, match="locations.general.mounts_path"),
+        pytest.raises(AirflowFailException, match="MOUNTS_PATH"),
     ):
         _get_job_handler(JobEngines.DOCKER)
 

docker-compose.yaml

@@ -61,6 +61,8 @@ x-airflow-common: &airflow-common
     # See https://airflow.apache.org/docs/apache-airflow/stable/administration-and-deployment/logging-monitoring/check-health.html#scheduler-health-check-server
     # yamllint enable rule:line-length
     AIRFLOW__SCHEDULER__ENABLE_HEALTH_CHECK: "true"
+    # the mounts folder as seen by the docker host, needed by the `docker` job engine
+    MOUNTS_PATH: ${MOUNTS_PATH:?error}
     # The following line can be used to set a custom config file, stored in the local config folder
     # If you want to use it, outcomment it and replace airflow.cfg with the name of your config file
     # AIRFLOW_CONFIG: '/opt/airflow/config/airflow.cfg'

envs/local.env

@@ -34,6 +34,7 @@ REDIS_HOST=redis-service
 REDIS_PORT=6379
 
 # all mounts (backup pool folders, results pool folders, instruments) need to be available in MOUNTS_PATH, cf. Readme
+# Note: must be absolute (if relative, e.g. in local tests, docker job runner will not work)
 MOUNTS_PATH=./local_test/mounts
 
 ## DOCKER ENGINE

envs/production.env

@@ -34,6 +34,7 @@ REDIS_HOST=<redis_host>  # access on another machine
 REDIS_PORT=<port>
 
 # all mounts (backup pool folders, results pool folders, instruments) need to be available in MOUNTS_PATH, cf. Readme
+# Note: must be absolute (if relative, e.g. in local tests, docker job runner will not work)
 MOUNTS_PATH=/home/kraken-user/alphakraken/production/mounts
 
 ## DOCKER ENGINE

envs/sandbox.env

@@ -34,6 +34,7 @@ REDIS_HOST=<redis_host>  # access on another machine
 REDIS_PORT=<port>
 
 # all mounts (backup pool folders, results pool folders, instruments) need to be available in MOUNTS_PATH, cf. Readme
+# Note: must be absolute (if relative, e.g. in local tests, docker job runner will not work)
 MOUNTS_PATH=/home/kraken-user/alphakraken/sandbox/mounts
 
 ## DOCKER ENGINE

shared/keys.py

@@ -28,6 +28,8 @@ class EnvVars(metaclass=ConstantsClass):
 
     KRAKEN_HOSTNAME = "KRAKEN_HOSTNAME"
 
+    MOUNTS_PATH = "MOUNTS_PATH"
+
     MONGO_HOST = "MONGO_HOST"
     MONGO_PORT = "MONGO_PORT"
     MONGO_USER = "MONGO_USER"

shared/path_views.py

@@ -1,9 +1,10 @@
 """Views on the data directories: the same tree, seen from different machines."""
 
+import os
 from pathlib import Path, PurePath, PurePosixPath
 from typing import Generic, TypeVar
 
-from shared.keys import ConstantsClass, InternalPaths
+from shared.keys import ConstantsClass, EnvVars, InternalPaths
 from shared.yamlsettings import YAMLSETTINGS, YamlKeys
 
 
@@ -94,19 +95,17 @@ def _build_docker_host_view() -> View[PurePosixPath]:
     The docker daemon resolves bind mounts in host coordinates, so the paths handed to it need
     to be translated from the container view to this one.
 
-    `locations.general.mounts_path` is required by the `docker` job engine only, so a missing key
-    yields a view without locations rather than an error: it is reported when the view is used.
+    `MOUNTS_PATH` is required by the `docker` job engine only, so an unset variable yields a view
+    without locations rather than an error: it is reported when the view is used.
     """
-    mounts_path = (
-        YAMLSETTINGS.get(YamlKeys.LOCATIONS, {})  # type: ignore[possibly-unbound-attribute]
-        .get(YamlKeys.Locations.GENERAL, {})
-        .get(YamlKeys.Locations.MOUNTS_PATH)
-    )
+    mounts_folder = os.getenv(EnvVars.MOUNTS_PATH)
 
     locations = (
         {}
-        if mounts_path is None
-        else {location: f"{mounts_path}/{location}" for location in _MOUNTED_LOCATIONS}
+        if mounts_folder is None
+        else {
+            location: f"{mounts_folder}/{location}" for location in _MOUNTED_LOCATIONS
+        }
     )
 
     return View("docker host", locations, PurePosixPath)

shared/tests/conftest.py

@@ -3,3 +3,4 @@
 import os
 
 os.environ["ENV_NAME"] = "_test_"
+os.environ["MOUNTS_PATH"] = "./tmp/test/mounts"

shared/tests/test_deployment_paths.py

@@ -49,7 +49,7 @@ def _env_yamls() -> list[tuple[str, dict]]:
 def test_compose_binds_mirror_the_mounts_folder() -> None:
     """Test that each bind puts the data at the same path below the mounts folder on both sides.
 
-    This is what lets the docker host view be derived from `locations.general.mounts_path`.
+    This is what lets the docker host view be derived from `MOUNTS_PATH`.
     """
     for host_path, container_path in _mount_binds():
         rel_path = container_path.removeprefix(InternalPaths.MOUNTS_PATH)

shared/tests/test_path_views.py

@@ -1,10 +1,12 @@
 """Tests for the path_views module."""
 
+import os
 from pathlib import Path, PurePosixPath, PureWindowsPath
 from unittest.mock import patch
 
 import pytest
 
+from shared.keys import EnvVars
 from shared.path_views import (
     AIRFLOW_CONTAINER_VIEW,
     Locations,
@@ -105,7 +107,7 @@ def test_container_view_has_only_the_mounted_locations() -> None:
 def test_cluster_view() -> None:
     """Test that the cluster view is built from the absolute paths in the yaml settings."""
     locations = {
-        YamlKeys.Locations.GENERAL: {YamlKeys.Locations.MOUNTS_PATH: "/some/mounts"},
+        YamlKeys.Locations.GENERAL: {},
         Locations.BACKUP: {YamlKeys.ABSOLUTE_PATH: "/some/pool/backup"},
         Locations.SETTINGS: {YamlKeys.ABSOLUTE_PATH: "/some/pool/settings"},
     }
@@ -124,11 +126,7 @@ def test_cluster_view() -> None:
 
 def test_docker_host_view() -> None:
     """Test that the docker host view mirrors the container view below the host mounts path."""
-    locations = {
-        YamlKeys.Locations.GENERAL: {YamlKeys.Locations.MOUNTS_PATH: "/some/mounts"}
-    }
-
-    with patch.dict(YAMLSETTINGS, {YamlKeys.LOCATIONS: locations}):
+    with patch.dict(os.environ, {EnvVars.MOUNTS_PATH: "/some/mounts"}):
         view = _build_docker_host_view()
 
     assert view.resolve(Locations.OUTPUT, "P1/out_some_file.raw") == PurePosixPath(
@@ -137,9 +135,10 @@ def test_docker_host_view() -> None:
     assert not view.has(Locations.SETTINGS)
 
 
-def test_docker_host_view_without_mounts_path_reaches_nothing() -> None:
-    """Test that a missing mounts path yields an empty view, reported only when it is used."""
-    with patch.dict(YAMLSETTINGS, {YamlKeys.LOCATIONS: {}}):
+def test_docker_host_view_without_mounts_env_reaches_nothing() -> None:
+    """Test that an unset `MOUNTS_PATH` yields an empty view, reported only when it is used."""
+    with patch.dict(os.environ):
+        del os.environ[EnvVars.MOUNTS_PATH]
         view = _build_docker_host_view()
 
     assert not view.has(Locations.OUTPUT)

shared/tests/test_yamlsettings.py

@@ -91,7 +91,6 @@ def test_returns_test_settings_for_test_environment(
             }
         },
         "locations": {
-            "general": {"mounts_path": "./tmp/test/mounts"},
             "settings": {"absolute_path": "./tmp/test/settings"},
             "output": {"absolute_path": "./tmp/test/output"},
             "backup": {"absolute_path": "./tmp/test/backup"},

shared/yamlsettings.py

@@ -31,7 +31,6 @@ class Locations:
         """Keys for accessing paths in the yaml config."""
 
         GENERAL = "general"
-        MOUNTS_PATH = "mounts_path"
 
         BACKUP = "backup"
         SETTINGS = "settings"
@@ -80,7 +79,6 @@ def load_alphakraken_yaml(cls) -> dict[str, dict[str, Any]]:
                     }
                 },
                 "locations": {
-                    "general": {"mounts_path": "./tmp/test/mounts"},
                     "settings": {"absolute_path": "./tmp/test/settings"},
                     "output": {"absolute_path": "./tmp/test/output"},
                     "backup": {"absolute_path": "./tmp/test/backup"},

tasks/plan.md

@@ -0,0 +1,100 @@
+# Implementation Plan: Named runners (D3-lite)
+
+Spec: `SPEC.md` at `d24994dd`. Task list: `tasks/todo.md`.
+Merged from the first plan (`d2a7fca7`) and `tasks/alternative_plan.md`.
+
+## Overview
+
+Replace the single `CLUSTER_VIEW` / `Settings.job_engine` pair by a list of named runners, each
+with engine, os, view and SSH prefix. Dissolve the yaml `locations` block into `runners[].view`,
+`mounts` and `backup.backup_base_path`. Pure refactoring except validation moving to the relative
+parts (spec 1.2.2).
+
+## Strategy
+
+The spec's end state has no fallback, but the path there does not need to be one commit.
+Every task leaves the repo deployable with the in-repo yamls:
+
+1. Four independent foundations land first (`MOUNTS_PATH` env, `backup_base_path`,
+   `_check_content`, `runners:`). Nothing consumes `RUNNERS` yet.
+2. Job dispatch switches to runner names (DB rename).
+3. SSH and path resolution switch to the runner; `CLUSTER_VIEW` goes with its last reader.
+4. `locations` is deleted from the yamls when no code reads it.
+
+Intermediate yamls carry `locations` and `runners` side by side for a few commits. That is
+duplication, not compatibility: no missing key is tolerated at any point.
+
+## Dependency graph
+
+```
+T1 DOCKER_HOST_VIEW from MOUNTS_PATH env      independent
+T2 backup.backup_base_path                    independent
+T3 _check_content explicit list               independent
+T4 shared/runners.py + runners: yaml          needs T2 (consistency assertion backup_base_path == slurm view.backup)
+        │
+T5 QuantingEnv.runner, factory takes Runner   needs T4 (+ T3: adds `runner` to the strict list)
+        └── T6 Settings.runner, webapp        needs T5
+                └── T7 migration              needs T6
+T8 slurm base dir + SSH prefix from runner    needs T5
+T9 prepare_job via runner.view, CLUSTER_VIEW deleted   needs T2, T6, T8 (last readers gone)
+T10 locations -> mounts, mount.sh, keys       needs T1, T9
+T11 docs, comment sweep, §9 greps             needs all
+```
+
+## Architecture decisions
+
+- **One transitional line, in `processor_impl` (T5).** T5 introduces `QuantingEnv.runner` and a
+  factory that takes a `Runner`; `processor_impl` feeds `runner=settings.job_engine` for one
+  commit. Valid because every in-repo runner name equals its engine. T6 removes it. Rejected
+  alternative: passing the runner name as `engine=` into an engine-keyed factory, which leaves
+  the factory wrong for a commit.
+- **DB rename split (T5, T6, T7)** instead of one 12-file commit. The rename changes an exported
+  env var name; the diffs should be reviewable on their own.
+- **`_check_content` first (T3)**, without `runner`; T5 adds it to the list. The validation
+  change is the one behaviour change and gets its own diff. Windows strings must pass it before T9.
+- **SSH prefix plumbed in one step (T8)** once the factory has a `Runner`. No commit passes a
+  module constant into a freshly added parameter.
+- **`CLUSTER_VIEW` deleted in T9**, the commit that removes its last reader. `YamlKeys.LOCATIONS`
+  lingers until T10, unread.
+- **`runner_view(name, **paths)` test helper** (T9) patches `RUNNERS[name].view._locations` the way
+  `yaml_locations` patched `CLUSTER_VIEW`; `yaml_locations` is deleted in the same commit.
+  `test_job_handler.py` constructs `Runner` objects directly (T8) and needs no helper.
+- **Import-time surface grows:** `shared.runners.RUNNERS` (T4) and `yamlsettings.BACKUP_BASE_PATH`
+  (T2) fail at import on an old yaml. The webapp imports `shared.runners` from T6 on, so the
+  webapp container needs the new yaml too. It needs no `MOUNTS_PATH`: an unset variable yields an
+  empty `DOCKER_HOST_VIEW`, an error only when the docker engine is selected.
+- **Import-time raises are tested through the builder functions** (`_build_runners`, a
+  `_read_backup_base_path`-style helper), not by reloading modules. One reload-based test at most
+  for 9.6.
+
+## Phases and checkpoints
+
+| Phase | Tasks | Checkpoint |
+|---|---|---|
+| 1 Foundations | T1, T2, T3, T4 | 1: local yaml loads with `runners`, `backup_base_path` and old `locations` side by side |
+| 2 Dispatch by runner | T5, T6, T7 | 2: 9.1 grep empty; local stack: settings with runner `slurm`, DAG run with `debug_no_cluster_ssh`, `_RUNNER` in the `prepare_job` XCom |
+| 3 Paths from the runner | T8, T9 | 3: 9.4 by test; `grep CLUSTER_VIEW` empty; `prepare_job` XCom unchanged vs. checkpoint 2 |
+| 4 Yaml final shape | T10 | 4: no `locations` key; local stack boots; `mount.sh` fstab line unchanged |
+| 5 Docs | T11 | 5: 9.2 grep clean; TODO count unchanged; three suites and pre-commit green |
+
+Each task is one commit, `pre-commit run --all-files` clean, three `pytest` suites green
+(spec §4; 5 known `test_dags` failures without `AIRFLOW_HOME`).
+
+## Risks and mitigations
+
+| Risk | Impact | Mitigation |
+|---|---|---|
+| T9 changes an exported string by accident | High | The existing full-dict assertion in `test_create_quanting_env` is the 7.3 regression; it changes only in T5 (`_JOB_ENGINE` -> `_RUNNER`) |
+| T6 fan-out misses a test fixture building `Settings` | Med | `grep -rn job_engine` incl. tests is the gate; `pytest -x` per suite |
+| `RUNNERS` / `BACKUP_BASE_PATH` at import: a yaml typo takes down every container incl. webapp | Med | Yamls and stub change in the same commit; consistency test T4 covers the in-repo yamls |
+| `mount.sh` sourcing `envs/${ENV}.env` pulls passwords and `ENV_NAME` into the shell | Med | Source in a subshell, export only `MOUNTS_PATH`; manual fstab diff before/after |
+| Sandbox DB holds `file_based` Settings, no in-repo yaml declares such a runner | Med | Migration prints distinct target names; decide per open question 1 before running it |
+| T5 bridge relies on runner name == engine | Low | One commit; named in the commit message; T6 removes it |
+| `PureWindowsPath / PurePosixPath` join | Low | Already exercised by `test_path_views.py:53-77` |
+
+## Open questions
+
+1. `file_based` Settings in the sandbox DB: remap via `_ENGINE_TO_RUNNER` to `slurm`, or add a
+   `file_based` runner to `alphakraken.sandbox.yaml`? Plan assumes: decide after the `--dry-run`
+   output, no in-repo yaml change.
+2. Spec 11.1, 11.2 stay assumed yes.