Skip to content

sp run --in-allocation: run a campaign inside one SLURM allocation - #964

Open
cailmdaley wants to merge 8 commits into
developfrom
feat/sp-in-allocation
Open

cailmdaley wants to merge 8 commits into
developfrom
feat/sp-in-allocation

Conversation

@cailmdaley

@cailmdaley cailmdaley commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Summary

sp run --in-allocation runs a whole campaign inside the SLURM allocation it is called from, under snakemake's local executor, so a campaign waits in the queue once instead of once per job.

Flag / behaviour What it does
--in-allocation --executor local --cores N --resources mem_mb=M, with N from the cpuset and M from the tightest cgroup memory limit less a reserve; later explicit flags win. Refuses to run outside an allocation.
default tile store $SLURM_TMPDIR/tile-store/<run>, so two campaigns can share one allocation (one srun step each); tile_store_root: still wins
resubmit after a timeout deletes tile_vignets.json for tiles with no final_cat (their node-local store died with the job) and releases a snakemake lock whose job has left the queue; any other lock stops the run. -n only reports.

workflow/README.md gains an "Execution mode: one allocation" section covering sizing (one node, ≤4 GB/core, the widest rule's last retry must fit) and the resume path.

Why now

Every v2 production batch on Nibi already runs through this branch: the integration branches g15 through g18 carry it, and g18 alone ran 33 one-node batches (~800 tiles, P3) with no run failures. It is the base of #954 (exposure stores node-local under --in-allocation, #953). This PR lands the mechanism production depends on, so that it stops being campaign-local.

Scope

  • In: workflow/bin/sp, workflow/README.md.
  • Out: the per-job SLURM executor path is unchanged; --in-allocation is opt-in.

Verification

  • Production: every P3 batch on Nibi ran through sp run --in-allocation on one 144-core node; g18's 33 batches (~800 tiles) had no run failures (surveyor's report, 11 Oct).
  • Not checked for this PR: whether a production batch has yet resumed through the timeout sweep. -n reports the sweep and lock release without acting.

Stacked after this: #954 → base moves to develop once this merges.


Claude (Opus 5.5) on behalf of Cail, from the Nibi scale-up.

🤖 Generated with Claude Code

https://claude.ai/code/session_01SYQ7H2aF2jXdXv9FSKyagY

cailmdaley and others added 4 commits October 2, 2026 19:41
Runs both invocations under snakemake's local executor inside the
allocation sp is called from, so a small campaign pays one queue wait
instead of one per job. sp derives --cores from the cpuset and
--resources mem_mb from the tightest cgroup memory limit (less a
reserve), refuses to run outside an allocation, and defaults the
node-local tile store to $SLURM_TMPDIR/tile-store through the existing
tile_store_root rebind. README documents sizing and the resume caveat.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dj4HnkNqqPkVCeDeUGEGuz
… per allocation

Data campaigns name node-local stores by bare tile id, so two campaigns
over the same tiles sharing one allocation would share stores. Default
to $SLURM_TMPDIR/tile-store/<run>, and document one srun step per
campaign.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Dj4HnkNqqPkVCeDeUGEGuz
A job cut short by its time limit leaves two things a plain resubmission
trips over. Its node-local tile store is gone, so a tile whose
tile_vignets.json survived without a final_cat stops at tile.smk's
"vignette store is missing" guard; and a killed snakemake leaves its lock.
sp now deletes those manifests before the run, and records the job that
holds the state dir so that a lock whose job has left the queue is
released. Any other lock stops the run before anything is swept. With -n
both only report.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01D8wdTpGpBtyK6uHwU7qWQW
sm() exported SP_RUN_CONFIG as the snapshot's copy into the caller's shell, so
on a resumed 'sp run' the snapshot refresh copied run_config.yaml onto itself
and cp's non-zero exit failed the run in seconds (smk-b122, smk-b124 after
timeouts).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XekXwUoqNTMvZiEEsCtwZx
(cherry picked from commit ab4c293)
@cailmdaley

Copy link
Copy Markdown
Contributor Author

Pushed 313ccd7 (cherry-picked from integration/smk-g19 ab4c293): sm() now scopes its SP_RUN_CONFIG=<snapshot>/run_config.yaml export to a subshell around the snakemake call. Before, the export leaked into sp's own shell, so on a resumed run ("resubmit as-is after a timeout") the snapshot refresh copied run_config.yaml onto itself, cp exited non-zero, and the run died within 30 s. Production hit this on smk-b122/b124; both are rerunning on the fix.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FD35GJ3MDN7ceDhFwxzMbU

The in-allocation sweep (5ee28fd, #964) removes tile_vignets.json when a
tile has no final catalogue. If ngmix and merge_cats already finished,
make_cat had no dependency on that manifest and ran alone against a lost
node-local store (smk-b81, tile 291.293, job 24195960).

Declare the vignets manifest as a make_cat input. A plain edge also queues
all completed ngmix chunks: Snakemake 9.23.1 propagates a missing producer
output even with ancient(). Instead, cut that edge for completed chunks
while the vignets manifest is missing. Require either the chunk directory
or a completed merge, so a lost pre-merge chunk still waits for the writer.
Finished tiles also cut the edge: otherwise the restored manifest's newer
mtime would wake preserved chunks on the next invocation, when the sweep
skips the now-finished tile. Unfinished tiles with an existing vignets
manifest retain normal timestamp and structural invalidation.

Trade-off: storage-only recovery and finished tiles trust existing chunk
completion manifests instead of rerunning expensive science to restore a
transient store. This uses the same disabled input rerun trigger and
finished-product assumption as the exposure-edge cut. To explicitly
recompute completed chunks after a sweep, delete their manifests too; to
rebuild a finished tile, delete its final catalogue first. No shell body
or params.pre changes, so the campaign-boundary pins stay unchanged.

Add planning regressions for merged outputs (including reclaimed temp
chunks), unmerged completed chunks, one unfinished chunk, and a lost chunk
directory, in data and image-sims modes. Assert writer-before-reader
edges, preserved completed chunks, profile trigger policy, stability on
the invocation after recovery, and ordinary upstream invalidation.
@cailmdaley

cailmdaley commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor Author

Corrected after review: vignette dependencies are now unconditional.
Fix-forward commit 7c3fcdd7 on feat/sp-in-allocation supersedes the unsafe conditional recovery in ca224f7e; its cherry-pick on integration/smk-g19 is 61b6255f, superseding b9eb4fff.

The original failure was a missing tile_make_cat dependency: sweeping tile_vignets.json could leave only make_cat scheduled against a lost node-local store.
My earlier attempt to preserve completed chunks was unsound: a completion manifest does not prove that a chunk will not run this invocation.
The review reproduced this with a swept vignets manifest and newer tile_detect.json: all eight chunks were scheduled, but their writer dependencies had been removed, allowing them to start before the store existed or race its rebuild.

The fix removes ngmix_vignets entirely.
Every tile_ngmix chunk and tile_make_cat declares the vignets manifest unconditionally.
When the sweep removes that manifest, the planned tile-shape jobs are now:

Scenario tile_vignets tile_ngmix tile_merge_cats tile_make_cat
Completed chunks and merge, vignets swept 1 8 1 1
Same, with newer tile_detect.json 1 8 1 1

The accepted cost is rerunning completed ngmix chunks on swept tiles.
Every scheduled reader waits for the writer; completed-output inference no longer removes any vignette edge.
Shell bodies and params.pre are unchanged, and their pins pass unchanged.

tests/workflow/test_resume.py covers both the original case and the review counterexample in data and image-simulation modes, including retained/reclaimed merged chunks, pending merges, partial chunks, and missing chunk directories.
It asserts the writer dependency for every chunk and for make_cat.
The new merged-state regressions fail against ca224f7e in both modes.

Verification used the requested shapepipe-develop-240b37e4.sif and supplied helpers, inside a 4-core/12-GiB allocation (job 24284506):

  • feat/sp-in-allocation: workflow 37 passed; unit 566 passed, 1 skipped.
  • integration/smk-g19: workflow 48 passed; unit 613 passed, 1 skipped.

The production holder must fast-forward /project/6001537/cdaley/shapepipe-g19 to origin/integration/smk-g19, then launch/resume through sp so its launch snapshot contains the correction.
I used my separate worktrees; the production and onealloc checkouts were not modified.

— Sol on behalf of Cail.

cailmdaley added a commit that referenced this pull request Oct 17, 2026
The in-allocation sweep (5ee28fd, #964) removes tile_vignets.json when a
tile has no final catalogue. If ngmix and merge_cats already finished,
make_cat had no dependency on that manifest and ran alone against a lost
node-local store (smk-b81, tile 291.293, job 24195960).

Declare the vignets manifest as a make_cat input. A plain edge also queues
all completed ngmix chunks: Snakemake 9.23.1 propagates a missing producer
output even with ancient(). Instead, cut that edge for completed chunks
while the vignets manifest is missing. Require either the chunk directory
or a completed merge, so a lost pre-merge chunk still waits for the writer.
Finished tiles also cut the edge: otherwise the restored manifest's newer
mtime would wake preserved chunks on the next invocation, when the sweep
skips the now-finished tile. Unfinished tiles with an existing vignets
manifest retain normal timestamp and structural invalidation.

Trade-off: storage-only recovery and finished tiles trust existing chunk
completion manifests instead of rerunning expensive science to restore a
transient store. This uses the same disabled input rerun trigger and
finished-product assumption as the exposure-edge cut. To explicitly
recompute completed chunks after a sweep, delete their manifests too; to
rebuild a finished tile, delete its final catalogue first. No shell body
or params.pre changes, so the campaign-boundary pins stay unchanged.

Add planning regressions for merged outputs (including reclaimed temp
chunks), unmerged completed chunks, one unfinished chunk, and a lost chunk
directory, in data and image-sims modes. Assert writer-before-reader
edges, preserved completed chunks, profile trigger policy, stability on
the invocation after recovery, and ordinary upstream invalidation.

(cherry picked from commit ca224f7)
Fix forward the unsafe conditional edge in ca224f7. A completion manifest does not mean a chunk will not run: after the vignets manifest is swept, a newer tile_detect input schedules all ngmix chunks, but the conditional removed their dependency on the writer. They could start before the store exists or race its rebuild.

Remove ngmix_vignets and restore every chunk unconditional vignets input. Keep the unconditional make_cat input. The accepted trade-off is that sweeping a tile now rebuilds its store and reruns all ngmix chunks and their merge, even if earlier outputs survived. Correct ordering is worth the extra ngmix time on these resumed tiles; no completed-output inference may remove a reader edge.

Replace the reuse-oriented tests and documentation. Test the original swept-store scenario and the review counterexample with newer tile_detect, in data and image-sims modes, with completed, reclaimed, partial and missing chunk products. Assert every scheduled chunk and make_cat depends directly on tile_vignets. The new merged-state regressions fail on ca224f7. Shell bodies and params.pre are unchanged.
Resume housekeeping invalidates scratch manifests, not durable map products. A map-specific sweep would protect only --in-allocation and could reopen exp_persist and a costly PSF fit. Document the boundary here; the functional change on feat/pointing-maps declares fragments and recovers them from staged raw MEFs after store reclamation.
@cailmdaley

cailmdaley commented Oct 17, 2026 •

Copy link
Copy Markdown
Contributor Author

Commit 26be430 corrects the resume documentation to match the review's lifecycle decision.
No map-specific resume sweep is added.

The functional fix is 313829f on #966: live stores produce declared fragments before reclamation; reclaimed fragments have no reachable rasterization producer.
Missing reclaimed fragments fail planning with an explicit backfill command and must be rebuilt deliberately outside the DAG.
The automatic raw-MEF fallback has been removed because code/params changes could otherwise delete finished fragments after their sources were removed.

bash -n workflow/bin/sp passes.
On #966, SIF allocation tests pass: 38 workflow tests and 8 map unit tests.
The six new immutable-fragment regression cases fail against the unsafe parent and pass with the fix.
The fix and this documentation are cherry-picked and pushed to integration/smk-g19 as b4d1f76 and 661bcdf.
Its SIF allocation tests pass: 63 workflow tests and 8 map unit tests.
Production remains untouched.

— Codex on behalf of Cail.

Correct the recovery boundary: reclaimed fragments must have no producer, not an automatic raw-MEF fallback. Code and params edits must never cause Snakemake to remove products whose sources have already gone.

This branch has not been deployed

No deployments
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