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

PLAN.md

@@ -0,0 +1,116 @@
+# 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).
+
+## 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.
+
+---
+
+## 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,7 +18,7 @@ pip install -e .
 
 ## Non-Python Dependencies
 
-- ecflow>=5.11.3
+- ecflow>=5.11.3,<5.17
 - nco>=5.1.8
 - ksh>=2020.0.0
 
@@ -32,7 +32,7 @@ pip install -e .
     mars_bond_id='251'
     staticdata = '/path/to/assets'
     workroot = f'/path/to/workroot'
-    mode = 'test'  # suite mode ('rd':research, 'test':test, 'prod':production)
+    mode = 'test'  # suite mode ('test':test, 'prod':production)
     expver = 'geoglows'
     exparch = '/path/to/archive'
     iniexparch = '/path/to/init_archive'
@@ -80,7 +80,7 @@ pip install -e .
     )
 
     # --------------------------------------------
-    # Configuration of EFAS software packages
+    # Configuration of GEOGloWS software packages
     # which are installed together with the suite.
     # --------------------------------------------
     packages = dict(

environment.yml

@@ -3,9 +3,11 @@ channels:
   - conda-forge
   - defaults
 dependencies:
-  - python>=3.11,<3.14
+  - python>=3.12,<3.14
   - pip
-  - ecflow
+  # comfies' node wrappers set Variable.parent, which ecflow 5.17 made
+  # read-only; pin below that until comfies is updated.
+  - ecflow<5.17
   - nco
   - pyyaml
   - numpy

geoglows_ecflow/resources/archive_to_aws.py

@@ -1,11 +1,12 @@
 import argparse
 import glob
-import json
 import os
 
 import boto3
 import yaml
 
+from geoglows_ecflow.resources.helper_functions import load_forecast_run
+
 
 def upload_to_s3(workspace: str, aws_config_file: str):
     """
@@ -22,10 +23,9 @@ def upload_to_s3(workspace: str, aws_config_file: str):
         forecast_bucket_uri = config["bucket_forecast_archive"]
         mapstyletable_bucket_uri = config["bucket_maptable_archive"]
 
-    with open(os.path.join(workspace, "forecast_run.json"), "r") as f:
-        data = json.load(f)
-        date = data["date"]
-        output_dir = data["output_dir"]
+    data = load_forecast_run(workspace)
+    date = data["date"]
+    output_dir = data["output_dir"]
 
     # Create an S3 client
     s3 = boto3.client(
@@ -60,17 +60,15 @@ def upload_to_s3(workspace: str, aws_config_file: str):
     argparser = argparse.ArgumentParser()
     argparser.add_argument(
         "workspace",
-        nargs=1,
         help="Path to suite home directory",
     )
     argparser.add_argument(
         "aws_config_file",
-        nargs=1,
         help="Path to AWS config file",
     )
 
     args = argparser.parse_args()
-    workspace = args.workspace[0]
-    aws_config_file = args.aws_config_file[0]
+    workspace = args.workspace
+    aws_config_file = args.aws_config_file
 
     upload_to_s3(workspace, aws_config_file)

geoglows_ecflow/resources/combine_esri_tables.py

@@ -1,12 +1,13 @@
 import argparse
 import logging
-import sys
 
 import glob
 import os
 
 import pandas as pd
 
+from geoglows_ecflow.resources.helper_functions import configure_logging
+
 
 def combine_esri_tables(workspace: str):
     """Combines the map_style_tables from each VPU into 1 CSV file per time
@@ -56,10 +57,6 @@ def combine_esri_tables(workspace: str):
     args = argparser.parse_args()
     workspace = args.workspace[0]
 
-    logging.basicConfig(
-        level=logging.INFO,
-        format="%(asctime)s %(levelname)s %(message)s",
-        stream=sys.stdout,
-    )
+    configure_logging()
 
     combine_esri_tables(workspace)

geoglows_ecflow/resources/compute_init_flows.py

@@ -1,10 +1,11 @@
 import argparse
-import json
 import os
 
 import pandas as pd
 import xarray as xr
 
+from geoglows_ecflow.resources.helper_functions import load_forecast_run
+
 
 # Time index 7 of the ensemble-mean Q corresponds to t+24h on the ENS
 # 3h-resolution grid (the HRES member is excluded from the average upstream by
@@ -14,8 +15,7 @@
 
 
 def main(workspace: str, vpu: str) -> None:
-    with open(os.path.join(workspace, "forecast_run.json"), "r") as f:
-        ymd = json.load(f)["date"]
+    ymd = load_forecast_run(workspace)["date"]
 
     avg_path = os.path.join(workspace, "output", f"nces_avg_{vpu}.nc")
     out_path = os.path.join(workspace, "input", vpu, f"Qinit_{ymd}.parquet")

geoglows_ecflow/resources/concat_forecast_warnings.py

@@ -1,10 +1,11 @@
 import os
 import glob
-import sys
 import logging
 import pandas as pd
 import argparse
 
+from geoglows_ecflow.resources.helper_functions import configure_logging
+
 
 def concat_warnings(workdir: str) -> None:
     """
@@ -51,10 +52,6 @@ def concat_warnings(workdir: str) -> None:
     args = parser.parse_args()
     workspace = args.workspace[0]
 
-    logging.basicConfig(
-        level=logging.INFO,
-        format="%(asctime)s - %(name)s - %(levelname)s - %(message)s",
-        stream=sys.stdout,
-    )
+    configure_logging()
 
     concat_warnings(workspace)