diff --git a/airflow_src/plugins/common/paths.py b/airflow_src/plugins/common/paths.py index 712f480d..b5240527 100644 --- a/airflow_src/plugins/common/paths.py +++ b/airflow_src/plugins/common/paths.py @@ -3,8 +3,8 @@ from pathlib import Path from shared.db.models import RawFile -from shared.keys import InternalPaths from shared.path_layout import get_output_folder_rel_path +from shared.path_views import AIRFLOW_CONTAINER_VIEW, Locations def get_internal_instrument_data_path(instrument_id: str) -> Path: @@ -12,7 +12,7 @@ def get_internal_instrument_data_path(instrument_id: str) -> Path: e.g. /opt/airflow/mounts/instruments/test2 """ - return Path(InternalPaths.MOUNTS_PATH) / InternalPaths.INSTRUMENTS / instrument_id + return AIRFLOW_CONTAINER_VIEW.resolve(Locations.INSTRUMENTS, instrument_id) def get_internal_backup_path() -> Path: @@ -20,7 +20,7 @@ def get_internal_backup_path() -> Path: e.g. /opt/airflow/mounts/backup """ - return Path(InternalPaths.MOUNTS_PATH) / InternalPaths.BACKUP + return AIRFLOW_CONTAINER_VIEW.resolve(Locations.BACKUP) def get_internal_backup_path_for_instrument( @@ -30,12 +30,12 @@ def get_internal_backup_path_for_instrument( e.g. /opt/airflow/mounts/backup/test2 """ - return get_internal_backup_path() / instrument_id + return AIRFLOW_CONTAINER_VIEW.resolve(Locations.BACKUP, instrument_id) def get_internal_output_path() -> Path: """Get absolute internal output path.""" - return Path(InternalPaths.MOUNTS_PATH) / InternalPaths.OUTPUT + return AIRFLOW_CONTAINER_VIEW.resolve(Locations.OUTPUT) def get_internal_output_path_for_raw_file( @@ -43,8 +43,6 @@ def get_internal_output_path_for_raw_file( software_type: str | None = None, ) -> Path: """Get absolute internal output path for the given raw file name.""" - return ( - Path(InternalPaths.MOUNTS_PATH) - / InternalPaths.OUTPUT - / get_output_folder_rel_path(raw_file, software_type) + return AIRFLOW_CONTAINER_VIEW.resolve( + Locations.OUTPUT, get_output_folder_rel_path(raw_file, software_type) ) diff --git a/airflow_src/plugins/jobs/_experimental/file_based_job_handler.py b/airflow_src/plugins/jobs/_experimental/file_based_job_handler.py index 843b404b..80af3013 100644 --- a/airflow_src/plugins/jobs/_experimental/file_based_job_handler.py +++ b/airflow_src/plugins/jobs/_experimental/file_based_job_handler.py @@ -13,7 +13,6 @@ # TODO: add unit tests import logging -from pathlib import Path from airflow.exceptions import AirflowFailException from common.keys import JobStates @@ -22,7 +21,8 @@ from jobs.job_handler import JobHandler from shared.db.interface import get_raw_file_by_id -from shared.keys import InternalPaths, SoftwareTypes +from shared.keys import SoftwareTypes +from shared.path_views import AIRFLOW_CONTAINER_VIEW, Locations class FileBasedJobHandler(JobHandler): @@ -33,8 +33,8 @@ def __init__(self): super().__init__() # is is a bit of a hack to use the output path here, but it avoids another bind mount - self._job_submit_dir = ( - Path(InternalPaths.MOUNTS_PATH) / InternalPaths.OUTPUT / "job_queue" + self._job_submit_dir = AIRFLOW_CONTAINER_VIEW.resolve( + Locations.OUTPUT, "job_queue" ) def start_job(self, quanting_env: QuantingEnv) -> str: diff --git a/airflow_src/tests/helpers.py b/airflow_src/tests/helpers.py index 1472f000..72b1aaa6 100644 --- a/airflow_src/tests/helpers.py +++ b/airflow_src/tests/helpers.py @@ -2,8 +2,10 @@ from collections.abc import Iterator from contextlib import contextmanager +from pathlib import PurePosixPath from unittest.mock import patch +from shared.path_views import CLUSTER_VIEW from shared.yamlsettings import YAMLSETTINGS, YamlKeys @@ -11,15 +13,22 @@ def yaml_locations(**paths: str) -> Iterator[None]: """Override the `locations` section of the yaml settings, e.g. `yaml_locations(slurm="/path/to/slurm")`. - Patches the settings that `get_path()` reads, rather than `get_path` at its import site, - so tests stay valid when code moves between modules. + Patches the settings that `get_path()` reads and the contents of `CLUSTER_VIEW`, rather than + either of them at its import site, so tests stay valid when code moves between modules. """ - with patch.dict( - YAMLSETTINGS, - { - YamlKeys.LOCATIONS: { - key: {YamlKeys.ABSOLUTE_PATH: path} for key, path in paths.items() - } - }, + with ( + patch.dict( + YAMLSETTINGS, + { + YamlKeys.LOCATIONS: { + key: {YamlKeys.ABSOLUTE_PATH: path} for key, path in paths.items() + } + }, + ), + patch.object( + CLUSTER_VIEW, + "_locations", + {key: PurePosixPath(path) for key, path in paths.items()}, + ), ): yield diff --git a/airflow_src/tests/plugins/jobs/test_file_based_job_handler.py b/airflow_src/tests/plugins/jobs/test_file_based_job_handler.py index 55de4a50..811936ea 100644 --- a/airflow_src/tests/plugins/jobs/test_file_based_job_handler.py +++ b/airflow_src/tests/plugins/jobs/test_file_based_job_handler.py @@ -10,7 +10,7 @@ from common.quanting_env import QuantingEnv from jobs._experimental.file_based_job_handler import FileBasedJobHandler -from shared.keys import InternalPaths +from shared.path_views import AIRFLOW_CONTAINER_VIEW, Locations @pytest.fixture @@ -47,16 +47,12 @@ def test_init_should_set_job_submit_directory(self) -> None: handler = FileBasedJobHandler() # then - expected_path = ( - Path(InternalPaths.MOUNTS_PATH) / InternalPaths.OUTPUT / "job_queue" - ) + expected_path = AIRFLOW_CONTAINER_VIEW.resolve(Locations.OUTPUT, "job_queue") assert handler._job_submit_dir == expected_path - @patch( - "jobs._experimental.file_based_job_handler.Path.open", new_callable=mock_open - ) - @patch("jobs._experimental.file_based_job_handler.Path.mkdir") - @patch("jobs._experimental.file_based_job_handler.Path.exists") + @patch("pathlib.Path.open", new_callable=mock_open) + @patch("pathlib.Path.mkdir") + @patch("pathlib.Path.exists") def test_start_job_should_create_job_file_when_directory_creation_succeeds( self, mock_exists: MagicMock, @@ -89,7 +85,7 @@ def test_start_job_should_create_job_file_when_directory_creation_succeeds( for expected_line in expected_content: handle.write.assert_any_call(expected_line) - @patch("jobs._experimental.file_based_job_handler.Path.exists") + @patch("pathlib.Path.exists") def test_start_job_should_raise_exception_when_job_file_already_exists( self, mock_exists: MagicMock, sample_quanting_env: QuantingEnv ) -> None: @@ -106,7 +102,7 @@ def test_start_job_should_raise_exception_when_job_file_already_exists( @patch( "jobs._experimental.file_based_job_handler.get_internal_output_path_for_raw_file" ) - @patch("jobs._experimental.file_based_job_handler.Path.exists") + @patch("pathlib.Path.exists") def test_get_job_status_should_return_pending_when_status_file_does_not_exist( self, mock_exists: MagicMock, @@ -140,7 +136,7 @@ def test_get_job_status_should_return_pending_when_status_file_does_not_exist( @patch( "jobs._experimental.file_based_job_handler.get_internal_output_path_for_raw_file" ) - @patch("jobs._experimental.file_based_job_handler.Path.exists") + @patch("pathlib.Path.exists") def test_get_job_status_should_return_correct_status_based_on_file_content( # noqa: PLR0913 self, mock_exists: MagicMock, @@ -159,7 +155,7 @@ def test_get_job_status_should_return_correct_status_based_on_file_content( # n # when with patch( - "jobs._experimental.file_based_job_handler.Path.open", + "pathlib.Path.open", mock_open(read_data=read_data), ): status = handler.get_job_status("test_raw_file_123") diff --git a/design_docs/PATH_HANDLING_DESIGN.md b/design_docs/PATH_HANDLING_DESIGN.md index e19ac155..aced0151 100644 --- a/design_docs/PATH_HANDLING_DESIGN.md +++ b/design_docs/PATH_HANDLING_DESIGN.md @@ -11,27 +11,27 @@ Drivers: (a) refactor for clarity, (b) enable a second "cluster view" on a Windo Every path in the system is ``` -frame.base(root) / layout(entity) +view.base(location) / layout(entity) ``` Three axes are conflated today: | Axis | Values | |---|---| -| **root** | backup, output, settings, software, slurm, instruments, logs | +| **location** | backup, output, settings, software, slurm, instruments, logs | | **layout** | `//`, `/[]/out_/` | -| **frame** | container, cluster, docker-host, SMB source (+ the new Windows one) | +| **view** | container, cluster, docker-host, SMB source (+ the new Windows one) | -The frame is chosen *implicitly*, by which accessor a caller happens to import +The view is chosen *implicitly*, by which accessor a caller happens to import (`get_internal_*` vs `get_path`). That is the root cause of §6.1-6.5. ## 2. Sizing facts -- The **cluster frame** has 6 non-test call sites: `processor_impl.py:122,178,188,272`, +- The **cluster view** has 6 non-test call sites: `processor_impl.py:122,178,188,272`, `handler_impl.py:328`, `job_handler.py:23`. -- The **container frame** has ~41 call sites across 12 files, but they are uniform and can keep +- The **container view** has ~41 call sites across 12 files, but they are uniform and can keep their current API as thin wrappers. -- The **host frame** has 1 site: `docker_job_handler.py:186`. +- The **host view** has 1 site: `docker_job_handler.py:186`. So the expensive-looking half (container) is the cheap half. @@ -49,7 +49,7 @@ Obtained 2026-09-01: ### D1 - Extra yaml key, branch on engine -`locations..windows_absolute_path`; `get_path(key, engine)` picks. +`locations..windows_absolute_path`; `get_path(key, engine)` picks. - **Pro:** hours of work, no migration, no new concepts. - **Con:** entrenches every problem in §6. A third view is a third key. `get_path` grows an @@ -57,34 +57,34 @@ Obtained 2026-09-01: is mangled on the Linux worker. Layout stays duplicated in 3 places. - **Rating:** cheapest, and paid for twice. -### D2 - Frame table + layout module (core refactor) +### D2 - View table + layout module (core refactor) -- `layout` module: pure, frame-free, returns `(Root, PurePosixPath)`. Single home for the +- `layout` module: pure, view-free, returns `(Locations, PurePosixPath)`. Single home for the raw-file and output layouts. Kills §6.3 and collapses the four relative anchors (§3.1-3.4) - into "root + rel"; `RemovePathProvider`'s `Path()` sentinel becomes an honest `rel="."` - on a named root. -- `frames` module: `Root` enum; `Frame(name, flavor, roots: dict[Root, PurePath])` with - `.resolve(root, rel)` and `.has(root)`. Instances `CONTAINER` (from `InternalPaths`), + into "location + rel"; `RemovePathProvider`'s `Path()` sentinel becomes an honest `rel="."` + on a named location. +- `views` module: `Locations` constants; `View(name, flavor, locations: dict[str, PurePath])` with + `.resolve(location, rel)` and `.has(location)`. Instances `AIRFLOW_CONTAINER_VIEW` (from `InternalPaths`), `CLUSTER` (from yaml `locations`), `HOST` (from `locations.general.mounts_path`). - A missing root raises naming both frame and root, so the §2 holes become explicit. + A missing location raises naming both view and location, so the §2 holes become explicit. - **Pro:** the ragged matrix of §2 becomes data with a real error instead of a silent hole. A new view = one dict + a flavor. Layout defined once. Only the 6 cluster sites must move; container helpers stay as one-line wrappers so the 41 sites do not churn. `mount.sh` can later be generated from the same table (attacks §6.6). -- **Con:** new module and vocabulary; does not *statically* prevent frame mixing; does not touch - the DB-persisted frames or the webapp TODOs (§5.12, §5.13). +- **Con:** new module and vocabulary; does not *statically* prevent view mixing; does not touch + the DB-persisted views or the webapp TODOs (§5.12, §5.13). - **Rating:** best clarity per unit of effort. Chosen as step 1. ### D3 - D2 + Runner objects `runners:` in yaml (cf. the `alphakraken.example.part.yaml` sketch), made real: -`Runner{name, engine, frame, ssh_connection_ids, job_script, root overrides}`; +`Runner{name, engine, view, ssh_connection_ids, job_script, location overrides}`; `Settings.runner` replaces `Settings.job_engine`; `prepare_job` resolves all exported paths -through `runner.frame`, while Airflow-side I/O keeps the container frame. +through `runner.view`, while Airflow-side I/O keeps the container view. - **Pro:** literally answers driver (c) - the nature of the quanting env *is* the runner. - A Windows cluster becomes one yaml block (`flavor: windows`, own roots, own script, own SSH + A Windows cluster becomes one yaml block (`flavor: windows`, own locations, own script, own SSH ids) plus a handler. Also fixes the global `cluster_ssh_*` prefix discovery (`common/utils.py:198`), which cannot address two clusters today. - **Con:** DB migration `job_engine -> runner` plus webapp settings form; yaml schema grows; @@ -92,22 +92,22 @@ through `runner.frame`, while Airflow-side I/O keeps the container frame. - **Rating:** the piece that actually buys the second cluster. D2 without D3 gets clarity but not the feature. -### D4 - Typed framed paths (on top of D2) +### D4 - Typed view-tagged paths (on top of D2) -`FramedPath(frame, path)` or a `NewType` per frame; `QuantingEnv` fields annotated by frame. +`ViewPath(view, path)` or a `NewType` per view; `QuantingEnv` fields annotated by view. - **Pro:** kills §6.1 statically; the `internal_` naming convention becomes redundant; `_check_content`'s whitelist-by-field-name disappears. - **Con:** the type leaks at every pydantic / mongoengine / `str()` / filesystem boundary. Ceremony out of proportion to 6 cluster call sites. -- **Rating:** skip. Revisit beyond ~4 frames. +- **Rating:** skip. Revisit beyond ~4 views. ### D5 - Relative-on-the-wire -Export only root-relative paths plus per-root base env vars; the job script joins. +Export only location-relative paths plus per-location base env vars; the job script joins. DB stores relative only; the webapp resolves for display. -- **Pro:** exactly one frame exists in Python. Fixes the persisted-frame problem (§5.12), the +- **Pro:** exactly one view exists in Python. Fixes the persisted-view problem (§5.12), the 6 webapp TODOs (§5.13), and `RawFile.backup_base_path` disappears. A new view then costs zero Python. - **Con:** joins move into bash/PowerShell; `{RAW_FILE_PATH}` placeholder semantics change @@ -116,7 +116,7 @@ DB stores relative only; the webapp resolves for display. ## 5. Recommendation -D2 now, D3 next, D4 never (until frames multiply), D5 as a separate later decision. +D2 now, D3 next, D4 never (until views multiply), D5 as a separate later decision. ## 6. Constraints any Windows view must satisfy (independent of the option chosen) @@ -128,11 +128,11 @@ D2 now, D3 next, D4 never (until frames multiply), D5 as a separate later decisi defence for the bash runner. - 6.2 **Path flavor.** Foreign-view paths must be built with `PureWindowsPath` / `PurePosixPath`, never `Path`: on the Linux worker `Path("\\\\srv\\share")` is one filename, - not a UNC root. The frame must own its flavor, and yaml roots must stay opaque strings so + not a UNC location. The view must own its flavor, and yaml locations must stay opaque strings so that both UNC and drive letters (§3.2) survive. - 6.3 **The job script is bash-only.** `submit_job.sh` uses `sbatch`, `module load`, `md5sum`. A Windows runner needs its own script and submitter. That is a Runner concern, not a path concern - an argument for D3. - 6.4 **`mount.sh`, `docker-compose.yaml` and `InternalPaths` agree by convention only** (§6.6), cf. the production `backup` mount-depth finding in `BOYSCOUT_20260901_081142.md`. Once the - container frame is a table, a test comparing it to `docker-compose.yaml` is ~20 lines. + container view is a table, a test comparing it to `docker-compose.yaml` is ~20 lines. diff --git a/design_docs/PATH_HANDLING_IMPLEMENTATION_PLAN.md b/design_docs/PATH_HANDLING_IMPLEMENTATION_PLAN.md index e0d210ca..3c7b3e3c 100644 --- a/design_docs/PATH_HANDLING_IMPLEMENTATION_PLAN.md +++ b/design_docs/PATH_HANDLING_IMPLEMENTATION_PLAN.md @@ -8,22 +8,23 @@ deliberately not started here; D2 only has to avoid blocking it. - 0.1 Each chunk is a separate commit, green on its own. No chunk changes behavior; the whole plan is a pure refactoring. -- 0.2 **Module naming:** flat modules `shared/path_layout.py` and `shared/path_frames.py`, not a +- 0.2 **Module naming:** flat modules `shared/path_layout.py` and `shared/path_views.py`, not a `shared/paths/` package. `./shared` is on `pythonpath` (`pyproject.toml:50-58`), so a `shared/paths` package would also be importable as `paths` - two module objects for one file, - two `CLUSTER` singletons. `path_layout` / `path_frames` are unshadowable. -- 0.3 **Flavor from day one:** `Frame` carries a `flavor` and builds its roots with - `PurePosixPath` / `PureWindowsPath`, even though every frame is posix today. Retrofitting - flavor after ~50 call sites exist is the expensive version. + two `AIRFLOW_CONTAINER_VIEW` singletons. `path_layout` / `path_views` are unshadowable. +- 0.3 **Flavor from day one:** `View` takes the path class it builds its locations with - + `Path` for the machine this code runs on, `PurePosixPath` / `PureWindowsPath` for any other - + even though every view is posix today. Retrofitting flavor after ~50 call sites exist is the + expensive version. - 0.4 Verification per chunk: `pytest shared`, `pytest airflow_src`, `pytest webapp` - (conda env `alphakraken2`, `AIRFLOW_HOME` exported, `--ignore` the docker job handler tests - unless `docker` is installed). + (conda env `alphakraken`, `AIRFLOW_HOME` exported and `airflow db init` run once, `--ignore` + the docker job handler tests unless `docker` is installed). ## 1. Chunks ### C1 - `shared/path_layout.py`, output layout moved -- **Goal:** one frame-free home for the output layout. +- **Goal:** one view-free home for the output layout. - **Do:** move `get_output_folder_rel_path` (`airflow_src/plugins/common/paths.py:39-63`) verbatim into `shared/path_layout.py`. Move `OUTPUT_FOLDER_PREFIX` with it, or import it. Repoint the 2 callers (`common/paths.py:78`, `processor_impl.py:29,180`). @@ -46,48 +47,61 @@ deliberately not started here; D2 only has to avoid blocking it. - **Risk:** `get_backup_base_path` feeds `RawFile.backup_base_path` in the DB. Assert the produced strings are byte-identical in a test before and after. -### C3 - `shared/path_frames.py`, no callers +### C3 - `shared/path_views.py`, no callers -- **Goal:** the frame table exists and is tested, nothing uses it yet. +- **Goal:** the view table exists and is tested, nothing uses it yet. - **Do:** - - `Root` enum: `INSTRUMENTS, BACKUP, OUTPUT, SETTINGS, SOFTWARE, SLURM, LOGS`. - - `Frame(name, flavor, roots: dict[Root, PurePath])` with `.resolve(root, rel) -> PurePath`, - `.has(root) -> bool`, and a `KeyError` naming both frame and root when a root is absent + - `Locations` constants: `INSTRUMENTS, BACKUP, OUTPUT, SETTINGS, SOFTWARE, SLURM, LOGS`. + A `ConstantsClass` like `InternalPaths` and `JobEngines`, not an enum: `StrEnum` needs + Python 3.11 and the python version of `apache/airflow:2.11.0` is unverified. + - `View(name, locations: dict[str, str], path_class)` with `.resolve(location, rel) -> PurePath`, + `.has(location) -> bool`, and a `KeyError` naming both view and location when a location is absent (this is where the §2 holes become explicit). - - Instances: `CONTAINER` from `InternalPaths` (`shared/keys.py:48-56`), `CLUSTER` from - `locations..absolute_path`, `HOST` from `locations.general.mounts_path` plus the - container-relative root names. - - `CLUSTER`/`HOST` construction reads yaml lazily, matching today's `get_path` behavior. -- **Done when:** unit tests cover resolve, the missing-root error, and a windows-flavor frame + - `AIRFLOW_CONTAINER_VIEW` is a module constant built from `InternalPaths` (`shared/keys.py:48-56`), carrying + `Path` because it is the only view this code does filesystem I/O in. + - `CLUSTER_VIEW` (from `locations..absolute_path`) and `DOCKER_HOST_VIEW` (from + `locations.general.mounts_path` plus the container-relative location names) are constants too, + built at import. `airflow_src/tests/helpers.py:yaml_locations` overrides them by patching the + contents of the view object, not the name, so the patch reaches every import site. Both carry + `PurePosixPath`, which also makes a foreign path impossible to `stat()` by accident. + - A missing `locations.general.mounts_path` yields a `DOCKER_HOST_VIEW` that reaches nothing, + rather than an error: the key is needed by the `docker` job engine only, and raising at import + would break every slurm-only deployment that omits it. +- **Done when:** unit tests cover resolve, the missing-location error, and a windows-flavor view resolving to `\\srv\share\backup\...` and `Z:\backup\...` from the same layout input. -- **Note:** `Root.LOGS` exists in the cluster and host frames only for prod/sandbox; the + Needs a `shared/tests/conftest.py` setting `ENV_NAME=_test_`: CI runs `pytest shared` on its + own, and importing the module loads the yaml. +- **Note:** `Locations`, `InternalPaths.{INSTRUMENTS,BACKUP,OUTPUT}` and `YamlKeys.Locations.*` + are three copies of the same strings until C4/C5 delete the latter two. A test guards them + against divergence in the meantime. +- **Note:** `Locations.LOGS` exists in the cluster and host views only for prod/sandbox; the container log path `/opt/airflow/logs` is outside `MOUNTS_PATH` and stays out of the table. -### C4 - Container frame cutover, API unchanged +### C4 - Container view cutover, API unchanged - **Goal:** `common/paths.py` stops knowing `InternalPaths`. - **Do:** reimplement `get_internal_*` (5 functions) as one-liners over - `CONTAINER.resolve(...)` plus `path_layout`. Their signatures and return types stay identical, + `AIRFLOW_CONTAINER_VIEW.resolve(...)` plus `path_layout`. Their signatures and return types stay identical, so the ~41 call sites in 12 files are untouched. -- **Done when:** `InternalPaths` is imported only by `path_frames.py` +- **Done when:** `InternalPaths` is imported only by `path_views.py` (plus `docker_job_handler.py` until C6). -### C5 - Cluster frame cutover, `get_path` removed +### C5 - Cluster view cutover, `get_path` removed - **Goal:** the cluster view has exactly one entry point. - **Do:** replace the 6 sites - `processor_impl.py:122,178,188,272`, `handler_impl.py:328`, - `job_handler.py:23` - with `CLUSTER.resolve(Root.X, ...)`. Delete + `job_handler.py:23` - with `CLUSTER_VIEW.resolve(Locations.X, ...)`. Delete `shared/yamlsettings.py:get_path` and repoint its test helpers. -- **Done when:** `grep get_path(` returns nothing outside `path_frames.py`. +- **Done when:** `grep get_path(` returns nothing outside `path_views.py`. - **Risk:** the only chunk touching the strings exported to the cluster (`QuantingEnv.raw_file_path`, `settings_path`, `output_path`, `custom_command`). Compare a full `QuantingEnv.to_dict()` before/after in a test. -### C6 - Host frame cutover +### C6 - Host view cutover - **Goal:** remove the container->host `relative_to` round-trip. - **Do:** `DockerJobHandler._to_host_path` (`docker_job_handler.py:185-197`) becomes - `HOST.resolve(root, rel)`; the handler is constructed with the frame rather than with + `DOCKER_HOST_VIEW.resolve(location, rel)`; the handler is constructed with the view rather than with `get_host_mounts_path()` (`job_handler.py:35`). Delete `get_host_mounts_path`. - **Done when:** `shared/yamlsettings.py` no longer exports any path accessor. @@ -96,11 +110,11 @@ deliberately not started here; D2 only has to avoid blocking it. - **Goal:** attack §6.6 - `InternalPaths`, `docker-compose.yaml` mount targets and yaml `mount_target` agree by convention only. - **Do:** a test that parses the volume lists in `docker-compose.yaml:451-538` and each - `envs/alphakraken.*.yaml`, and asserts every `CONTAINER` root reachable by the worker is + `envs/alphakraken.*.yaml`, and asserts every `AIRFLOW_CONTAINER_VIEW` location reachable by the worker is actually mounted there and that `mount_target` matches. Note the mounts are per service and, for `instruments`/`backup`, per instrument - (`:460,462,520,521,537,538`), so the assertion is "every root has a mount whose target is that - root or a child of it", not a set equality. + (`:460,462,520,521,537,538`), so the assertion is "every location has a mount whose target is that + location or a child of it", not a set equality. - **Done when:** the test fails on the production `backup` mount-depth discrepancy reported in `BOYSCOUT_20260901_081142.md`, or that discrepancy is resolved first and the test guards it. - **Note:** decide the production `backup` question (`//samba-pool-1/pool-1` vs @@ -125,7 +139,7 @@ they progressively empty `yamlsettings` and `InternalPaths`. whitelist-by-field-name (D3/D4). - `check_for_malicious_content` flavor awareness (needed before any Windows path is exported, cf. design §6.1 - but nothing exports one until D3). -- The DB-persisted frames (`RawFile.backup_base_path`, `Metrics.output_path`, +- The DB-persisted views (`RawFile.backup_base_path`, `Metrics.output_path`, `RawFile.file_info` keys) and the 6 webapp TODOs (D5). -- Generating `mount.sh` from the frame table. +- Generating `mount.sh` from the view table. - The msqc-extractor container. diff --git a/docs/deployment.md b/docs/deployment.md index 1b3c8298..ba89d383 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -199,7 +199,7 @@ The first view ("worker PC view") enables read/write access, by mounting on the AlphaKraken host PC a specific (network) folder (e.g. `\\pool-backup\pool-backup` or `\\pool-output\pool-output`) using `cifs` mounts (wrapped by `mount.sh`) to a target folder and then mapping this target folder to a worker container -in `docker-compose.yaml`, such that it can be accessed in a unified manner from within the containers (cf. `InternalPaths`). +in `docker-compose.yaml`, such that it can be accessed in a unified manner from within the containers (cf. the `AIRFLOW_CONTAINER_VIEW` view in `shared/path_views.py`). The second view ("cluster view") is the location of the data on the shared filesystem as seen from the Slurm cluster (e.g. `/fs/pool/pool-backup` or `/fs/pool/pool-output`), diff --git a/envs/alphakraken.local.yaml b/envs/alphakraken.local.yaml index 322adb77..b440491f 100644 --- a/envs/alphakraken.local.yaml +++ b/envs/alphakraken.local.yaml @@ -5,7 +5,7 @@ instruments: # the following is required for mounting only (read by mount.sh): username: user mount_src: //0.0.0.0/test1/ - mount_target: instruments/test1 # DO NOT CHANGE THE 'instruments' part -> InternalPaths.INSTRUMENTS + mount_target: instruments/test1 # DO NOT CHANGE THE 'instruments' part -> path_views.Locations.INSTRUMENTS # skip_processing: true # optional, default: false # skip_quanting: true # optional, default: false # min_free_space_gb: 100 # optional, default: taken from Airflow variable MIN_FREE_SPACE_GB. Set to -1 to exclude from file removing. @@ -23,12 +23,12 @@ locations: # xxx: # username: user # username for mounting # mount_src: //mount_src/xxx # source path for mounting ("worker pc view" part 1) - # mount_target: xxx # DO NOT CHANGE! this is related to the file structure within the containers -> cf. InternalPaths ("worker pc view" part 2) + # mount_target: xxx # DO NOT CHANGE! this is related to the file structure within the containers -> cf. path_views.Locations ("worker pc view" part 2) # absolute_path: /fs/pool/pool-0/alphakraken_sandbox/xxx # this is the absolute path of this folder on the shared file system ("cluster view") backup: username: user mount_src: //mount_src/backup - mount_target: backup # DO NOT CHANGE! relative to mounts_path -> InternalPaths.BACKUP + mount_target: backup # DO NOT CHANGE! relative to mounts_path -> path_views.Locations.BACKUP absolute_path: /fs/pool/pool-0/alphakraken_sandbox/backup settings: # no need for mount information (yet) @@ -44,7 +44,7 @@ locations: output: username: user mount_src: //mount_src/output - mount_target: output # DO NOT CHANGE! relative to mounts_path -> InternalPaths.OUTPUT + mount_target: output # DO NOT CHANGE! relative to mounts_path -> path_views.Locations.OUTPUT absolute_path: /fs/pool/pool-0/alphakraken_sandbox/output logs: username: user diff --git a/shared/keys.py b/shared/keys.py index d5b0762b..d65a50b6 100644 --- a/shared/keys.py +++ b/shared/keys.py @@ -51,10 +51,6 @@ class InternalPaths(metaclass=ConstantsClass): MOUNTS_PATH = "/opt/airflow/mounts/" ENVS_PATH = "/opt/airflow/envs/" - INSTRUMENTS = "instruments" - BACKUP = "backup" - OUTPUT = "output" - class SoftwareTypes(metaclass=ConstantsClass): """Types of software that can be used for quanting.""" diff --git a/shared/path_views.py b/shared/path_views.py new file mode 100644 index 00000000..634abab5 --- /dev/null +++ b/shared/path_views.py @@ -0,0 +1,116 @@ +"""Views on the data directories: the same tree, seen from different machines.""" + +from pathlib import Path, PurePath, PurePosixPath +from typing import Generic, TypeVar + +from shared.keys import ConstantsClass, InternalPaths +from shared.yamlsettings import YAMLSETTINGS, YamlKeys + + +class Locations(metaclass=ConstantsClass): + """The data directories that can be addressed within a view. + + The values are the keys of the `locations` section of the yaml settings, and at the same time + the folder names below the mounts folder within the containers. + """ + + INSTRUMENTS = "instruments" + BACKUP = "backup" + OUTPUT = "output" + SETTINGS = "settings" + SOFTWARE = "software" + SLURM = "slurm" + LOGS = "logs" + + +# the path flavor a view is expressed in, `Path` for the machine this code runs on +_P = TypeVar("_P", bound=PurePath) + +# the locations that are mounted into the containers, cf. docker-compose.yaml +_MOUNTED_LOCATIONS = (Locations.INSTRUMENTS, Locations.BACKUP, Locations.OUTPUT) + + +class View(Generic[_P]): + """The absolute paths of the data directories as seen from one machine (container, cluster, ..). + + Not every location is reachable from every machine. + """ + + def __init__( + self, name: str, locations: dict[str, str], path_class: type[_P] + ) -> None: + """Initialize the View. + + :param name: name of the view, used in error messages + :param locations: absolute path of each reachable location, cf. `Locations` + :param path_class: `Path` for the machine this code runs on, a `PurePath` flavor otherwise + """ + self._name = name + self._locations = { + location: path_class(path) for location, path in locations.items() + } + + def has(self, location: str) -> bool: + """Whether the given `location` is reachable in this view.""" + return location in self._locations + + 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 + + +AIRFLOW_CONTAINER_VIEW: View[Path] = View( + "airflow container", + { + location: f"{InternalPaths.MOUNTS_PATH}{location}" + for location in _MOUNTED_LOCATIONS + }, + Path, +) + + +def _build_cluster_view() -> View[PurePosixPath]: + """Build the view of a machine that accesses the data via the shared file system.""" + locations: dict[str, dict[str, str]] = YAMLSETTINGS.get(YamlKeys.LOCATIONS, {}) # type: ignore[invalid-assignment] + + absolute_paths = { + location: values[YamlKeys.ABSOLUTE_PATH] + for location, values in locations.items() + if YamlKeys.ABSOLUTE_PATH in values + } + + return View("cluster", absolute_paths, PurePosixPath) + + +def _build_docker_host_view() -> View[PurePosixPath]: + """Build the view from within the processing (e.g. msqc) docker containers. + + Note this is not the airflow containers, they use InternalPaths. + 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 = ( + YAMLSETTINGS.get(YamlKeys.LOCATIONS, {}) # type: ignore[possibly-unbound-attribute] + .get(YamlKeys.Locations.GENERAL, {}) + .get(YamlKeys.Locations.MOUNTS_PATH) + ) + + locations = ( + {} + if mounts_path is None + else {location: f"{mounts_path}/{location}" for location in _MOUNTED_LOCATIONS} + ) + + return View("docker host", locations, PurePosixPath) + + +CLUSTER_VIEW: View[PurePosixPath] = _build_cluster_view() +DOCKER_HOST_VIEW: View[PurePosixPath] = _build_docker_host_view() diff --git a/shared/tests/conftest.py b/shared/tests/conftest.py new file mode 100644 index 00000000..93382580 --- /dev/null +++ b/shared/tests/conftest.py @@ -0,0 +1,5 @@ +"""Shared code for all tests.""" + +import os + +os.environ["ENV_NAME"] = "_test_" diff --git a/shared/tests/test_path_views.py b/shared/tests/test_path_views.py new file mode 100644 index 00000000..59e9a44f --- /dev/null +++ b/shared/tests/test_path_views.py @@ -0,0 +1,158 @@ +"""Tests for the path_views module.""" + +from pathlib import Path, PurePosixPath, PureWindowsPath +from unittest.mock import patch + +import pytest + +from shared.path_views import ( + AIRFLOW_CONTAINER_VIEW, + Locations, + View, + _build_cluster_view, + _build_docker_host_view, +) +from shared.yamlsettings import YAMLSETTINGS, YamlKeys + + +def test_resolve() -> None: + """Test that a relative path is resolved against the location it belongs to.""" + view = View("some_view", {Locations.BACKUP: "/some/backup"}, PurePosixPath) + + # when + result = view.resolve(Locations.BACKUP, Path("test1/1970_01/some_file.raw")) + + assert result == PurePosixPath("/some/backup/test1/1970_01/some_file.raw") + + +def test_resolve_without_rel_path() -> None: + """Test that omitting the relative path yields the location itself.""" + view = View("some_view", {Locations.BACKUP: "/some/backup"}, PurePosixPath) + + assert view.resolve(Locations.BACKUP) == PurePosixPath("/some/backup") + + +def test_resolve_raises_on_unreachable_location() -> None: + """Test that resolving a location that the view does not have names both view and location.""" + view = View("some_view", {Locations.BACKUP: "/some/backup"}, PurePosixPath) + + with pytest.raises( + KeyError, match="'settings' is not reachable in the 'some_view' view" + ): + view.resolve(Locations.SETTINGS) + + +def test_has() -> None: + """Test that a view reports which locations it can reach.""" + view = View("some_view", {Locations.BACKUP: "/some/backup"}, PurePosixPath) + + assert view.has(Locations.BACKUP) + assert not view.has(Locations.SETTINGS) + + +def test_resolve_windows_unc_path() -> None: + """Test that a windows view resolves a relative path to a UNC path.""" + view = View( + "some_view", {Locations.BACKUP: "//some_server/some_share"}, PureWindowsPath + ) + + # when + result = view.resolve( + Locations.BACKUP, PurePosixPath("test1/1970_01/some_file.raw") + ) + + assert str(result) == r"\\some_server\some_share\test1\1970_01\some_file.raw" + + +def test_resolve_windows_drive_letter_path() -> None: + """Test that a windows view resolves a relative path to a mapped drive path.""" + view = View("some_view", {Locations.BACKUP: "Z:/backup"}, PureWindowsPath) + + # when + result = view.resolve( + Locations.BACKUP, PurePosixPath("test1/1970_01/some_file.raw") + ) + + assert str(result) == r"Z:\backup\test1\1970_01\some_file.raw" + + +def test_container_view() -> None: + """Test that the container view resolves to the mounts folder, as a local path.""" + result = AIRFLOW_CONTAINER_VIEW.resolve( + Locations.BACKUP, "test1/1970_01/some_file.raw" + ) + + assert result == Path("/opt/airflow/mounts/backup/test1/1970_01/some_file.raw") + + +def test_container_view_has_only_the_mounted_locations() -> None: + """Test that the locations that are not mounted into the containers are absent.""" + assert [ + AIRFLOW_CONTAINER_VIEW.has(location) + for location in [Locations.INSTRUMENTS, Locations.BACKUP, Locations.OUTPUT] + ] == [True, True, True] + assert [ + AIRFLOW_CONTAINER_VIEW.has(location) + for location in [ + Locations.SETTINGS, + Locations.SOFTWARE, + Locations.SLURM, + Locations.LOGS, + ] + ] == [False] * 4 + + +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"}, + Locations.BACKUP: {YamlKeys.ABSOLUTE_PATH: "/some/pool/backup"}, + Locations.SETTINGS: {YamlKeys.ABSOLUTE_PATH: "/some/pool/settings"}, + } + + with patch.dict(YAMLSETTINGS, {YamlKeys.LOCATIONS: locations}): + view = _build_cluster_view() + + assert view.resolve(Locations.BACKUP, "test1/1970_01") == PurePosixPath( + "/some/pool/backup/test1/1970_01" + ) + assert view.resolve(Locations.SETTINGS) == PurePosixPath("/some/pool/settings") + # the `general` section carries no absolute path and is not a location + assert not view.has(YamlKeys.Locations.GENERAL) + assert not view.has(Locations.OUTPUT) + + +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}): + view = _build_docker_host_view() + + assert view.resolve(Locations.OUTPUT, "P1/out_some_file.raw") == PurePosixPath( + "/some/mounts/output/P1/out_some_file.raw" + ) + 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: {}}): + view = _build_docker_host_view() + + assert not view.has(Locations.OUTPUT) + with pytest.raises(KeyError, match="not reachable in the 'docker host' view"): + view.resolve(Locations.OUTPUT) + + +def test_locations_agree_with_the_yaml_key_names() -> None: + """Test that the two vocabularies for the same folder names have not diverged.""" + assert { + YamlKeys.Locations.BACKUP, + YamlKeys.Locations.SETTINGS, + YamlKeys.Locations.OUTPUT, + YamlKeys.Locations.SLURM, + YamlKeys.Locations.SOFTWARE, + } <= set(Locations.get_values())