Skip to content

fix: stop the fake-prediction scripts downloading a detector they discard - #26

Merged
MateoLostanlen merged 5 commits into
mainfrom
fix/engine-model-download
Sep 10, 2026
Merged

MateoLostanlen merged 5 commits into
mainfrom
fix/engine-model-download

Conversation

@MateoLostanlen

@MateoLostanlen MateoLostanlen commented Sep 8, 2026

Copy link
Copy Markdown
Member

Two independent failures that together destroyed twenty GPU-hours on 2026-09-07, plus the reason nothing survived them.

The crash

optimize_sequential.py, copy_failures.py and scan_raw_fp.py each construct a pyroengine Engine, then replace engine.model with _PassthroughModel on the very next line. They replay predictions predict_sequential.py already computed and never run a detector — but Engine.__init__ downloads one from HuggingFace anyway.

That download was dead weight from the start. It became fatal when the model repository stopped answering anonymously: pyronear/yolo11s_mighty-mongoose_v5.1.0, which uv.lock resolves, now returns HTTP 401. Stubbing Classifier before construction keeps the grid search hermetic, so the next model-repo rotation cannot break it again. Reuses the _Stub each file already defines for its API clients.

Why a three-second failure cost ten hours, twice

dvc push ran only after a full dvc repro. The runner is ephemeral, so train_yolo_best succeeding — ten hours of GPU — meant nothing once optimize_sequential_val failed three seconds later. The protection was inverted: the most expensive stage sat behind the most fragile ones, which cost minutes and depend on the network, external repositories and third-party packages.

The pipeline is now split, with a push between the two halves. Uploading the blob is not sufficient on its own — DVC storage is content-addressed, so the dvc.lock naming its hash is published to the result branch at the same point; otherwise the weights sit in S3 unreachable and the next run retrains them regardless.

Before training, the workflow looks for the corresponding result branch. It restores that branch's dvc.lock and pulls train_yolo_best with its dependencies only when the repository contents match, excluding dvc.lock and results/. This lets a fresh runner reuse the saved weights after a downstream failure. Different code or configuration starts a new training run; a failed checkpoint download stops the job before training.

Two smaller consequences, both from review: each result-branch commit is guarded against an empty index (train_yolo_best cached means dvc.lock is unchanged, and git commit treats that as an error), and a concurrency group serialises runs per train_* branch — two overlapping runs force-push the same result branch, so a slower older one could otherwise reset the pointer to its own model. It also releases the GPU instance a superseded run would hold for hours.

Validation

All three scripts build their engine under HF_HUB_OFFLINE=1 — so a warm cache could not mask a constructor download — keeping _PassthroughModel and carrying nb_consecutive_frames and conf_thresh through unchanged. pytest passes. ruff check reports the same 5 pre-existing errors before and after; the workflow YAML parses and the empty-commit guard was exercised on a scratch repository.

A local Git/DVC integration test exercises the actual workflow commands: first run without a checkpoint, retry after downstream failure with an empty cache, rejection after a code change outside DVC's declared dependencies, and failure when checkpoint storage is unavailable. Both pytest tests pass with DVC 3.67.0; Ruff check and format pass for the new test. No GPU or AWS run was launched.

Not fixed here

Upgrading to pyro-engine main — which moved to the standalone pyro_predictor package and a public model — is blocked upstream by huggingface_hub==0.23.1 against this repo's >=1.8.0. pyronear/pyro-engine#412 unpins it.

…card

Engine.__init__ builds a Classifier, which fetches a detector from
HuggingFace. None of these three scripts runs it: each swaps engine.model for
_PassthroughModel one line after construction, because they replay predictions
that predict_sequential.py already computed. The download was dead weight from
the start.

It turned fatal when the model repository stopped answering anonymously.
pyronear/yolo11s_mighty-mongoose_v5.1.0, which uv.lock resolves, now returns
HTTP 401, as does nimble-narwhal_v6.0.0 on pyro-engine's current path. Two
GPU runs died on it after ten hours each, at a stage lasting three seconds,
and dvc push runs after dvc repro, so neither trained model survived.

Stubbing Classifier keeps the grid search hermetic, so the next model-repo
rotation cannot break it again. Reuses the _Stub already defined at the top of
each file for the API clients, and targets pyroengine.engine.Classifier, which
is where the locked version resolves it from.

Verified with HF_HUB_OFFLINE=1 so a warm cache could not mask a constructor
download: all three build, keep _PassthroughModel, and carry nb_consecutive_frames
and conf_thresh through unchanged.
The runner is ephemeral: whatever is not in the DVC remote when the job ends
is gone. dvc push ran only after a full dvc repro, so any stage failing after
training took the weights with it.

That is not hypothetical. On 2026-09-07 two runs reached train_yolo_best and
completed it — ten hours of GPU each — then died in optimize_sequential_val,
a stage lasting three seconds that only grid-searches parameters over
predictions computed earlier. Twenty GPU-hours, nothing kept.

The protection was inverted: the most expensive stage sat behind the most
fragile ones. Everything after training costs minutes and depends on the
network, external repositories and third-party packages; training itself needs
only the GPU and data already on disk.

Splitting the repro puts a push between the two, so the model reaches the
remote before anything fragile runs. Weights survive a failure in grid search,
export or evaluation, and the run can be resumed from them instead of retrained.
… stages

Uploading the blob was not enough. DVC storage is content-addressed, so
without the dvc.lock naming its hash nobody can find it again — and that lock
lived only on the ephemeral runner until the result branch was pushed, which
happened after the stages that fail. The weights would have sat in S3,
unreachable, and the next run would have retrained them anyway.

Publishing the pointer next to the push is what makes the earlier split
actually recover anything.

Both result-branch checkouts become `-B`, since the branch now exists by the
time the second one runs, and the second commit is guarded: it sees only what
the remaining stages added to dvc.lock, which is nothing when they were all
cached, and git treats an empty diff as an error.
…ther

Two holes in the previous commit.

The model-pointer commit had the same empty-index failure the results commit
was already guarded against: when train_yolo_best is cached and dvc.lock
already records the hash, git add stages nothing and git commit exits 1,
killing the job before it reaches any of the work it was protecting.

And two runs on one train_* branch force-push the same result branch, so a
slower older run can land after a newer one and reset the pointer to its own
model. If the newer run then fails, the weights it uploaded are unreachable —
exactly the failure this branch set out to prevent. Superseding the older run
also releases the GPU instance it would otherwise hold for hours.
@MateoLostanlen
MateoLostanlen merged commit 862fbba into main Sep 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant