diff --git a/tests/test_fao_delivery_runner.py b/tests/test_fao_delivery_runner.py index 389da0ad..157bd43d 100644 --- a/tests/test_fao_delivery_runner.py +++ b/tests/test_fao_delivery_runner.py @@ -109,6 +109,37 @@ def test_it_refuses_rather_than_proceeding(self, result): "publish credentials. It would then have proceeded to spend GPU hours." ) + def test_it_builds_the_publish_config_rather_than_counting_variables(self): + """The check that would have caught the real failure. + + PredictionStoreConfig.from_environment() requires NINE variables — the three secrets + plus PROD_FORECASTS_BUCKET_ID/NAME, PROD_FORECASTS_COLLECTION_ID/NAME and + METADATA_DATABASE_ID/NAME. The first version of this preflight checked the three + secrets, so it would have reported ready with six of nine missing and the run would + have failed at the publish, after the whole roster trained. + + Counting variables cannot be made correct by adding six more names either: the extra + can be absent, the endpoint unreachable, the key expired (#359 — 2026-11-17). Only + constructing the thing answers the question. + """ + code = _code_only(FAO.read_text()) + assert "PredictionStoreConfig" in code and "from_environment()" in code, ( + "preflight must CONSTRUCT the publish config, not enumerate variable names" + ) + assert "import appwrite" in code, ( + "preflight must also confirm the appwrite extra is importable — variables can all " + "be set while the SDK that uses them is missing (#517)" + ) + + def test_it_tells_the_operator_the_variables_must_be_exported(self): + """pipeline-core stopped auto-loading a .env from the working directory (#346, C-177), + so a correct .env sitting beside the operator is not enough. Without this line the + failure looks like wrong credentials rather than unexported ones.""" + code = _code_only(FAO.read_text()) + assert re.search(r"set -a", code) and re.search(r"C-177|#346", FAO.read_text()), ( + "preflight must say the nine variables need exporting, and name why" + ) + def test_it_names_the_missing_publish_secrets(self, result): out = result.stdout + result.stderr for var in ("APPWRITE_ENDPOINT", "APPWRITE_DATASTORE_PROJECT_ID", @@ -130,6 +161,27 @@ def test_it_checks_conda_because_the_postprocessor_needs_it(self, result): "(tools/launcher/postprocessor.sh:72) and step 4 is the last step." ) + def test_the_disk_floor_is_not_below_the_floor_it_delegates_to(self): + """Found by a falsification pass on the forecasting claim. + + This script delegates to pod_run_model.sh eight times, and that script refuses below + 40GB for ONE model. A floor below 40 here is a preflight that reports ready and then + refuses at model 3 — the exact opposite of why this mode exists. The first version + said 60GB for all eight, which was lower than the per-model transient for a run eight + times the size. + """ + fao = _code_only(FAO.read_text()) + model = _code_only((REPO / "tools" / "podrun" / "pod_run_model.sh").read_text()) + delegated = re.search(r'AVAIL_GB" -ge (\d+)', model) + assert delegated, "pod_run_model.sh's disk floor moved; re-read it" + mine = re.search(r"DISK_FLOOR_GB=(\d+)", fao) + assert mine, "this script no longer declares DISK_FLOOR_GB" + assert int(mine.group(1)) >= int(delegated.group(1)), ( + f"this orchestrator demands {mine.group(1)}GB for eight models while the script it " + f"delegates to demands {delegated.group(1)}GB for one. Preflight would pass and a " + "later model would refuse, after hours." + ) + def test_it_reports_every_problem_not_only_the_first(self, result): """A preflight that dies on the first missing thing costs a round trip per problem, on hardware billed by the second.""" diff --git a/tools/podrun/pod_run_fao_delivery.sh b/tools/podrun/pod_run_fao_delivery.sh index 54467cb0..ed794fed 100755 --- a/tools/podrun/pod_run_fao_delivery.sh +++ b/tools/podrun/pod_run_fao_delivery.sh @@ -126,19 +126,62 @@ if [ -s /root/.netrc ] && [ "$(stat -c %a /root/.netrc 2>/dev/null)" != "600" ]; echo " fixed: /root/.netrc was not 600" fi -# The publish credentials. Three of the nine are real secrets; the rest are identifiers. -# Checked HERE and not at first publish, because views-pipeline-core's own -# PredictionStoreConfig claims to read these "once at startup and fail loud" and does not — -# it is called from _build_datastore, i.e. after training (views-pipeline-core#557). Until -# that moves, this is the only check that happens before the money is spent. +# The publish credentials. NINE variables are required, not three — measured, not assumed: +# PredictionStoreConfig.from_environment() names ENDPOINT, DATASTORE_PROJECT_ID, +# DATASTORE_API_KEY, PROD_FORECASTS_BUCKET_ID, PROD_FORECASTS_BUCKET_NAME, +# PROD_FORECASTS_COLLECTION_ID, PROD_FORECASTS_COLLECTION_NAME, METADATA_DATABASE_ID and +# METADATA_DATABASE_NAME. Only the first three are secrets; the rest are identifiers, and a +# missing identifier fails the publish exactly as hard as a missing secret. +# +# The first version of this block checked the three secrets only. It would have passed with six +# of nine missing, and the run would have failed at the publish — after the entire roster +# trained. The authoritative check is the CONSTRUCTION below; this loop survives only to give a +# faster, friendlier message for the case an operator hits most often. for v in APPWRITE_ENDPOINT APPWRITE_DATASTORE_PROJECT_ID APPWRITE_DATASTORE_API_KEY; do eval "val=\${$v:-}" [ -n "$val" ] || note "\$$v — a publish secret; see reports/fao_delivery_runbook.md" done +# They must be EXPORTED, not merely present in a .env beside you: pipeline-core stopped +# auto-loading a .env from the working directory (#346, register C-177), because a library +# reading whatever .env its caller happens to be standing in is what the Appwrite seam contract +# §3 forbids. `set -a; . /root/.secrets; set +a` is the shape that works. case "${APPWRITE_DATASTORE_API_KEY:-}" in *[![:print:]]*) note "\$APPWRITE_DATASTORE_API_KEY contains a non-printable character — re-paste it" ;; esac +# And then BUILD the thing, rather than concluding from three non-empty strings that it will +# build. Variables being set is not the same as the store being constructible: the appwrite +# extra can be missing, the endpoint can be unreachable, the key can be expired (#359: this one +# expires 2026-11-17). Every one of those surfaces at `_build_datastore`, whose first caller is +# the publish — i.e. after the entire roster has trained (views-pipeline-core#557). +# +# Suggested by the views-pipeline-core session, which pointed out this is exactly what #557 +# argues the manager should do itself, and that doing it by hand costs nothing until it does. +if [ -x "$VENV/bin/python" ] && [ "$FAIL" = "0" ]; then + if PUBERR=$("$VENV/bin/python" - <<'PUBCHECK' 2>&1 +import sys +try: + from views_pipeline_core.configs.prediction_store import PredictionStoreConfig +except Exception as e: + sys.exit("cannot import PredictionStoreConfig: %s: %s" % (type(e).__name__, e)) +try: + cfg = PredictionStoreConfig.from_environment() +except Exception as e: + sys.exit("PredictionStoreConfig.from_environment() failed: %s: %s" % (type(e).__name__, e)) +try: + import appwrite # noqa: F401 +except Exception as e: + sys.exit("the appwrite extra is not installed: %s: %s" % (type(e).__name__, e)) +print("publish config builds") +PUBCHECK + ); then + echo " publish path: $PUBERR" + else + note "the publish path does not build — this WOULD have failed after the whole roster trained: + $PUBERR" + fi +fi + # conda, for the postprocessor leg ONLY. tools/launcher/postprocessor.sh uses # `conda shell.bash hook` / `conda create --prefix` / `conda activate`, while this pod builds # a uv venv — so the two legs need different interpreters and a pod can satisfy one and not @@ -158,10 +201,32 @@ fi nvidia-smi -L >/dev/null 2>&1 || note "no GPU visible" AVAIL_GB=$(df -BG --output=avail "$ROOT" 2>/dev/null | tail -1 | tr -dc '0-9') -# Eight forecasting runs plus the pooled ensemble, on one disk. -[ "${AVAIL_GB:-0}" -ge 60 ] || note "only ${AVAIL_GB:-0}GB free on $ROOT; eight forecasts plus the pool need ~60GB" +# DISK_FLOOR_GB, and the honest state of what it is based on. +# +# pod_run_model.sh refuses below 40GB for ONE model ("one model needs ~20GB"), and this script +# delegates to it eight times — so 40 is a hard lower bound that will be re-checked at every +# model whatever is written here. A floor BELOW the floor of the thing it delegates to is a +# preflight that says "ready" and then refuses at model 3, which is the opposite of this +# script's purpose. The first version of this check said 60 for all eight, which was exactly +# that mistake: lower than the per-model transient for a run eight times the size. +# +# The retained component is an ESTIMATE and cannot be better than that yet: measured on this +# machine, one model's calibration output is ~2.5GB per predictions directory, and NO +# FORECASTING RUN HAS EVER COMPLETED ON THIS ROSTER, so the retained size of a forecast is +# unmeasured. A forecast has one origin against calibration's 13, so it should be smaller — +# "should be" is doing real work in that sentence. +# +# 40 transient + 8 x ~5GB retained, rounded up for the pooled ensemble, which is also +# unmeasured. Revise this number from the first completed run rather than reasoning about it +# again; that is the whole of the trigger. +DISK_FLOOR_GB=80 +[ "${AVAIL_GB:-0}" -ge "$DISK_FLOOR_GB" ] || note "only ${AVAIL_GB:-0}GB free on $ROOT; eight forecasts plus the pool need >=${DISK_FLOOR_GB}GB (40GB is the per-model transient that pod_run_model.sh enforces on its own, eight times over, plus retained output)" if [ "$FAIL" = "1" ]; then + echo + echo " Note: the nine publish variables must be EXPORTED into this shell, not just present" + echo " in a file — pipeline-core no longer auto-loads a .env (#346, C-177). Try:" + echo " set -a; . /root/.secrets; set +a" echo die "preflight failed — nothing above costs GPU time to fix. Fix them and re-run --preflight." fi