diff --git a/SPEC.md b/SPEC.md new file mode 100644 index 00000000..946b554d --- /dev/null +++ b/SPEC.md @@ -0,0 +1,445 @@ +# Spec: Named runners (D3-lite) + +Commit: `e0561ecf` (branch `path_refactoring_4`). +Follows `design_docs/PATH_HANDLING_DESIGN.md` option D3, cut down. D2 is landed. + +## 1. Objective + +One deployment runs quanting jobs on several *named runners*. A runner is a configured place +where jobs execute: an engine (how), a view (which absolute paths the job sees, in which path +flavour), and the SSH connections to reach it. Hosts whose OS and path flavour differ from the +Airflow worker's (Windows: UNC or drive letter) must be expressible. + +This is an upfront refactoring. It prepares the seam for an SSH job handler; it does not add +that handler. No exported string changes for existing deployments, except where §5 says so. + +### 1.1 What is wrong today + +- 1.1.1 `Settings.job_engine` names *how* (slurm/docker/file_based), never *where*. Two Slurm + clusters, or one cluster plus one Windows box, cannot be told apart + (`shared/db/models.py:272`). +- 1.1.2 `prepare_job` resolves every exported path through the single `CLUSTER_VIEW`, whatever + the engine (`airflow_src/dags/impl/processor_impl.py:123,174,184,266`). +- 1.1.3 SSH connections are found by one global prefix `cluster_ssh_connection` + (`airflow_src/plugins/common/utils.py:158-168`). +- 1.1.4 `_check_content` rejects every `\` and `:`, so no Windows path can be exported + (`processor_impl.py:273-320`, `shared/validation.py:14`). + +### 1.2 Decisions taken (2026-09-02) + +- 1.2.1 `Settings.job_engine` is renamed to `Settings.runner`, with a one-shot DB migration. +- 1.2.2 Validation moves to the user-controlled relative parts; resolved bases count as admin + configuration. This is the one deliberate behaviour change. +- 1.2.3 Every runner declares its complete `view`. There is no default view and no fallback. + `locations..absolute_path` is removed; the one non-job reader of it, the persisted display + path in `get_backup_base_path`, reads a new key `backup.backup_base_path` in the existing + top-level `backup` block. + `CLUSTER_VIEW` is deleted. (Supersedes the earlier fallback decision, 2026-09-02.) +- 1.2.4 A yaml without `runners:` fails at import with a clear error. No implicit defaults. +- 1.2.5 The docker runner keeps binding host paths onto the paths the placeholders resolved to; + admins copy the old `absolute_path` values into its `view` to keep exported strings equal. +- 1.2.6 With `absolute_path` gone, the per-location entries hold mount information only. They + move to a new top-level `mounts.` block. The `locations` block disappears entirely. Entries + that had only an `absolute_path` (`settings`, `software`, `slurm`) are not carried over. +- 1.2.7 `locations.general.mounts_path` is dropped. It duplicated `MOUNTS_PATH` from + `envs/.env`, and the two already disagree in the local environment. `MOUNTS_PATH` is + passed into the containers instead and read from the environment. It must be absolute when the + docker runner is used, because the Docker daemon rejects relative bind sources; this is + documented as a comment in the `.env` files, not enforced. + +## 2. Design + +### 2.1 Yaml + +```yaml +backup: # existing block + backup_type: local + purging_verification_type: local + backup_base_path: /fs/pool-0/alphakraken/backup # persisted for display, cf. 2.2 + s3: ... + +mounts: # read by mount.sh and the consistency test only; the mounts folder itself is MOUNTS_PATH in .env + backup: + username: user + mount_src: //mount_src/backup + mount_target: backup + output: ... + logs: ... + # settings, software, slurm are not mounted and do not appear here + +runners: + - name: slurm # referenced by Settings.runner; unique within the list + engine: slurm # one of shared.keys.JobEngines + os: linux # required; linux | macos | windows, determines the path flavour of `view` + ssh_connection_id_prefix: cluster_ssh_connection # required for engines that use SSH + view: # the data directories as seen from this runner, complete + backup: /fs/pool-0/alphakraken/backup + output: /fs/pool-0/alphakraken/output + settings: /fs/pool-0/alphakraken/settings + software: /fs/home/kraken-read/software + slurm: /fs/pool-0/alphakraken/slurm + - name: docker + engine: docker + os: linux + # no ssh_connection_id_prefix: the docker engine does not use SSH, the key would be ignored + view: # paths inside the job container, cf. 1.2.5 + backup: /fs/pool-0/alphakraken/backup + output: /fs/pool-0/alphakraken/output + settings: /fs/pool-0/alphakraken/settings + software: /fs/home/kraken-read/software + - name: win_box # illustrative, not in the in-repo yamls + engine: slurm # `ssh` once that handler exists; the factory rejects unknown engines. + # NOTE: this entry validates but cannot run: the slurm handler builds + # bash and gets a windows path. Illustrates the yaml shape only. + os: windows + ssh_connection_id_prefix: win_box_ssh + view: + backup: '\\server\share\backup' + output: 'Z:\alphakraken\output' + settings: 'Z:\alphakraken\settings' + software: 'C:\alphakraken\software' +``` + +- `runners` is a list; each entry carries its `name`. Order is the display order in the webapp. + +- Per-runner `view` is a flat `location -> path` map (no `absolute_path` sub-key: there is + no mount information at this level). Values are opaque strings; the flavour comes from `os`. + The key is named `view`, not `locations`, to match `Runner.view` and the `View` class. + Two runners on one file system repeat the paths; yaml anchors are available if that hurts. +- A location a runner does not declare is unreachable and fails at `View.resolve` with the + existing error naming view and location. +- `mounts.` is not read by any Python view. `DOCKER_HOST_VIEW` derives from the + `MOUNTS_PATH` environment variable (`EnvVars.MOUNTS_PATH`, passed through the + `airflow-common-env` block of `docker-compose.yaml`) plus the fixed location names. A missing + variable yields an empty view, reported by the factory when the docker engine is selected, as + today. +- All three in-repo yamls (`envs/alphakraken.{local,sandbox,production}.yaml`) move their + `absolute_path` values into a `slurm` and a `docker` runner and gain `backup_base_path` with + the value of the former `locations.backup.absolute_path`, so the migration in 2.8 maps 1:1. + `view.backup` of the `slurm` runner holds the same value; the consistency test (2.9) asserts + the equality. The `_test_` stub in `shared/yamlsettings.py:73-90` gets `slurm`, `docker`, + `file_based` with the current test paths, gains + `backup: {backup_base_path: ./tmp/test/backup}` (today's `locations.backup.absolute_path`) and + loses `general.mounts_path`; the test conftests set `MOUNTS_PATH` in the environment next to + `ENV_NAME`. + +### 2.2 `shared/runners.py` (new) + +```python +class OperatingSystems(metaclass=ConstantsClass): + LINUX = "linux" + MACOS = "macos" + WINDOWS = "windows" + + +@dataclass(frozen=True) +class Runner: + """A configured place where jobs execute.""" + + name: str + engine: str + os: str # one of OperatingSystems; kept for the future SSH handler (job script per OS) + view: View[PurePath] + ssh_connection_id_prefix: str | None # optional in yaml; engines that need it check for None + + +def _build_runners(entries: list[dict]) -> dict[str, Runner]: + """Validate the yaml `runners` list and build the runners, order kept.""" + + +RUNNERS: dict[str, Runner] = _build_runners(YAMLSETTINGS[YamlKeys.RUNNERS]) # keyed by name + + +def get_runner(name: str) -> Runner: + """Raises KeyError naming the known runners.""" +``` + +- Built at import, like the views in `shared/path_views.py`. All validation lives in + `_build_runners`, which the tests call directly (7.1). Import-time validation: list + present and non-empty, every entry has a `name`, names unique, `engine` in `JobEngines`, `os` + present and in `OperatingSystems`, `view` present and every key of it in `Locations`. + `ssh_connection_id_prefix` is optional here and not interpreted: the loader knows nothing about + which engines use SSH. Which locations an engine needs is likewise not checked here; a missing + one fails at first use via `View.resolve`, naming view and location. No key has a default. Each + failure names the runner and the yaml key. +- `os: linux` and `os: macos` -> `PurePosixPath`, `os: windows` -> `PureWindowsPath`. `macos` + exists for completeness and is treated exactly like `linux`. Never `Path`: no code does + filesystem I/O in a runner view. +- `view` is required for every runner; `Runner.view = View(name, yaml_view, path_class)`. +- `CLUSTER_VIEW` and `_build_cluster_view` are deleted from `shared/path_views.py`. Its one + non-job reader, `handler_impl.get_backup_base_path`, becomes + `PurePosixPath(BACKUP_BASE_PATH) / get_raw_file_folder_rel_path(raw_file)` with + `BACKUP_BASE_PATH` read from `backup.backup_base_path` in `shared/yamlsettings.py`, + missing key raising at import like the runners do. +- `CLUSTER_SSH_CONNECTION_ID_PREFIX` (`common/constants.py:6`) is deleted without replacement; + the prefix always comes from the runner. The error text in `get_cluster_ssh_hook` names the + prefix it was given. +- Lives in `shared` because the webapp needs the runner names and engines (2.7). + +### 2.3 Settings and QuantingEnv + +- `Settings.runner = StringField(required=True, max_length=64)`, no default. `job_engine` is + deleted. `create_settings(runner=...)`. +- `QuantingEnv.job_engine` -> `runner: str = Field(alias="_RUNNER")`. Underscore prefix, so it + is never exported to a job. + +### 2.4 prepare_job + +`runner = get_runner(settings.runner)`; the four `CLUSTER_VIEW.resolve` calls in +`processor_impl.py` become `runner.view.resolve`. Parameter types `PurePosixPath` -> `PurePath`. +Relative paths stay posix-separated whatever the runner OS: they are layout, not view. Only the +resolved absolute strings change flavour. + +### 2.5 Handler factory and SSH + +- `_get_job_handler(runner: Runner)`. `start_job/get_job_status/get_job_result(..., runner_name)` + look the runner up. `ssh_sensor.py:58` reads `.runner`. +- Slurm: `SlurmSSHJobHandler(runner.view.resolve(Locations.SLURM), runner.ssh_connection_id_prefix)`. + The factory raises `AirflowFailException` naming the runner if the prefix is `None`. +- Docker: unchanged, `DockerJobHandler(DOCKER_HOST_VIEW)`. The host view is a property of the + worker host, not of a runner. +- `ssh_execute(command, ssh_connection_id_prefix)`, `get_cluster_ssh_hook(attempt_no, prefix, ...)`, + `_get_cluster_ssh_connections(prefix)`. The `debug_no_cluster_ssh` shortcut is untouched. +- Unknown `engine` still raises `ValueError` in the factory. Adding the SSH handler later is: + one `JobEngines` constant, one factory branch, one module. + +#### 2.5.1 Why SSH credentials stay in Airflow Connections + +Considered and rejected (2026-09-02): moving host, user and password into the runner block. + +- The yaml is read by every Airflow component and by the webapp; only the workers running + `ssh_execute` need the credentials. Airflow Connections are Fernet-encrypted in the metadata DB + and reachable from Airflow only. +- The yaml is loaded once at import; rotating a password would need a container restart. + Connections change live and can be tested in the UI. +- Reproducible, file-based setup is available without moving secrets: + `AIRFLOW_CONN_=ssh://user:pw@host` in `envs/.env`, next to `MONGO_PASSWORD`. + +#### 2.5.2 Why a prefix, not a list of connection ids + +A `ssh_connection_ids: [...]` list would make the runner self-contained, but adding or removing a +head node would then touch the yaml and restart the containers. With a prefix, connections are +added and removed in Airflow alone, and the existing round-robin discovery +(`_get_cluster_ssh_connections`) is reused unchanged. The list is a possible follow-up. + +Both rationales are recorded as comments next to `ssh_connection_id_prefix` in +`envs/alphakraken.local.yaml` and in `docs/deployment.md` (SSH connection section), cf. 2.10. + +### 2.6 Validation (`_check_content`) + +Replace the dump-everything loop with an explicit list. Strict check (no spaces, no absolute): +`relative_raw_file_path`, `relative_output_path`, `speclib_file_name`, `fasta_file_name`, +`config_file_name`, `software_type`, `metrics_type`, `raw_file_id`, `project_id`, +`settings_name`, `year_month_folder`, `runner`, `slurm_mem` (ends up in `sbatch --mem=`). +`software` with `allow_absolute_paths=True`, as today (an absolute `software` is a valid config). +`config_params` via `substitute_dummy_values(settings.config_params)` with spaces, as today. +Not checked: `raw_file_path`, `settings_path`, `output_path`, `custom_command` (base from yaml +plus parts checked above), `slurm_time` (as today). +Add one `TODO: revisit validation: which fields need which check, and where (webapp vs. here)` +above the list. + +Coverage is equivalent to today's: the absolute fields were only ever "yaml base + relative +part", and the relative part is now checked directly. The allowed character set in +`shared/validation.py` does not change. + +### 2.7 Webapp (`webapp/pages_/settings.py`) + +- Selectbox options `list(RUNNERS)`; default the first declared runner. `SHOW_JOB_ENGINE_SELECT` + -> `SHOW_RUNNER_SELECT`. Prefill key `runner`. +- Line 509 check becomes `RUNNERS[runner].engine == JobEngines.DOCKER and software_type != CUSTOM`. +- Help texts at 314 and 415 say "runner". + +### 2.8 Migration + +`shared/_migrations/from_0.9.0/_migrate_job_engine_to_runner.py`, same shape as +`_migrate_backfill_settings_fields.py`: for each Settings document with `job_engine` and without +`runner`, set `runner` from an editable `_ENGINE_TO_RUNNER` dict (identity by default), unset +`job_engine`. `--dry-run`; at the end, print the distinct target runner names with their counts, +so they can be compared with the yaml (the sandbox may hold `file_based` Settings, which no +in-repo yaml declares). Docstring states the precondition: the yaml must declare runners with +those names. + +### 2.9 Consistency test + +Extend `shared/tests/test_deployment_paths.py`: every in-repo yaml declares `runners`, each +engine and os is known, each runner has `view`, `backup.backup_base_path` is present and equals +the `slurm` runner's `view.backup`, no +top-level `locations` key exists, every `mounts.` entry has `mount_src` and `mount_target`, +and each in-repo `slurm` runner declares all five locations it uses (the import-time check only +rejects unknown keys). The existing mount-target assertions (`test_deployment_paths.py:84-98`) +iterate `mounts` instead of `locations`. + +### 2.9a mount.sh + +`mount.sh:52-56` sets `ENTITY_TYPE` to `mounts` instead of `locations` for `backup`/`output`/ +`logs`; `mounts.` has the same depth as `instruments.`, so `get_data` is unchanged. The +mounts folder comes from `MOUNTS_PATH` in `envs/${ENV}.env`, which the script sources, instead of +the yaml. Behaviour of the generated fstab line is unchanged. + +### 2.10 Docs + +- `envs/alphakraken.local.yaml` is the commented reference: document the block there, including + a one-line version of 2.5.1 and 2.5.2 next to `ssh_connection_id_prefix`. +- `docs/deployment.md`, "Setup SSH connection" section: state that credentials stay in Airflow + and why, and how a runner selects its connections by prefix. +- `docs/deployment.md:323-360` (standalone docker section) says "runner" instead of + "execution engine", mentions the `runners:` block, and drops the instruction to keep + `locations.general.mounts_path` and `MOUNTS_PATH` in sync (lines 232 and 346), replacing it + with "`MOUNTS_PATH` must be absolute for the docker runner". +- `envs/{local,sandbox,production}.env`: comment on `MOUNTS_PATH` saying it must be absolute + when the docker runner is used. The local value stays relative. +- `docs/deployment.md`, mounting section: `MOUNTS_PATH` must be absolute when `mount.sh` is + used, a relative value resolves against the current directory and yields a relative fstab + line. +- `docs/deployment.md`, upgrade notes: deploy the new yaml and code together, then run the + migration (2.8) before any quanting DAG runs. New code without the migration fails every job + (`Settings.runner` is unset); the new yaml on old code fails at import (no `locations`). +- `shared/config_params.py:30,33`: relative paths are "relative to the runner's backup/output + location" instead of naming `locations..absolute_path`. + +## 3. Tech stack + +Python 3.11+ (Airflow 2.11 image), pydantic v2, mongoengine, streamlit, pytest, ruff (`ALL`), +`ty`. No new dependencies. + +## 4. Commands + +``` +conda activate alphakraken2 +export AIRFLOW_HOME=; airflow db init # once, else 5 test_dags failures +pytest shared +pytest webapp +pytest airflow_src --ignore=airflow_src/tests/plugins/jobs/test_docker_job_handler.py # unless `docker` is installed +pre-commit run --all-files +``` + +CI runs the three `pytest` commands separately (`.github/workflows/branch-checks.yaml:36-52`). + +## 5. Project structure + +``` +shared/runners.py new: Runner, _build_runners, RUNNERS, get_runner, OperatingSystems +shared/path_views.py CLUSTER_VIEW and _build_cluster_view removed; DOCKER_HOST_VIEW reads EnvVars.MOUNTS_PATH +shared/yamlsettings.py YamlKeys.RUNNERS (+ nested), YamlKeys.MOUNTS, YamlKeys.Backup.BACKUP_BASE_PATH; YamlKeys.LOCATIONS, ABSOLUTE_PATH, Locations removed; BACKUP_BASE_PATH; _test_ stub +shared/keys.py EnvVars.MOUNTS_PATH +docker-compose.yaml MOUNTS_PATH added to airflow-common-env +envs/{local,sandbox,production}.env comment: MOUNTS_PATH absolute for the docker runner +mount.sh reads mounts., sources envs/${ENV}.env for MOUNTS_PATH +shared/keys.py JobEngines unchanged +shared/db/models.py, shared/db/interface.py Settings.runner +shared/_migrations/from_0.9.0/_migrate_job_engine_to_runner.py +shared/tests/test_runners.py new +shared/tests/test_deployment_paths.py extended +airflow_src/plugins/jobs/job_handler.py factory takes Runner +airflow_src/plugins/jobs/slurm_ssh_job_handler.py takes ssh prefix +airflow_src/plugins/jobs/docker_job_handler.py docstring only: MOUNTS_PATH instead of locations.general.mounts_path +airflow_src/plugins/jobs/_experimental/file_based_job_handler.py docstring only: runner view instead of locations.software.absolute_path +airflow_src/plugins/sensors/ssh_utils.py, sensors/ssh_sensor.py +airflow_src/plugins/common/utils.py ssh discovery by prefix argument +airflow_src/plugins/common/constants.py CLUSTER_SSH_CONNECTION_ID_PREFIX removed +airflow_src/plugins/common/quanting_env.py runner field +airflow_src/dags/impl/processor_impl.py runner.view, _check_content +airflow_src/dags/impl/handler_impl.py get_backup_base_path reads BACKUP_BASE_PATH +airflow_src/tests/helpers.py yaml_locations() -> runner_view(name, **paths) +webapp/pages_/settings.py +envs/alphakraken.{local,sandbox,production}.yaml +docs/deployment.md +# comment/docstring rewording only, so that 9.2 holds; existing TODOs stay TODOs: +shared/path_views.py Locations docstring ("keys of the `locations` section") +shared/tests/test_deployment_paths.py line 52 docstring +webapp/pages_/settings.py TODOs at 311 (`cf. CLUSTER_VIEW`) and 381 (`locations.backup.absolute_path`) +envs/alphakraken.local.yaml line 27 comment +airflow_src/dags/impl/handler_impl.py line 68 comment +airflow_src/plugins/common/constants.py line 10 comment +airflow_src/tests/plugins/jobs/test_docker_job_handler.py line 37 comment +docs/deployment.md line 187 (`locations.slurm.absolute_path` -> the slurm runner's `view.slurm`) +``` + +## 6. Code style + +Follows the existing modules. Example of the target style, from `shared/path_views.py`: + +```python +def resolve(self, location: str, rel_path: PurePath | str = "") -> _P: + """Get the absolute path of `rel_path`, which is relative to `location`, in this view.""" + if location not in self._locations: + raise KeyError( + f"Location '{location}' is not reachable in the '{self._name}' view, " + f"reachable are: {sorted(self._locations)}." + ) + return self._locations[location] / rel_path +``` + +- Yaml keys and engine/os names are constants (`ConstantsClass`), never literals at use sites. +- Comments say why, not what. Docstrings scoped to the public API. +- Imports at module top. Flat `shared/runners.py`, not a package (cf. plan §0.2 shadowing). +- Each chunk is its own commit, green on its own, `pre-commit` clean. + +## 7. Testing strategy + +pytest, tests next to the existing ones (`shared/tests`, `airflow_src/tests`, `webapp/tests`). + +- 7.1 `test_runners.py`: `_build_runners` from a yaml list; missing `name`, duplicate `name`, missing `os`, + missing `view`, unknown location key, unknown `os` each fail; a prefix on a `docker` runner and a + runner without `slurm` are accepted; `macos` yields the same view as `linux`; windows runner resolves + `\\server\share\backup\test1\1970_01\f.raw` and `Z:\...\out_f.raw\alphadia` from the layout + functions; `get_runner` KeyError names known runners. +- 7.2 `test_processor_impl.py`: `prepare_job` with a windows runner (patched `RUNNERS`) yields + the windows strings in `RAW_FILE_PATH`, `SETTINGS_PATH`, `OUTPUT_PATH`, `CUSTOM_COMMAND`, + substituted `_CONFIG_PARAMS`, and `_check_content` returns no errors. `_check_content` still + rejects `..`, `;`, `$` in relative paths, file names, `software`, `config_params`. +- 7.3 Regression: for a `slurm` runner whose `view` equals the former `absolute_path` + values, `QuantingEnv.to_dict()` is byte-identical to before, except `_JOB_ENGINE` -> `_RUNNER`. + `get_backup_base_path` yields the same string as before for the same yaml values. +- 7.4 `test_utils.py`: two prefixes select disjoint connection sets. +- 7.5 `test_job_handler.py`: factory per engine with a `Runner`; unknown engine raises; a + `slurm` runner without `ssh_connection_id_prefix` raises naming the runner. +- 7.6 `webapp/tests`: selectbox options come from `RUNNERS`; docker-only-custom check keyed by + engine of the selected runner. +- 7.7 Consistency test per 2.9; each assertion shown to fail on a mutated yaml. +- 7.8 `test_path_views.py`: `DOCKER_HOST_VIEW` built from `MOUNTS_PATH` in the environment; unset + variable yields a view that reaches nothing. +- 7.9 Coverage expectation: every new branch in `runners.py` and `_check_content` is hit. + +## 8. Boundaries + +- **Always:** run the three pytest commands and `pre-commit` before each commit; one chunk per + commit; keep relative paths posix; keep `get_backup_base_path` independent of any runner. +- **Ask first:** any change to the allowed character set in `shared/validation.py`; adding a + `JobEngines` value; touching `docker_job_handler.py` beyond the constructor call site and the + module docstring; any further change to exported env var names. +- **Never:** implement the SSH handler or a Windows job script here; add backwards + compatibility for missing `runners:` or old `job_engine`; change the persisted DB paths + (`RawFile.backup_base_path`, `Metrics.output_path`); generate `mount.sh`; touch + `msqc-extractor`; commit `BOYSCOUT_*.md`. + +## 9. Success criteria + +- 9.1 `grep -rn job_engine --include='*.py'` outside `shared/_migrations` returns nothing. +- 9.2 `grep -rn 'CLUSTER_VIEW\|absolute_path\|mounts_path\|locations\.[a-z*]*\.\|"locations"\|^locations:\|YamlKeys.LOCATIONS'` + over `*.py`, `*.sh`, `*.md` and `envs/*.yaml` returns only the migration scripts and the + design docs. `InternalPaths.MOUNTS_PATH` (the container-side constant) is unaffected and not + part of this criterion. +- 9.3 Tests 7.1 to 7.7 pass; full suite green (the 5 known `test_dags` env failures excepted + when `AIRFLOW_HOME` is not set up). +- 9.4 A yaml with a windows runner produces a `QuantingEnv` whose absolute paths are UNC or + drive-letter strings and that passes `_check_content`. +- 9.5 A `Settings.runner` that is not declared fails the DAG with a message listing the declared + runners. +- 9.6 A yaml without `runners:` fails at import naming the missing key. +- 9.7 Migration `--dry-run` on a copy of the sandbox DB reports every Settings document exactly + once. + +## 10. Out of scope + +The SSH job handler and its Windows job script. Per-runner job scripts. Changing how the docker +handler binds paths (1.2.5 keeps it). Generating `mount.sh` from the yaml (only its key lookup +moves, 2.9a). DB-persisted views and the six webapp path TODOs +(D5). `QuantingEnv` view-typed fields (D4). Runner-specific `Pools` (`cluster_slots_pool` still +gates all runners). + +## 11. Open questions + +- 11.1 Should `settings_path` for a windows runner keep the trailing-slash-free join, i.e. does + the future Windows job script want `Z:\settings\NAME` exactly? Assumed yes. +- 11.2 `Settings.runner` max length 64: enough for hostnames-as-names? Assumed yes. diff --git a/airflow_src/plugins/jobs/docker_job_handler.py b/airflow_src/plugins/jobs/docker_job_handler.py index ae319d96..cf203b83 100644 --- a/airflow_src/plugins/jobs/docker_job_handler.py +++ b/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. diff --git a/airflow_src/plugins/jobs/job_handler.py b/airflow_src/plugins/jobs/job_handler.py index e7b5f30c..26f7d76a 100644 --- a/airflow_src/plugins/jobs/job_handler.py +++ b/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." ) logging.info("Using DockerJobHandler") diff --git a/airflow_src/tests/conftest.py b/airflow_src/tests/conftest.py index e0c1dac3..5ef362a1 100644 --- a/airflow_src/tests/conftest.py +++ b/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 diff --git a/airflow_src/tests/plugins/jobs/test_job_handler.py b/airflow_src/tests/plugins/jobs/test_job_handler.py index 01e66bcd..2e25ac64 100644 --- a/airflow_src/tests/plugins/jobs/test_job_handler.py +++ b/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) diff --git a/docker-compose.yaml b/docker-compose.yaml index 1c73e14c..fe3efd82 100644 --- a/docker-compose.yaml +++ b/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' diff --git a/envs/local.env b/envs/local.env index b825fd46..077ba0e5 100644 --- a/envs/local.env +++ b/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 diff --git a/envs/production.env b/envs/production.env index a9ae41bb..3637b053 100644 --- a/envs/production.env +++ b/envs/production.env @@ -34,6 +34,7 @@ REDIS_HOST= # access on another machine REDIS_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 diff --git a/envs/sandbox.env b/envs/sandbox.env index 1c6b0bba..e600cc99 100644 --- a/envs/sandbox.env +++ b/envs/sandbox.env @@ -34,6 +34,7 @@ REDIS_HOST= # access on another machine REDIS_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 diff --git a/shared/keys.py b/shared/keys.py index d65a50b6..f29d4d93 100644 --- a/shared/keys.py +++ b/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" diff --git a/shared/path_views.py b/shared/path_views.py index 634abab5..a70ee5a5 100644 --- a/shared/path_views.py +++ b/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) diff --git a/shared/tests/conftest.py b/shared/tests/conftest.py index 93382580..bc4b7246 100644 --- a/shared/tests/conftest.py +++ b/shared/tests/conftest.py @@ -3,3 +3,4 @@ import os os.environ["ENV_NAME"] = "_test_" +os.environ["MOUNTS_PATH"] = "./tmp/test/mounts" diff --git a/shared/tests/test_deployment_paths.py b/shared/tests/test_deployment_paths.py index 1973813a..bf5f6ecf 100644 --- a/shared/tests/test_deployment_paths.py +++ b/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) diff --git a/shared/tests/test_path_views.py b/shared/tests/test_path_views.py index 59e9a44f..4c30790d 100644 --- a/shared/tests/test_path_views.py +++ b/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) diff --git a/shared/tests/test_yamlsettings.py b/shared/tests/test_yamlsettings.py index 0435eecd..bdf79fa2 100644 --- a/shared/tests/test_yamlsettings.py +++ b/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"}, diff --git a/shared/yamlsettings.py b/shared/yamlsettings.py index 68e9687e..81cbde16 100644 --- a/shared/yamlsettings.py +++ b/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"}, diff --git a/tasks/plan.md b/tasks/plan.md new file mode 100644 index 00000000..d3e5a1c2 --- /dev/null +++ b/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. diff --git a/tasks/todo.md b/tasks/todo.md new file mode 100644 index 00000000..583d1f7b --- /dev/null +++ b/tasks/todo.md @@ -0,0 +1,292 @@ +# Tasks: Named runners (D3-lite) + +Spec refs are to `SPEC.md`. Every task: one commit, `pre-commit run --all-files` clean, +`pytest shared`, `pytest webapp`, `pytest airflow_src --ignore=airflow_src/tests/plugins/jobs/test_docker_job_handler.py` green. + +## Phase 1: Foundations (independent slices) + +### Task 1: `DOCKER_HOST_VIEW` from the `MOUNTS_PATH` environment variable + +**Description:** `EnvVars.MOUNTS_PATH`; `_build_docker_host_view` reads it, unset yields an empty +view. `docker-compose.yaml` passes `MOUNTS_PATH` through `airflow-common-env`. The three test +conftests set `MOUNTS_PATH` next to `ENV_NAME`; the `_test_` stub loses `general.mounts_path`. +Factory error text and `docker_job_handler.py` module docstring name the variable instead of the +yaml key. `envs/*.env` comment: must be absolute when the docker runner is used. The yaml key +`locations.general.mounts_path` stays, unread, until T10. Spec 1.2.7, 7.8. + +**Acceptance criteria:** +- [x] `MOUNTS_PATH=/m` -> `DOCKER_HOST_VIEW.resolve(OUTPUT, "P1/x") == PurePosixPath("/m/output/P1/x")`; unset -> `has(OUTPUT)` is `False` (7.8). +- [x] Docker factory branch with empty view raises naming `MOUNTS_PATH`. +- [x] `grep -rn mounts_path shared airflow_src docker-compose.yaml` returns nothing. + +**Verification:** +- [x] `pytest shared/tests/test_path_views.py airflow_src/tests/plugins/jobs/test_job_handler.py` +- [x] All three suites (conftests changed); `docker compose config` shows `MOUNTS_PATH` in an airflow service's environment (checked via the yaml anchors, `docker compose` not runnable in the sandbox). + +**Dependencies:** None +**Files:** `shared/keys.py`, `shared/path_views.py`, `shared/yamlsettings.py` (stub), `docker-compose.yaml`, `envs/{local,sandbox,production}.env`, `airflow_src/plugins/jobs/job_handler.py`, `airflow_src/plugins/jobs/docker_job_handler.py` (docstring), `{shared,airflow_src,webapp}/tests/conftest.py`, `shared/tests/test_path_views.py`, `airflow_src/tests/plugins/jobs/test_job_handler.py` +**Scope:** M by change size, L by file count (one to three lines each) + +### Task 2: `backup.backup_base_path` replaces `CLUSTER_VIEW` in `get_backup_base_path` + +**Description:** `YamlKeys.Backup.BACKUP_BASE_PATH`; module constant `BACKUP_BASE_PATH` in +`yamlsettings.py`, read at import through a small function that raises `KeyError` naming the key. +Three yamls and the `_test_` stub gain `backup.backup_base_path` = former +`locations.backup.absolute_path` (stub: `./tmp/test/backup`). `get_backup_base_path` = +`PurePosixPath(BACKUP_BASE_PATH) / get_raw_file_folder_rel_path(raw_file)`; line 68 comment +reworded. Consistency test: key present in every in-repo yaml. Spec 1.2.3, 2.2, 7.3 second half. + +**Acceptance criteria:** +- [ ] `get_backup_base_path` returns the same string as before for the same yaml values (regression test with patched `BACKUP_BASE_PATH`). +- [ ] The reader function raises naming `backup.backup_base_path` on a dict without the key. +- [ ] `handler_impl.py` no longer imports `CLUSTER_VIEW`. + +**Verification:** +- [ ] `pytest shared/tests/test_yamlsettings.py shared/tests/test_deployment_paths.py airflow_src/tests/dags/impl/test_handler_impl.py` +- [ ] All three suites + +**Dependencies:** None +**Files:** `envs/alphakraken.{local,sandbox,production}.yaml`, `shared/yamlsettings.py`, `shared/tests/test_yamlsettings.py`, `airflow_src/dags/impl/handler_impl.py`, `airflow_src/tests/dags/impl/test_handler_impl.py`, `shared/tests/test_deployment_paths.py` +**Scope:** M + +### Task 3: `_check_content` explicit field list + +**Description:** Replace the dump-everything loop (spec 2.6). Strict check on the listed relative +fields incl. `slurm_mem` (`runner` is added in T5); `software` with `allow_absolute_paths=True`; +`config_params` via `substitute_dummy_values(settings.config_params)` with spaces. Not checked: +`raw_file_path`, `settings_path`, `output_path`, `custom_command`, `slurm_time`. One +`TODO: revisit validation ...` above the list. `shared/validation.py` untouched. + +**Acceptance criteria:** +- [ ] Still rejects `..`, `;`, `$` in relative paths, file names, `software`, `slurm_mem`, `config_params` (7.2 second half). +- [ ] A `QuantingEnv` with `raw_file_path='\\server\share\x.raw'`, `output_path='Z:\out'` and posix relative parts returns no errors. +- [ ] Every new branch covered (7.9). + +**Verification:** +- [ ] `pytest airflow_src/tests/dags/impl/test_processor_impl.py -k check_content` +- [ ] `pytest airflow_src` + +**Dependencies:** None +**Files:** `airflow_src/dags/impl/processor_impl.py`, `airflow_src/tests/dags/impl/test_processor_impl.py` +**Scope:** S + +### Task 4: `shared/runners.py` and the `runners:` yaml block + +**Description:** New module per spec 2.2: `OperatingSystems`, frozen `Runner`, `_build_runners`, +`RUNNERS`, `get_runner`. `YamlKeys.RUNNERS` plus nested keys (`name`, `engine`, `os`, +`ssh_connection_id_prefix`, `view`). The three yamls gain `runners:` with a `slurm` and a `docker` +runner whose `view` copies the `absolute_path` values; the `_test_` stub gains `slurm`, `docker`, +`file_based` with the `./tmp/test/...` paths. `local.yaml` carries the reference comments incl. +the one-line 2.5.1/2.5.2 rationale next to `ssh_connection_id_prefix`. Nothing consumes `RUNNERS` +yet. Consistency test: every in-repo yaml declares `runners`, engine and os known, each runner has +`view`, each `slurm` runner declares all five locations, `backup_base_path == slurm view.backup`. +`locations` stays in the yamls. Spec 2.1, 2.2, 2.9 runner parts, 7.1. + +**Acceptance criteria:** +- [ ] `_build_runners` rejects: empty list, missing `name`, duplicate `name`, unknown `engine`, missing or unknown `os`, missing `view`, unknown `view` key; each error names runner and key. +- [ ] Accepts: prefix on a `docker` runner, runner without `slurm`; `macos` == `linux` flavour. +- [ ] Windows runner resolves `\\server\share\backup\test1\1970_01\f.raw` and `Z:\...\out_f.raw\alphadia` from the layout functions; `get_runner("nope")` raises `KeyError` listing known names. +- [ ] Each new consistency assertion fails on a mutated yaml (one mutation each, reverted). +- [ ] No yaml key or engine/os literal at a use site in `runners.py`. + +**Verification:** +- [ ] `pytest shared/tests/test_runners.py shared/tests/test_deployment_paths.py` +- [ ] `pytest shared` + +**Dependencies:** T2 (equality assertion) +**Files:** `shared/runners.py` (new), `shared/tests/test_runners.py` (new), `shared/yamlsettings.py`, `envs/alphakraken.{local,sandbox,production}.yaml`, `shared/tests/test_deployment_paths.py` +**Scope:** M + +### Checkpoint 1: Foundations +- [ ] Four commits, three suites green, pre-commit clean. +- [ ] `envs/alphakraken.local.yaml` loads with `runners`, `backup_base_path` and old `locations` side by side; nothing imports `shared.runners` outside tests. +- [ ] Human review before Phase 2. + +## Phase 2: Jobs are dispatched by runner + +### Task 5: `QuantingEnv.runner`, factory takes a `Runner` + +**Description:** `QuantingEnv.job_engine` -> `runner: str = Field(alias="_RUNNER")`. +`_get_job_handler(runner: Runner)` dispatches on `runner.engine`; `start_job/get_job_status/ +get_job_result(..., runner_name)` call `get_runner`. `ssh_sensor.py` reads `.runner`. +`processor_impl` passes `quanting_env.runner` to the job functions and, for this commit only, +sets `runner=settings.job_engine` (transitional line, removed in T6, named in the commit +message). `runner` joins the strict list of `_check_content`. Slurm branch still resolves +`CLUSTER_VIEW` for its base dir (T8). Spec 2.3, 2.5 factory signature, 7.5 partial, 9.5. + +**Acceptance criteria:** +- [ ] `QuantingEnv.to_dict()` differs from before only in `_JOB_ENGINE` -> `_RUNNER` (existing full-dict assertion in `test_create_quanting_env`, spec 7.3). +- [ ] Factory per engine with a `Runner`; unknown engine raises `ValueError`. +- [ ] `start_job(env, runner_name="nope")` raises `KeyError` listing declared runners (9.5). +- [ ] `grep -rn job_engine airflow_src --include='*.py'` hits only the transitional line. + +**Verification:** +- [ ] `pytest airflow_src/tests/plugins/jobs/test_job_handler.py airflow_src/tests/plugins/sensors airflow_src/tests/dags/impl/test_processor_impl.py` +- [ ] `pytest airflow_src` + +**Dependencies:** T3, T4 +**Files:** `airflow_src/plugins/common/quanting_env.py`, `airflow_src/plugins/jobs/job_handler.py`, `airflow_src/plugins/sensors/ssh_sensor.py`, `airflow_src/dags/impl/processor_impl.py`, tests: `conftest.py`, `test_job_handler.py`, `test_ssh_sensor.py`, `test_processor_impl.py` +**Scope:** M + +### Task 6: `Settings.runner` replaces `job_engine`; webapp selectbox + +**Description:** `Settings.runner = StringField(required=True, max_length=64)`, `job_engine` +deleted; `create_settings(runner=...)`. The T5 transitional line becomes `runner=settings.runner`. +Webapp (spec 2.7): options `list(RUNNERS)`, default first declared, `SHOW_RUNNER_SELECT`, prefill +key `runner`, docker-only-custom check via `RUNNERS[runner].engine`, help texts at 314 and 415 +say "runner". Test 7.6. + +**Acceptance criteria:** +- [ ] `grep -rn job_engine --include='*.py' . | grep -v _migrations` returns nothing (9.1). +- [ ] Webapp selectbox options equal `list(RUNNERS)`; docker + non-custom rejected by the engine of the selected runner (7.6). +- [ ] `create_settings` without `runner` raises. + +**Verification:** +- [ ] `pytest shared/tests/db/test_interface.py webapp airflow_src/tests/dags/impl/test_processor_impl.py` +- [ ] All three suites + +**Dependencies:** T5 +**Files:** `shared/db/models.py`, `shared/db/interface.py`, `webapp/pages_/settings.py`, `airflow_src/dags/impl/processor_impl.py` (one line), tests: `shared/tests/db/test_interface.py`, `webapp/tests/pages_/test_settings.py`, `airflow_src/tests/dags/impl/test_processor_impl.py` (mocks) +**Scope:** M + +### Task 7: Migration `job_engine` -> `runner` + +**Description:** `shared/_migrations/from_0.9.0/_migrate_job_engine_to_runner.py`, shape of +`_migrate_backfill_settings_fields.py`: for each Settings document with `job_engine` and without +`runner`, `$set runner` from `_ENGINE_TO_RUNNER` (identity by default), `$unset job_engine`. +`--dry-run`; prints distinct target runner names with counts. Docstring states the yaml +precondition. Spec 2.8, 9.7. + +**Acceptance criteria:** +- [ ] Documents already carrying `runner` are skipped; each document reported once (9.7). +- [ ] Summary lists distinct target names with counts. + +**Verification:** +- [ ] `--dry-run` against a local mongo with two hand-made Settings docs (one legacy, one migrated): 1 updated, 1 skipped. +- [ ] `pre-commit run --all-files` + +**Dependencies:** T6 +**Files:** `shared/_migrations/from_0.9.0/_migrate_job_engine_to_runner.py` (new) +**Scope:** S + +### Checkpoint 2: Runner flows +- [ ] Three suites green, pre-commit clean; `grep -n "job_engine" airflow_src/dags/impl/processor_impl.py` empty. +- [ ] Local stack: create a settings entry in the webapp with runner `slurm`, trigger a quanting DAG with `debug_no_cluster_ssh=true`, the `prepare_job` XCom shows `_RUNNER: slurm` and the same paths as before. +- [ ] Migration `--dry-run` runs against a sandbox DB copy; note the distinct names (open question 1). +- [ ] Human review before Phase 3. + +## Phase 3: Paths come from the runner + +### Task 8: Slurm base dir and SSH connections from the runner + +**Description:** Factory: `SlurmSSHJobHandler(runner.view.resolve(Locations.SLURM), +runner.ssh_connection_id_prefix)`, `AirflowFailException` naming the runner if the prefix is +`None`. `ssh_execute(command, ssh_connection_id_prefix)`, `get_cluster_ssh_hook(attempt_no, +prefix, ...)`, `_get_cluster_ssh_connections(prefix)`; error text names the given prefix. +`CLUSTER_SSH_CONNECTION_ID_PREFIX` deleted; `constants.py:10` comment reworded. +`debug_no_cluster_ssh` untouched. `test_job_handler.py` builds `Runner` objects directly and drops +`yaml_locations`. Spec 2.5, 7.4, 7.5. + +**Acceptance criteria:** +- [ ] Two prefixes select disjoint connection sets (7.4). +- [ ] `slurm` runner with `ssh_connection_id_prefix=None` raises naming the runner (7.5). +- [ ] `grep -rn CLUSTER_SSH_CONNECTION_ID_PREFIX .` empty; `job_handler.py` no longer imports `CLUSTER_VIEW`. + +**Verification:** +- [ ] `pytest airflow_src/tests/plugins/jobs airflow_src/tests/common/test_utils.py airflow_src/tests/plugins/sensors` +- [ ] `pytest airflow_src` + +**Dependencies:** T5 +**Files:** `airflow_src/plugins/jobs/job_handler.py`, `airflow_src/plugins/jobs/slurm_ssh_job_handler.py`, `airflow_src/plugins/sensors/ssh_utils.py`, `airflow_src/plugins/common/utils.py`, `airflow_src/plugins/common/constants.py`, tests: `test_job_handler.py`, `test_utils.py`, `test_ssh_utils.py`, `test_slurm_ssh_job_handler.py` +**Scope:** L by file count; mechanical plumbing of one argument, not split (a partial chain leaves a dead parameter) + +### Task 9: `prepare_job` resolves through `runner.view`; `CLUSTER_VIEW` deleted + +**Description:** `runner = get_runner(settings.runner)`; the four `CLUSTER_VIEW.resolve` calls +become `runner.view.resolve`; parameter types `PurePosixPath` -> `PurePath`; relative paths stay +posix. `CLUSTER_VIEW`, `_build_cluster_view` and `test_cluster_view` removed. Test helper +`yaml_locations` -> `runner_view(name, **paths)` patching `RUNNERS[name].view._locations`; +`test_processor_impl.py` switches to it. Test 7.2: windows runner yields windows strings in +`RAW_FILE_PATH`, `SETTINGS_PATH`, `OUTPUT_PATH`, `CUSTOM_COMMAND`, substituted `_CONFIG_PARAMS`, +and `_check_content` passes. Spec 2.4, 7.2, 7.3, 9.4. + +**Acceptance criteria:** +- [ ] Windows-runner `prepare_job` output matches 9.4; `_RELATIVE_RAW_FILE_PATH` and `RELATIVE_OUTPUT_PATH` stay `/`-separated. +- [ ] Slurm runner with the former `absolute_path` values: `to_dict()` unchanged vs. T5 (7.3). +- [ ] `grep -rn CLUSTER_VIEW --include='*.py' .` empty. + +**Verification:** +- [ ] `pytest airflow_src/tests/dags/impl/test_processor_impl.py shared/tests/test_path_views.py` +- [ ] All three suites + +**Dependencies:** T2, T6, T8 +**Files:** `airflow_src/dags/impl/processor_impl.py`, `shared/path_views.py`, `airflow_src/tests/helpers.py`, `airflow_src/tests/dags/impl/test_processor_impl.py`, `shared/tests/test_path_views.py` +**Scope:** M + +### Checkpoint 3: Paths through the runner +- [ ] Three suites green, pre-commit clean; 9.4 by test. +- [ ] Local stack: `prepare_job` XCom identical to checkpoint 2. +- [ ] Human review before Phase 4. + +## Phase 4: Yaml final shape + +### Task 10: `locations` -> `mounts`, `mount.sh`, keys removed + +**Description:** In the three yamls move `backup`, `output`, `logs` mount entries to top-level +`mounts:` and delete `locations` (`general.mounts_path`, every `absolute_path`; `settings`, +`software`, `slurm` entries not carried over, spec 1.2.6). `_test_` stub loses `locations`. +`YamlKeys.LOCATIONS`, `ABSOLUTE_PATH`, `YamlKeys.Locations` removed; `YamlKeys.MOUNTS` added; +`Locations` docstring in `path_views.py` reworded. `mount.sh`: `ENTITY_TYPE=mounts`, +`MOUNTS_PATH` from `envs/${ENV}.env` sourced in a subshell exporting only that variable. +Consistency test: no top-level `locations`, every `mounts.` has `mount_src` and +`mount_target`, mount-target assertions iterate `mounts`; line 52 docstring reworded. +Spec 1.2.6, 1.2.7, 2.1, 2.9, 2.9a, 7.7. + +**Acceptance criteria:** +- [ ] `grep -n '^locations:' envs/*.yaml` empty; `grep -rn 'YamlKeys.LOCATIONS\|ABSOLUTE_PATH\|mounts_path' --include='*.py' . | grep -v _migrations` returns only `shared/validation.py:ABSOLUTE_PATH_ERROR`. +- [ ] `ENV=local ./mount.sh {backup,output,logs,test1} fstab` prints the same lines as before this task when `MOUNTS_PATH` in `envs/local.env` equals the old yaml value (manual diff, noted in the commit message). +- [ ] Each new consistency assertion fails on a mutated yaml (7.7). + +**Verification:** +- [ ] `pytest shared`; all three suites +- [ ] Manual `mount.sh ... fstab` diff + +**Dependencies:** T1, T9 +**Files:** `envs/alphakraken.{local,sandbox,production}.yaml`, `shared/yamlsettings.py`, `shared/path_views.py` (docstring), `shared/tests/test_yamlsettings.py`, `shared/tests/test_deployment_paths.py`, `mount.sh` +**Scope:** M + +### Checkpoint 4: Yaml final shape +- [ ] Three suites green, pre-commit clean; 9.6 holds (import fails naming `runners` on a stub without it, one reload-based test at most). +- [ ] Local stack boots on the new yaml; `prepare_job` XCom identical to checkpoint 3. +- [ ] Human review before Phase 5. + +## Phase 5: Docs and closure + +### Task 11: Docs, comment sweep, success criteria + +**Description:** Spec 2.10 and the reword-only list at the end of spec §5: `docs/deployment.md` +(SSH section with 2.5.1/2.5.2 and prefix selection; standalone docker section says "runner", +mentions `runners:`, drops the two-keys-in-sync instruction at 232/346 for "`MOUNTS_PATH` must be +absolute for the docker runner"; mounting section: relative `MOUNTS_PATH` yields a relative fstab +line; upgrade notes: new yaml and code together, then migration before any quanting DAG runs, +webapp container included; line 187 `view.slurm`). `shared/config_params.py:30,33`. Remaining +reword sites: `webapp/pages_/settings.py:311,381`, `file_based_job_handler.py:10`, +`test_docker_job_handler.py:37`. TODOs reworded, never resolved. + +**Acceptance criteria:** +- [ ] 9.1 and 9.2 greps return only migration scripts and `design_docs`. +- [ ] Every 2.10 bullet has a corresponding diff hunk. +- [ ] `git grep -c TODO -- '*.py' | awk -F: '{s+=$2} END {print s}'` equals the count on `main` plus one (the T3 TODO). + +**Verification:** +- [ ] `grep -rn 'CLUSTER_VIEW\|absolute_path\|mounts_path\|locations\.[a-z*]*\.\|"locations"\|^locations:\|YamlKeys.LOCATIONS' --include='*.py' --include='*.sh' --include='*.md' . envs/*.yaml | grep -v node_modules` +- [ ] All three suites; `pre-commit run --all-files` + +**Dependencies:** T10 +**Files:** `docs/deployment.md`, `shared/config_params.py`, `webapp/pages_/settings.py`, `airflow_src/plugins/jobs/_experimental/file_based_job_handler.py`, `airflow_src/tests/plugins/jobs/test_docker_job_handler.py` +**Scope:** S (text only) + +### Checkpoint 5: Complete +- [ ] Spec §9.1 to 9.7 satisfied; 9.7 run against a sandbox DB copy. +- [ ] Eleven commits, each green on its own (`git rebase -x 'pytest shared' main` as optional spot check). +- [ ] Ready for PR. diff --git a/webapp/tests/conftest.py b/webapp/tests/conftest.py index 93382580..bc4b7246 100644 --- a/webapp/tests/conftest.py +++ b/webapp/tests/conftest.py @@ -3,3 +3,4 @@ import os os.environ["ENV_NAME"] = "_test_" +os.environ["MOUNTS_PATH"] = "./tmp/test/mounts"