Workflow simplification

.github/workflows/tests.yml

@@ -0,0 +1,38 @@
+name: tests
+
+on:
+  push:
+    branches: [main]
+  pull_request:
+
+jobs:
+  test:
+    runs-on: ubuntu-latest
+    strategy:
+      fail-fast: false
+      matrix:
+        python-version: ["3.12", "3.13"]
+
+    defaults:
+      run:
+        # Login shell so the micromamba environment stays activated across steps.
+        shell: bash -el {0}
+
+    steps:
+      - uses: actions/checkout@v4
+
+      # ecflow is not on PyPI; it is installed from conda-forge via the
+      # environment file. river-route and the rest come along with it.
+      - name: Create environment
+        uses: mamba-org/setup-micromamba@v2
+        with:
+          environment-file: environment.yml
+          create-args: >-
+            python=${{ matrix.python-version }}
+          cache-environment: true
+
+      - name: Install package with dev dependencies
+        run: pip install -e ".[dev]"
+
+      - name: Run tests
+        run: pytest -q --cov=geoglows_ecflow.resources --cov-report=term-missing

.gitignore

@@ -1,13 +1,15 @@
 # geoglows_ecflow
+# Deployment configs hold secrets and stay untracked; the example is tracked.
 *config.yml
+*config.yaml
+!config.example.yaml
 *ecflow_start.sh
 *data/
 *log/
 *geoglows_forecast/
 ecflow_home/
 *geoglows_forecast.def
 *.ecf.*
-*.cfg
 
 # pytest and coverage
 .coverage

PLAN.md

@@ -0,0 +1,127 @@
+# Workflow Simplification Plan
+
+A living plan for simplifying the `geoglows_ecflow` workflow. Branched off the
+`rapid-to-river-route` work (PR #27), so all references below assume the
+river-route codebase, not the RAPID `main`.
+
+## Goals
+
+1. **Reduce complexity** — eliminate duplication, simplify functions, separate
+   inputs from logic.
+2. **Make implicit explicit** — name constants and configuration, split mixed
+   functions, document non-obvious behavior.
+3. **Improve maintainability** — make different configurations easy to run and
+   add a unit-test safety net.
+
+## Scope
+
+- **In scope:** `geoglows_ecflow/resources/*.py`,
+  `geoglows_ecflow/workflow/builders/builder.py`,
+  `geoglows_ecflow/workflow/parts/*`, and the `.ecf` task scripts.
+- **Mostly frozen:** `geoglows_ecflow/workflow/comfies/*` — vendored ECMWF
+  framework code (Apache 2.0, ~4,700 lines). Left untouched except for the
+  minimal Python-3.12+ compatibility fixes noted below (the suite could not be
+  imported at all without them) and the deploy-config format switch (see
+  Decisions: config format).
+
+## Decisions
+
+- **Tests:** unit tests on the pure functions, plus in-memory
+  suite-definition smoke tests (these were originally deferred but pulled
+  forward to guard the builder refactor).
+- **HRES member:** high-resolution is always ensemble member **52**, kept as a
+  single named constant `HRES_ENSEMBLE_MEMBER` rather than a configurable value.
+- **Config format:** the deploy config is now plain **YAML** instead of the old
+  `.cfg` (Python source executed via `SourceFileLoader`). `gdeploy` reads
+  `config.yaml`; `config.example.yaml` is the committed template. This is a
+  clean break — the Python-config path was removed, not kept as a fallback. It
+  required touching the otherwise-frozen comfies loader (`config.py`,
+  `sdeploy.py`): the broken `YAMLConfigFile` was repaired and a `YAMLConfigPath`
+  added; `PythonConfigFile`/`PythonConfigPath` and `_load_source` were deleted.
+  The `Config` wrapper and `builder.py` are unchanged — YAML loads into the same
+  nested-dict shape. Computed values the Python format allowed (f-strings,
+  variable reuse) must now be written out as literals.
+
+---
+
+## Phase 1 — Pure cleanup (no behavior change) — DONE
+
+- [x] `builder.py`: fix `Task("dimmy")` typo.
+- [x] `helper_functions.py`: fix `create_logger` so file logging attaches.
+- [x] `builder.py`: remove duplicate imports and the duplicated `nodes` import.
+- [x] `builder.py`: remove read-but-unused config vars.
+- [x] `builder.py`: fix stale docstring ("GLOFAS suite" → GEOGloWS).
+- [x] `generate_esri_table.py`: fix `int or str` type hint.
+- [x] Standardize the `argparse(nargs=1)` + `args.x[0]` antipattern.
+
+## Phase 2 — Test harness + CI — DONE
+
+- [x] Add `pytest` (+ `pytest-cov`) as a dev dependency.
+- [x] Create `tests/` with fixtures.
+- [x] Unit tests for the pure functions (ensemble parsing, VPU listing, date
+      parsing, state-init lookback, forecast preprocess, return-period /
+      exceedance, init-flow time index, zarr round-trip).
+- [x] GitHub Actions workflow (`.github/workflows/tests.yml`). Now builds a
+      conda env from `environment.yml` (ecflow has no PyPI wheel) on a
+      Python **3.12–3.13** matrix and runs the whole suite.
+
+## Phase 3 — Centralize duplication — DONE
+
+- [x] Shared zarr-writing helper (`resources/zarr_io.py`).
+- [x] `helper_functions.load_forecast_run` loader.
+- [x] `RETURN_PERIODS` defined once and used for the ladders.
+- [x] Standardized `logging` setup (`configure_logging`).
+
+## Phase 4 — Make implicit explicit — MOSTLY DONE
+
+- [x] `HRES_ENSEMBLE_MEMBER = 52` — used in `netcdf_to_zarr.py` and the
+      `range(1, HRES_ENSEMBLE_MEMBER + 1)` ensemble loop in `builder.py`.
+- [ ] `nco_calc.ecf` `grep -v ..._52.nc` (×3) — **decision pending**: wire an
+      ecflow `%HRES_MEMBER%` variable vs. leave `52` + a comment. Highest risk:
+      wrong wiring silently changes which member is excluded from the mean.
+- [x] `EMOS_BASE != "12"` gate (×3) → `is_00z_cycle()` helper.
+- [x] Magic numbers named: thickness ladder (`THICKNESS_THRESHOLDS`),
+      stream-order (`MIN_STREAM_ORDER`), 10-day window (`FORECAST_WINDOW_DAYS`),
+      `MEM` values (`ENS_TASK_MEM_MB` / `ARCHIVE_QINIT_MEM_MB`).
+- [x] Timer offsets (`hours=7`/`hours=9`/`"14:15"`) — **resolved by deletion**:
+      they lived only in the broken `rd`/research-mode branch, which has been
+      removed (see follow-ups). Nothing to extract; revisit if `rd` returns.
+- [x] Consolidate `self.config.get(...)` reads in `builder.py` into one
+      documented block.
+
+## Follow-ups (later tasks)
+
+- ecFlow-**server** tests (building the def against a live server / `--dry`);
+  the in-memory structural tests are done, this is the heavier version.
+- README refresh (carried over from PR #27 review).
+- **comfies is incompatible with ecflow 5.17+.** Its node wrappers set
+  `Variable.parent` (`ooflow.py:1925`), which ecflow 5.17 made a read-only
+  built-in, so building any suite raises `AttributeError`. Worked around by
+  pinning `ecflow<5.17` in `environment.yml`; the real fix is to rename
+  comfies' parent-tracking attribute so it no longer collides.
+- **Research (`rd`) mode removed.** `mode='rd'` was the only path with
+  `follow_osuite=False` and crashed unconditionally at `barrier_hh.ymd`
+  (`barrier_hh` is a `NominalTime`, which has no `ymd`) — broken since the
+  original 2024-07-25 authoring, so never usable. Removed the `rd` choice, the
+  `follow_osuite`/`in_production`/`in_test` flags, the non-`follow_osuite`
+  branch (the `+7h`/`+9h`/`14:15` run timers), and the dead crash line. `prod`
+  and `test` are the remaining modes. If research mode is wanted again, it
+  should be reintroduced correctly (with the intended barrier-repeat wiring).
+
+---
+
+## Current status
+
+**Branch `workflow-simplification`** (fork `JakeGimenes`), open as a PR against
+`rapid-to-river-route`. Phases 1–3 complete; Phase 4 complete except the
+`nco_calc.ecf` decision. The timer constants are resolved by deletion (the
+`rd`/research mode that owned them has been removed). **33 pytest tests pass** —
+the resources tests run anywhere; the suite-definition tests require `ecflow`
+(conda-forge).
+
+The vendored `comfies` framework got the minimum Python-3.12+ compatibility
+fixes needed to import it at all (`imp` → `importlib`, `pkg_resources` →
+`packaging`); everything else in `comfies/*` is unchanged.
+
+**Remaining actionable work:** the `nco_calc.ecf` `HRES_MEMBER` decision and the
+README refresh.

README.md

@@ -18,78 +18,116 @@ pip install -e .
 
 ## Non-Python Dependencies
 
-- ecflow>=5.11.3
+- ecflow>=5.11.3,<5.17
 - nco>=5.1.8
 - ksh>=2020.0.0
 
-## geoglows_ecflow configuration file (config.cfg)
+## geoglows_ecflow configuration file (config.yaml)
 
-```python
-    name = 'suite_name'
-    srcroot = "/path/to/source"
-    first_date = first_barrier = 'YYYYMMDD'
-    vpu_list = []
-    mars_bond_id='251'
-    staticdata = '/path/to/assets'
-    workroot = f'/path/to/workroot'
-    mode = 'test'  # suite mode ('rd':research, 'test':test, 'prod':production)
-    expver = 'geoglows'
-    exparch = '/path/to/archive'
-    iniexparch = '/path/to/init_archive'
-    mars_workers = '3'
-    script_extension = '.ecf'
-
-    # suite's source code
-    source = dict(
-    root = srcroot,
-    builder = 'geoglows_ecflow.workflow.builders.builder',
-    includes = 'scripts/troika:suites/scripts/tems:{includes}',
-    scripts = 'scripts/tems:{scripts}'
-    )
-
-    # deploy location
-    target = dict(
-        root = "/path/to/deploy_location",
-    )
-
-    # where to run computations
-    jobs = dict(
-        manager = dict(
-            name='troika',
-        ),
-        root = '/path/to/job_root',
-        limit = 26,
-        destinations = dict(
-            default = dict(
-                host = '%SCHOST:ab%',
-                bkup_host = '%SCHOST_BKUP%',
-                user = 'user_name',
-                queue = 'nf',
-                account = 'ECACCOUNT',
-                sthost = 'sthost',
-            ),
-            parallel = dict(
-                host = '%SCHOST:ab%',
-                bkup_host = '%SCHOST_BKUP%',
-                user = user,
-                queue = 'nf',
-                ncpus = '12',
-                mem = '1000',
-            )
-        )
-    )
-
-    # --------------------------------------------
-    # Configuration of EFAS software packages
-    # which are installed together with the suite.
-    # --------------------------------------------
-    packages = dict(
-        scripts = dict(
-            srcdir = srcroot + 'scripts',
-        ),
-    )
+The deployment configuration is a plain YAML file. Copy
+[`config.example.yaml`](config.example.yaml) to `config.yaml` and edit the
+values for your environment (`config.yaml` is gitignored so secrets stay out
+of version control). Values like `%SCHOST:ab%` are ecFlow variables passed
+through verbatim, and `{includes}`/`{scripts}` are sdeploy search-path
+placeholders.
+
+```yaml
+name: suite_name
+mode: test                 # 'test' or 'prod'
+first_date: "YYYYMMDD"
+first_barrier: "YYYYMMDD"
+vpu_list: []
+ens_members: 51
+mars_workers: 3
+script_extension: ".ecf"
+
+expver: geoglows
+exparch: /path/to/archive
+iniexparch: /path/to/init_archive
+staticdata: /path/to/assets
+workroot: /path/to/workroot
+
+# suite's source code
+source:
+  root: /path/to/source
+  builder: geoglows_ecflow.workflow.builders.builder
+  includes: "scripts/troika:suites/scripts/tems:{includes}"
+  scripts: "scripts/tems:{scripts}"
+
+# deploy location
+target:
+  root: /path/to/deploy_location
+
+# where to run computations
+jobs:
+  manager:
+    name: troika
+  root: /path/to/job_root
+  limit: 26
+  destinations:
+    default:
+      host: "%SCHOST:ab%"
+      bkup_host: "%SCHOST_BKUP%"
+      user: user_name
+      queue: nf
+      account: ECACCOUNT
+      sthost: sthost
+    parallel:
+      host: "%SCHOST:ab%"
+      bkup_host: "%SCHOST_BKUP%"
+      user: user_name
+      queue: nf
+      ncpus: "12"
+      mem: "1000"
+
+# GEOGloWS software packages installed alongside the suite
+packages:
+  scripts:
+    srcdir: /path/to/source/scripts
+```
+
+## Troika job submission
+
+The suite submits jobs through [troika](https://github.com/ecmwf/troika), ECMWF's
+job-submission tool. On Atos, troika and its site configuration are provided by the
+system. To run **locally**, install the optional `troika` dependency and point the
+suite at a small local troika config.
+
+Install with the troika extra:
+
+```bash
+pip install .[troika]
 ```
 
+Create a local troika config (copy [`troika.example.yml`](troika.example.yml) to
+`troika.yml`) that runs jobs as plain local processes. The **site name must match the
+`host`** used in the config's job destinations:
+
+```yaml
+sites:
+  localhost:
+    type: direct        # run the job directly (no SLURM/PBS)
+    connection: local   # on this machine (no ssh)
+```
+
+Then add `executable` and `config` to the `jobs.manager` block of your `config.yaml`:
+
+```yaml
+jobs:
+  manager:
+    name: troika
+    executable: /path/to/troika    # output of `which troika`
+    config: /path/to/troika.yml
+  # ...
+  destinations:
+    default:
+      host: localhost              # must match the site name in troika.yml
+      user: your_user
+```
+
+With that, deploying and running the suite (see *Local run example*) submits every task
+through troika as a local process.
+
 ## AWS configuration file (aws_config.yml)
 
 ```yaml
@@ -117,12 +155,12 @@ ecflow_start.sh -d /path/to/ecflow_home
 Generate the suite definition (via CLI or Python):
 
 ```bash
-gdeploy --config /path/to/config.cfg
+gdeploy --config /path/to/config.yaml
 ```
 
 ```python
 from geoglows_ecflow.workflow.create import main
-main("/path/to/config.cfg")
+main("/path/to/config.yaml")
 ```
 
 Start a local ecflow server, then load and begin the suite: