From b15b3bd4b6489c1b6939ec16036354c2264fdda0 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Wed, 30 Sep 2026 00:31:30 +0200 Subject: [PATCH 1/2] fix(#499): the FAO preflight's disk floor was below the floor it delegates to MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by a /falsify pass on "we are ready for a 40-lesson forecasting run reaching faoapi", and it is my defect from #524. pod_run_fao_delivery.sh demanded 60GB for EIGHT models plus the pooled ensemble, while pod_run_model.sh — which it delegates to eight times — refuses below 40GB for ONE model ("one model needs ~20GB"). So the orchestrator's preflight could report ready and the delegated script could then refuse partway through the roster, after hours of paid GPU time. That is precisely the failure --preflight exists to prevent, introduced into --preflight. Raised to DISK_FLOOR_GB=80: 40GB transient, which the delegated script enforces per model regardless of what is written here, plus eight models' retained output and the pool. The retained component is an ESTIMATE and the comment now says so rather than implying a measurement. One model's calibration output is ~2.5GB per predictions directory on this machine, but NO FORECASTING RUN HAS EVER COMPLETED ON THIS ROSTER — rusty_bucket's forecasting_log.txt is from 2026-07-20 and lists temporary_crane and temporary_fox at Deployment Status: shadow, a different roster entirely — so the retained size of a forecast is unmeasured. A forecast has one origin against calibration's thirteen so it should be smaller, and "should be" is doing real work in that sentence. Trigger for revising the number: the first completed run, rather than reasoning about it a second time. Guarded by a test that compares this floor against the delegated script's, so the two cannot drift apart again — the defect was not the number, it was that nothing related them. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GG2cY2HaqmUMpwUyR5V82K --- tests/test_fao_delivery_runner.py | 21 +++++++++++++++++++++ tools/podrun/pod_run_fao_delivery.sh | 22 ++++++++++++++++++++-- 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/tests/test_fao_delivery_runner.py b/tests/test_fao_delivery_runner.py index 389da0ad..3ef8fa5a 100644 --- a/tests/test_fao_delivery_runner.py +++ b/tests/test_fao_delivery_runner.py @@ -130,6 +130,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..5d431101 100755 --- a/tools/podrun/pod_run_fao_delivery.sh +++ b/tools/podrun/pod_run_fao_delivery.sh @@ -158,8 +158,26 @@ 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 From ad1ad2931df0f279206f7e74b54cc34f1e51f193 Mon Sep 17 00:00:00 2001 From: Polichinl Date: Wed, 30 Sep 2026 00:35:57 +0200 Subject: [PATCH 2/2] =?UTF-8?q?fix(#499):=20preflight=20checked=203=20of?= =?UTF-8?q?=209=20publish=20variables=20=E2=80=94=20build=20the=20config?= =?UTF-8?q?=20instead=20of=20counting?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by the /falsify pass on the forecasting claim, acting on a suggestion from the views-pipeline-core session. My preflight from #524 checked three environment variables and concluded the publish would work. Measured against the real code, PredictionStoreConfig requires NINE: APPWRITE_ENDPOINT, APPWRITE_DATASTORE_PROJECT_ID, APPWRITE_DATASTORE_API_KEY, APPWRITE_PROD_FORECASTS_BUCKET_ID, APPWRITE_PROD_FORECASTS_BUCKET_NAME, APPWRITE_PROD_FORECASTS_COLLECTION_ID, APPWRITE_PROD_FORECASTS_COLLECTION_NAME, APPWRITE_METADATA_DATABASE_ID, APPWRITE_METADATA_DATABASE_NAME Only the first three are secrets. A missing identifier fails the publish exactly as hard as a missing secret, so the check would have reported ready with six of nine absent — and the run would have failed at the publish, after the entire roster trained. The precise failure this mode exists to prevent, in the mode that exists to prevent it. Twice in one night now, which is its own signal. Adding the six missing names would not fix it. The extra can be absent, the endpoint unreachable, the key expired (#359: 2026-11-17). Preflight now CONSTRUCTS the config and imports the SDK, which answers the actual question instead of a proxy for it. views-pipeline-core suggested this and observed it is what #557 argues the manager should do itself; until it does, doing it by hand costs seconds. Second finding from the same measurement, and an operator trap: pipeline-core no longer auto-loads a .env from the working directory (#346, register C-177) — a library reading whatever .env its caller is standing in is what the Appwrite seam contract §3 forbids. So a correct /root/.secrets sitting beside the operator is NOT enough; the variables must be exported. Without that line the failure presents as wrong credentials rather than unexported ones. Preflight now prints `set -a; . /root/.secrets; set +a`. Both guarded, and the guards assert the construction and the exported-variables note exist rather than re-listing nine names — a guard that enumerates the same thing the code does is the defect that produced this commit. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GG2cY2HaqmUMpwUyR5V82K --- tests/test_fao_delivery_runner.py | 31 +++++++++++++++ tools/podrun/pod_run_fao_delivery.sh | 57 +++++++++++++++++++++++++--- 2 files changed, 83 insertions(+), 5 deletions(-) diff --git a/tests/test_fao_delivery_runner.py b/tests/test_fao_delivery_runner.py index 3ef8fa5a..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", diff --git a/tools/podrun/pod_run_fao_delivery.sh b/tools/podrun/pod_run_fao_delivery.sh index 5d431101..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 @@ -180,6 +223,10 @@ 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