From fb56e9aad9102fa0e5afb467141f1d41f4072104 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:42:38 +0200 Subject: [PATCH 01/43] docs(workflow): present-tense comments in catalogue merges Co-Authored-By: GPT-6.1 Sol --- workflow/scripts/merge_final_cat.py | 91 +++++--------- workflow/scripts/merge_star_cat.py | 189 +++++++++------------------- 2 files changed, 90 insertions(+), 190 deletions(-) diff --git a/workflow/scripts/merge_final_cat.py b/workflow/scripts/merge_final_cat.py index e4a0b4d7c..14d804d29 100644 --- a/workflow/scripts/merge_final_cat.py +++ b/workflow/scripts/merge_final_cat.py @@ -1,74 +1,40 @@ #!/usr/bin/env python3 -"""Collect the campaign's per-tile final catalogues into ONE hdf5 file. +"""Collect the campaign's per-tile final catalogues into one HDF5 file. Run as the shell of the campaign-level ``final_cat_merge`` rule, never by hand. -WHAT IT PRODUCES, AND FOR WHOM. ``/final_cat_.hdf5``: +``/final_cat_.hdf5`` holds one dataset per tile, carrying the columns named by the input type's ``final_cat.param`` (``workflow/config/cfis/`` for data, ``workflow/config/cfis_image_sims/`` for image sims), plus an ``n_tiles`` attribute on the file root. sp_validation opens that file as its ``galaxy_cat_path`` -(``sp_validation/catalog.py``), so its SCHEMA is an interface and not a choice — -see ``SPVAL_GROUP`` below for the one legacy literal in it. +(``sp_validation/catalog.py``); see ``spval_group`` for the group-path interface. (sp_validation's own ``merge_catalogues`` is a different layer entirely: it works over already-calibrated ``shape_catalog_comprehensive_*.fits``. It does not do this merge, and this does not do that one.) -WHAT IT REUSES, AND WHAT IT DOES NOT. The column extraction is -``create_final_cat.py``'s — ``read_param_file`` for the parameter list, -``read_data`` and ``copy_data`` for pulling those columns out of one catalogue -with their stored dtypes — so the column grammar keeps exactly one definition. -Those three are REPRODUCIBLE FUNCTIONS, and this PR is what made them so: the -parameter list comes back ordered rather than through a set, ``copy_data`` -allocates the requested columns alone rather than leaving every other column of -the source as uninitialised memory, and a missing column raises with its own -name instead of falling out of a bare ``except:`` as an UnboundLocalError. The -fixes are upstream, in that script, because a hand-run of it deserves them as -much as this rule does. -Its ``process()`` is NOT used and neither is any of its discovery: that function -walks a directory tree the workflow does not have and never will, and it groups -by a unit ShapePipe v2 no longer has. This script walks the workflow's own -products tree instead (``tiles/<2-char prefix>//final_cat-.hdf5``, -one dataset per column) and writes the merged hdf5 itself. - -WHERE ``create_final_cat.py`` IS FOUND. Beside this workflow, at -``/scripts/python/create_final_cat.py`` — resolved relative to THIS file, -so it follows the launch code snapshot (``bin/sp``) exactly as -``workflow/scripts/*`` does, and a campaign never reads a mid-run edit. It is -loaded by path rather than imported: it is a script, not an installed module, -and the container's ``shapepipe`` install does not carry it. - -IT RECONCILES, IT NEITHER REBUILDS NOR BLINDLY APPENDS, and the machinery for -that is ``hdf5_reconcile.py``, shared with the star side so the campaign's two -products cannot disagree about what an output owes its inputs. That module -carries the argument in full: an append reads the appended tiles, a source that -changed is re-read, a tile that left the campaign is deleted, a column-set -change refreshes everything, and a no-op leaves the file untouched. -``create_final_cat.py``'s own ``process()`` implements only the append-only half -— it skips a tile already in the file, whatever the file on disk now says — -which is right for a hand-driven update and wrong for a DAG output. (Its ``-s`` -single-ID mode implements ``check`` and ``remove``; ``add`` is accepted by the -argument validator and then falls through to the ordinary walk, so it is not a -way to add one tile by hand.) - -WHICH TILES — AND WHY THE JOB DERIVES THE SET RATHER THAN BEING TOLD IT. The set -is the CAMPAIGN's: every tile both declared in ``tile_list`` and present in the -index, which is exactly the Snakefile's TILES_READY, rebuilt here from the same -two files the Snakefile started from (``--tile-list`` and ``--index-db``, read -through ``build_index.campaign_tiles`` so there is one definition and not two -that can drift). It is derived rather than passed because at DR6 scale the set -is ~20k paths and a shell command reaches ``execve`` as a SINGLE argv entry -capped at 128 KiB by ``MAX_ARG_STRLEN``; the rule's ``input`` is the DAG edge -and its ``params`` carries a fingerprint of that same list, which is the rerun -trigger. - -THE TWO SETS ARE THE SAME SET, which is the point of deriving it this way rather -than globbing ``/tiles``: a products root shared with an earlier, -larger tile list would hand the job tiles the fingerprint never saw and no rerun -trigger would notice. A tile in the derived set whose catalogue is missing is a -hard error here, not a skip — under the DAG it cannot happen, since every one of -them is a declared input of this job. +Column extraction uses ``scripts/python/create_final_cat.py``'s +``read_param_file``, ``read_data`` and ``copy_data``; those functions define +column selection and ordering. Discovery uses the workflow's products tree: +``tiles/<2-char prefix>//final_cat-.hdf5``, one dataset per column. + +``create_final_cat.py`` is loaded by path relative to this file, so it follows +``bin/sp``'s launch code snapshot. It is a script, not an installed module. + +Reconciliation uses ``hdf5_reconcile.py``, shared with the star-catalogue merge; +see that module for source tracking and update semantics. This script does not +use ``create_final_cat.py``'s append-only ``process()``. + +``build_index.campaign_tiles`` derives membership from ``--tile-list`` and +``--index-db`` rather than passing ~20k paths in a shell argument (limited to +128 KiB by ``MAX_ARG_STRLEN``). See the Snakefile's ``final_cat_merge`` rule +for dependencies and the membership fingerprint. + +@sc [label:coupling] final-merge-campaign-membership +Merge only tiles in the tile-list/index intersection, not a glob of the shared +products root: unrelated tiles would evade the rule's membership fingerprint. +A selected tile with no catalogue is a hard error, not a skip. @sc [decision:catalogue_assembly.failure_sentinels,label:selection] never-fit-rows-pass-through Every row of every tile catalogue reaches the merged file, unchanged, @@ -96,11 +62,10 @@ def spval_group(campaign: str) -> str: """The hdf5 group the campaign's per-tile datasets live under. - ``patches/`` is a LEGACY KEY IN sp_validation's FILE SCHEMA, kept verbatim - only so its reader works unchanged (CosmoStat/sp_validation#340 tracks - removing it); it names nothing in this workflow, which has campaigns and - tiles and no other unit. This is the one place the literal appears — - everything else here says campaign. + @sc [label:schema] final-merge-spval-group-path + Keep ``patches/`` as the group path required by sp_validation's + reader. CosmoStat/sp_validation#340 tracks the reader migration; ``patches`` + is a file-schema key, not a workflow unit. """ return f"patches/{campaign}" diff --git a/workflow/scripts/merge_star_cat.py b/workflow/scripts/merge_star_cat.py index 371119546..729b60c1c 100644 --- a/workflow/scripts/merge_star_cat.py +++ b/workflow/scripts/merge_star_cat.py @@ -1,74 +1,45 @@ #!/usr/bin/env python3 -"""Collect the campaign's per-CCD PSF validation catalogues into ONE hdf5 file. +"""Collect the campaign's per-CCD PSF validation catalogues into one HDF5 file. Run as the shell of the campaign-level ``star_cat_merge`` rule, never by hand. -WHAT IT PRODUCES, AND FOR WHOM. ``/full_starcat_.hdf5``: +``/full_starcat_.hdf5``: one dataset per exposure at ``exposures/``, holding that exposure's every CCD's ``validation_psf--.fits`` rows stacked, with a ``CCD_NB`` column recording which CCD each row came from. It is the input to the rho/tau -statistics. Historically this was ``combine_runs.bash psf`` plus a -``merge_starcat_runner`` pass producing one flat FITS table, -``full_starcat-0000000.fits``, and sp_validation still opens that name today; -its readers move to this hdf5 under CosmoStat/sp_validation#340, the same -migration that retires the ``patches/`` key on the galaxy side. - -WHY HDF5, AND WHY ONE DATASET PER EXPOSURE. The campaign's two products should -behave the same way, and one flat table cannot: appending a tile meant -restacking every exposure the campaign had ever seen — ~40 GB of members at DR6 -scale to add ~2 MB. Per-exposure datasets make the file RECONCILABLE -(hdf5_reconcile.py carries that argument, and merge_final_cat.py is the same -machinery on the tile side), so an append reads the appended exposures and -nothing else while the file still cannot drift from its inputs. Memory follows: -one exposure at a time, not one campaign. - -NATIVE DTYPES. Columns are written as the validation_psf files store them — -float32 stays float32. The FITS writer this replaces widened every float column -to ``1D``, doubling both the file and the peak memory of the job that wrote it, -for no information. - -CCD_NB IS AN INTEGER. It is parsed out of the member name -(``validation_psf--.fits``), where it is always digits, so a string -buys nothing — and an int column costs 4 bytes a row against the 8 a -two-character fixed-width string does. - -IT READS THE TARS, IT DOES NOT UNPACK THEM. ``exp_persist`` packs each -exposure's keepers into one uncompressed tar on the persistent root -(``/exp///psf/.tar``) precisely because inodes, -not bytes, bind on /project. Unpacking ~20k tars x ~40 members to merge them -would materialise ~800k files on the filesystem that design exists to protect. -Members are read through the archive's own file object — seekable, the tar -being uncompressed by design — so the counting pass costs a header rather than -a member. - -THE OPTIONAL COLUMNS ARE A PER-FILE QUESTION. A pix2wcs-converted catalogue has -no MAG/SNR/ACCEPTED where an ordinary one does, and a campaign can hold both. -Deciding once for the merge is wrong in both directions: it either fails on the -first converted file or silently zeroes the real values of every ordinary one. -Each file is asked for its own schema, and only the files that lack a column are -zero-filled. - -WHICH EXPOSURES — AND WHY THE JOB DERIVES THE SET RATHER THAN BEING TOLD IT. -The set is the CAMPAIGN's: every exposure read by a tile that is both declared -in ``tile_list`` and present in the index, which is the Snakefile's TILES_READY -walked one edge further. This script rebuilds it from the same two files the -Snakefile started from (``--tile-list`` and ``--index-db``, both small, both on -the persistent root, both read through ``build_index.campaign_exposures`` so -there is one query and not two that can drift), and then takes the exposures -whose ``exp_persist`` manifest is on the persistent root. - -It is derived rather than passed because at DR6 scale the set is ~20k paths and -a shell command reaches ``execve`` as a SINGLE argv entry capped at 128 KiB by -``MAX_ARG_STRLEN``. So the rule's ``input`` is the DAG EDGE — what must exist -before this runs — and its ``params`` carries a FINGERPRINT of the same set, -which is what makes the merge rerun when the set changes. A glob over -``/exp`` would NOT be the same set: it would sweep in exposures of -an earlier, larger tile list sharing the products root, stacking rows the -fingerprint never saw and no rerun trigger would notice. - -THE MANIFEST, NOT THE TAR, IS WHAT IT READS FIRST: the manifest records what was -actually packed, member by member, with sizes, sha256 digests and the product -each came from, so this script never guesses at tar contents. +statistics. sp_validation reads it once CosmoStat/sp_validation#340 lands; +until then it opens the flat ``full_starcat-0000000.fits``. + +Per-exposure datasets bound memory to one exposure and allow incremental +updates rather than restacking ~40 GB of members at DR6 scale to add ~2 MB. +See ``hdf5_reconcile.py`` for reconciliation semantics, shared with +``merge_final_cat.py``. + +Required columns retain their stored dtypes; optional columns use ``OPTIONAL``'s +fixed dtypes. ``CCD_NB`` is an int32 parsed from the member name +(``validation_psf--.fits``). + +Members are read directly from the persistent tar, without unpacking ~20k +archives x ~40 members into ~800k files. See ``persist_exp.py`` for archive +layout and retention policy. The counting pass reads FITS headers only. + +@sc [label:schema] star-merge-optional-column-fill +Check MAG/SNR/ACCEPTED availability per member: a campaign can mix ordinary +and pix2wcs-converted catalogues. Zero-fill only members lacking the column, +so converted files remain readable without discarding ordinary files' values. + +``build_index.campaign_exposures`` derives exposures from ``--tile-list`` and +``--index-db`` rather than passing ~20k paths in a shell argument (limited to +128 KiB by ``MAX_ARG_STRLEN``). See the Snakefile's ``star_cat_merge`` rule +for dependencies and the membership fingerprint. + +@sc [label:coupling] star-merge-campaign-membership +Consider only exposures used by the tile-list/index intersection, then select +those with persistent manifests and matching validation members. Do not glob +the shared products root: unrelated exposures would evade the rule's fingerprint. + +Manifests select archives; ``read_exposure`` selects matching member names from +each archive. Member digests are not verified here. """ import argparse @@ -86,39 +57,28 @@ import hdf5_reconcile import persist_exp -# The members this merge consumes, named as the keep list names them and -# resolved through the same catalogue persist_exp packs by — so the glob has one -# definition and adding a product cannot leave the two disagreeing. They are -# always there to find: persist_exp packs this product for every exposure -# whatever `persist_exp:` says, and fails the pack rather than writing a -# manifest without it. +# Use persist_exp's mandatory validation product and its pattern; see +# persist_exp.py for the packing requirement. MEMBER_PRODUCT = persist_exp.ALWAYS MEMBER_PATTERN = persist_exp.resolve(MEMBER_PRODUCT) -# The group holding the per-exposure datasets. Unlike the galaxy side's -# `patches/`, this name is ours and says what it holds. +# Group holding the per-exposure datasets. GROUP = "exposures" -# The validation_psf table's HDU: what MergeStarCatPSFEX defaulted to and what -# psfex_interp writes — a SExtractor-style file, empty primary, header-carrying -# image extension, then the table. +# psfex_interp writes a SExtractor-style file: empty primary, header-carrying +# image extension, then the validation_psf table. HDU = 2 -# The columns, in the order the FITS full_starcat carried them, which is the -# order every consumer has seen. The optional three are zero-filled per file. +# Required columns in consumer order; optional columns follow below. COLUMNS = ("X", "Y", "RA", "DEC", "HSM_G1_PSF", "HSM_G2_PSF", "HSM_T_PSF", "HSM_M4_1_PSF", "HSM_M4_2_PSF", "HSM_RHO4_PSF", "HSM_G1_STAR", "HSM_G2_STAR", "HSM_T_STAR", "HSM_M4_1_STAR", "HSM_M4_2_STAR", "HSM_RHO4_STAR", "HSM_FLAG_PSF", "HSM_FLAG_STAR") -# CANONICAL DTYPES, not whatever the first file that carries the column happens -# to use. These three are absent from pix2wcs-converted catalogues, so an -# exposure whose files all lack them would otherwise be allocated a fallback -# dtype while its neighbours got the real one — and datasets under exposures/* -# would then differ in dtype, which np.concatenate refuses and no digest can -# repair, since nothing about the schema CHANGED. Pinning the dtype here is what -# makes every exposure's dataset the same shape whatever its files carry. +# @sc [label:schema] star-merge-optional-dtypes +# Pin optional dtypes even when every member lacks these columns; otherwise +# exposure datasets can have incompatible structured dtypes at concatenation. OPTIONAL = {"MAG": np.float32, "SNR": np.float32, "ACCEPTED": np.int32} CCD_COLUMN = "CCD_NB" ALL_COLUMNS = COLUMNS + tuple(OPTIONAL) + (CCD_COLUMN,) @@ -141,14 +101,9 @@ def ccd_number(member_name: str) -> int: def is_member(entry: dict) -> bool: """Is this manifest entry one of the members this merge reads? - BY PRODUCT NAME, OR FAILING THAT BY FILE NAME. persist_exp records the - product every member came from and always packs psf_validation, so the name - is the answer for anything it writes today. The glob is the fallback, and it - earns its place twice over: a tar packed before the product field existed - has no label at all, and a keep list written as a raw glob - (`validation_psf-*.fits` rather than `psf_validation`) labels its members - with the glob. Neither should make the campaign's star catalogue silently - empty. + Accept the product label or matching filename. Filename matching supports + manifests with no product field and keep lists expressed as raw globs. + This check diagnoses labels; ``tars`` uses filenames for merge membership. """ return (entry.get("product") == MEMBER_PRODUCT or fnmatch(entry["name"], MEMBER_PATTERN)) @@ -172,26 +127,17 @@ def manifests(products_dir: Path, tile_list: Path, index_db: Path) -> list: def tars(manifest_paths: list) -> tuple: """``[(exposure, tar path)]`` for the merge, and the exposures with nothing. - Every tar is checked for existence HERE, so a products root missing a file - fails before a single row is read rather than an hour in. The tar is also - the unit's SOURCE for reconciling: its size and mtime are what a later - invocation compares against to decide whether this exposure changed. + Check each selected tar's existence before reading rows. The tar is the + reconciliation source; see ``hdf5_reconcile.py`` for source stamps. """ chosen, empty = [], [] for exp, man_path in manifest_paths: man = json.loads(man_path.read_text()) - # MEMBERSHIP IS THE MEMBER NAME, and only the member name. is_member() - # will also accept a manifest's own product LABEL, which is the right - # test for "did this exposure keep the product" — but a label is not - # what read_exposure() selects on, and a mislabeled entry whose name - # does not match would put this exposure in the merge and then abort - # the whole campaign when the tar turned out to hold nothing selectable. - # So the two agree by construction: both ask the name. + # Select by filename, as read_exposure does; a product label alone + # does not guarantee a selectable member in the tar. if not any(fnmatch(f["name"], MEMBER_PATTERN) for f in man["files"]): if any(is_member(f) for f in man["files"]): - # Labelled as the product, named as something else. Worth one - # line — it means a manifest we did not write, or a keep list - # whose glob does not match the member it matched. + # Report a product label inconsistent with the member name. print(f"[merge_star_cat] {exp}: manifest labels a " f"{MEMBER_PRODUCT} member whose name does not match " f"{MEMBER_PATTERN}; not merging it") @@ -208,18 +154,10 @@ def tars(manifest_paths: list) -> tuple: def read_exposure(exp: str, tar_path: Path) -> np.ndarray: """One exposure's every CCD, stacked, as a structured array. - TWO PASSES over the tar's members, and neither holds the exposure twice: - the first reads only each member's FITS HEADER — NAXIS2, the row count — - and the second allocates the columns once at their exact final length and - fills them slice by slice. Members are visited in sorted name order, so the - row order is a function of the tar's contents alone. - - NOTE ON WHEN THIS IS CALLED AGAIN. The unit's source is the TAR, so adding a - retention product re-packs it, moves its mtime, and refreshes this exposure - even though its validation members are byte-for-byte what they were. Reading - one exposure is seconds and the alternative — stamping the members rather - than the archive — buys a rarely-taken shortcut for a per-member bookkeeping - cost on every exposure. Not worth it. + Count rows from FITS headers, allocate once, then fill member slices in + sorted name order. Only one exposure is held in memory. Repacking any + retention product changes the archive source stamp and can refresh this + exposure even when its validation members are unchanged. """ try: tf = tarfile.open(tar_path) @@ -241,19 +179,16 @@ def read_exposure(exp: str, tar_path: Path) -> np.ndarray: ignore_missing_simple=True) as hdul: hdu = hdul[HDU] counts.append(hdu.header["NAXIS2"]) - # ColDefs.dtype describes the table without reading it. NOTE: - # it is the RAW storage dtype and ignores TSCAL/TZERO, so a - # scaled column would be allocated narrower than the values - # .data returns. Latent, not live: no validation_psf column is - # scaled. Read the dtype off .data if one ever is. + # @sc [label:schema] star-merge-unscaled-required-columns + # Required validation_psf columns must be unscaled: + # ColDefs.dtype ignores TSCAL/TZERO. Scaled columns require + # dtypes from .data to avoid narrowing their decoded values. if dtypes is None: dtypes = hdu.columns.dtype n_total += counts[-1] fields = [(c, dtypes[c]) for c in COLUMNS] - # The optional three take their CANONICAL dtype, not one file's (see - # OPTIONAL): every exposure's dataset must have the same dtype whether - # or not its files carry the column. + # See OPTIONAL for the cross-exposure dtype contract. fields += list(OPTIONAL.items()) fields += [(CCD_COLUMN, np.int32)] data = np.empty(n_total, dtype=np.dtype(fields)) @@ -269,7 +204,7 @@ def read_exposure(exp: str, tar_path: Path) -> np.ndarray: for col in COLUMNS: data[col][sl] = rows[col] for col in OPTIONAL: - # THIS file's schema, not the exposure's. + # See the module's per-member optional-column contract. data[col][sl] = rows[col] if col in have else 0 data[CCD_COLUMN][sl] = ccd_number(name) at += n_rows From d69823f20099072cec8a8dae426b19e57041f05a Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:43:34 +0200 Subject: [PATCH 02/43] docs(pipeline): present-tense comments in sqlite_store.py Co-Authored-By: GPT-6.1 Sol --- src/shapepipe/pipeline/sqlite_store.py | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/shapepipe/pipeline/sqlite_store.py b/src/shapepipe/pipeline/sqlite_store.py index bc1240c9a..0d8fdf361 100644 --- a/src/shapepipe/pipeline/sqlite_store.py +++ b/src/shapepipe/pipeline/sqlite_store.py @@ -11,13 +11,13 @@ file with sqlite's ``immutable=1`` URI parameter instead, which skips all locking and change detection. -``immutable=1`` is only correct for a file that no process writes while it -is open: sqlite neither sees concurrent changes nor replays a hot journal -left by a crashed writer, so it would return uncommitted rows. Every call -site of this module reads a store that an earlier module wrote and closed -(``make_post_process`` reads the header log ``merge_headers`` wrote). As a -guard, opening a store that has a rollback journal or write-ahead log next -to it raises instead of reading it. +@sc [label:custody] immutable-sqlite-store +The writer must close the store before a reader opens it, and no process may +write it while an immutable reader is open. SQLite skips change detection +and journal recovery in this mode, so concurrent writes or an unrecovered +journal can yield invalid data. Opening rejects stores with a rollback +journal or write-ahead log beside them; this guard does not prevent a writer +from starting after the check. """ @@ -52,9 +52,8 @@ class ImmutableSqliteDict(Mapping): FileNotFoundError If ``path`` is not an existing file RuntimeError - If a ``-journal`` or ``-wal`` file sits next to ``path``: a writer is - mid-transaction or crashed in one, and an immutable read would see - uncommitted data + If a ``-journal`` or ``-wal`` file sits next to ``path``; the store + may have an active writer or require recovery before an immutable read """ From e577ba45990a9c5f028fd9e1379cba96a52bdb21 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:43:34 +0200 Subject: [PATCH 03/43] docs(testing): present-tense comments in simulate.py Co-Authored-By: GPT-6.1 Sol --- src/shapepipe/testing/simulate.py | 62 ++++++++++++------------------- 1 file changed, 24 insertions(+), 38 deletions(-) diff --git a/src/shapepipe/testing/simulate.py b/src/shapepipe/testing/simulate.py index 76933cc80..aaf7bdb26 100644 --- a/src/shapepipe/testing/simulate.py +++ b/src/shapepipe/testing/simulate.py @@ -1,7 +1,7 @@ """Simulation utilities for ShapePipe validation tests. -Stable across shapepipe branches and ngmix versions — contains only galsim -and numpy, no shapepipe processing or ngmix fitting code. +Uses only galsim and numpy, keeping image generation independent of +ShapePipe processing and ngmix fitting code. Author: Axel Guinot """ @@ -59,40 +59,35 @@ def make_data( guardrail needs: the PSF carries a shape that an unbiased deconvolution must remove from a round galaxy. wcs : galsim.BaseWCS, optional - Uniform drawing WCS. Default None means ``galsim.PixelScale - (pixel_scale)`` with the legacy world-space sub-pixel shift draw — - bit-identical rng consumption and values to earlier versions of this - function. A non-trivial jacobian (rotation/shear — the ngmix#72 - sensitivity axis) switches the shift draw to PIXEL space (uniform - ±0.5 pixel per axis), guaranteeing the object lands within half a - pixel of the stamp centre — the contract that round-to-nearest-pixel - centroid logic (e.g. ngmix ``centroid_source="wcs"``) depends on. - Note ``psfs_sigmas`` are HSM pixel-unit sigmas of the *WCS-drawn* - PSF stamp; under a sheared WCS they are not circularly symmetric - measures. + Uniform drawing WCS. With None, use ``galsim.PixelScale(pixel_scale)`` + and draw shifts uniformly in world space within ±pixel_scale/2. + With an explicit WCS, draw shifts uniformly within ±0.5 pixel per + axis and transform them to world space; see the centroid contract below. + ``psfs_sigmas`` are HSM pixel-unit sigmas of the WCS-drawn model PSF + stamp, not circularly symmetric measures under a sheared WCS. return_centers : bool, optional If True, also return per-epoch true object centres as ``galsim.PositionD`` in galsim image coordinates (1-based; stamp - centre is ``(img_size + 1) / 2``). Default False keeps the legacy - 6-tuple return. + centre is ``(img_size + 1) / 2``). Default False returns six items. psf_model_fwhm_ratio : float, optional - Multiplicative size error injected into the PSF *model* stamp handed to - ngmix, relative to the *true* PSF that convolves the galaxy. Default 1.0 - (no error: the same PSF object renders the galaxy and the model stamp, - byte-for-byte identical to the legacy behaviour). A value != 1.0 makes - the model PSF the wrong size — over-sized (>1) over-deconvolves, under- - sized (<1) leaves residual smoothing — so metacal deconvolves by a PSF - that does not match the one in the data. This is the controlled-PSF- - model-error knob: it tests whether metacal's response correction (which - is computed with the same wrong model) absorbs a deconvolution error, - the one shape-measurement systematic the zero-error sim could not probe. + Model PSF FWHM divided by the true PSF FWHM. Default 1.0 adds no + size error. Values above 1 over-deconvolve; values below 1 leave + residual smoothing. This controlled mismatch tests whether metacal's + response correction absorbs an error in its deconvolution model. psf_model_shear : tuple of float or None, optional Extra ellipticity (Δg1, Δg2) added to the PSF *model* stamp on top of ``psf_shear``, relative to the true PSF. Default None (no error). A non-zero value mismodels the PSF shape, injecting a controlled additive (PSF-leakage) error into the recovered shear. When both this is None and ``psf_model_fwhm_ratio`` is 1.0 the model PSF *is* the true PSF object, - so the output is byte-for-byte the legacy result. + so there is no model mismatch before stamp noise is added. + + @sc [label:frame] simulation-subpixel-centres + Shifts stay within half a pixel of the stamp centre on each pixel axis, + as required by round-to-nearest-pixel centroid logic. Both drawing paths + consume random shifts in y-then-x order so seeded simulations are reproducible. + An explicit WCS transforms pixel shifts to world coordinates; drawing a + world-space box under a sheared WCS can exceed the half-pixel bounds. Returns ------- @@ -111,9 +106,7 @@ def make_data( wcs = galsim.PixelScale(scale) def draw_shift(): - # Legacy world-space draw — these exact lines (incl. dy-then-dx - # order) keep rng consumption and values bit-identical to the - # pre-wcs version of this function. + # World-space shifts for PixelScale; see the centroid contract. dy, dx = rng.uniform(low=-scale / 2, high=scale / 2, size=2) return dx, dy, dx / scale, dy / scale @@ -122,10 +115,7 @@ def draw_shift(): transform = np.array([[jac.dudx, jac.dudy], [jac.dvdx, jac.dvdy]]) def draw_shift(): - # Pixel-space draw (same dy-then-dx order): under a sheared - # jacobian a world-space ±scale/2 box maps to a pixel-space - # parallelogram exceeding ±0.5 pixel, which would silently break - # rounded-centroid logic with off-by-one-pixel centres. + # Transform bounded pixel shifts; see the centroid contract. dpy, dpx = rng.uniform(low=-0.5, high=0.5, size=2) dx, dy = transform @ (dpx, dpy) return float(dx), float(dy), dpx, dpy @@ -151,11 +141,7 @@ def draw_shift(): ), ).shift(dx, dy) - # The PSF *model* stamp handed to ngmix. By default it is the very same - # object that convolved the galaxy (zero model error, byte-for-byte). A - # size and/or shape error makes it differ from the true PSF, so metacal - # deconvolves by the wrong PSF — the systematic the zero-error sim could - # not exercise. + # Reuse the true PSF when neither model-error parameter is set. if psf_model_fwhm_ratio == 1.0 and psf_model_shear is None: psf_model = psf else: From 1ade7aee43b8b053d5655fc5352d3338cba75c60 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:43:34 +0200 Subject: [PATCH 04/43] docs(utilities): present-tense comments in cfis.py Co-Authored-By: GPT-6.1 Sol --- src/shapepipe/utilities/cfis.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/shapepipe/utilities/cfis.py b/src/shapepipe/utilities/cfis.py index 205d44a6f..27cbabd3b 100644 --- a/src/shapepipe/utilities/cfis.py +++ b/src/shapepipe/utilities/cfis.py @@ -601,8 +601,7 @@ def get_tile_number_list(tile_name_list): return nix_list, niy_list -# Survey-wide object ID: ``tile_id * TILE_UNIQUE_ID_BASE + NUMBER``, with -# ``tile_id = RRR * 1000 + DDD``. Assumes fewer than 10**6 objects per tile. +# Radix of the object-ID encoding; see get_tile_unique_id. TILE_UNIQUE_ID_BASE = 10**6 @@ -634,6 +633,12 @@ def get_tile_unique_id(tile_id, number): Return the survey-wide object ID ``tile_id * 10**6 + number``. + @sc [label:schema] tile-object-id-encoding + Tile IDs and object numbers must each be integers in ``[0, 10**6)``. + Object numbers must be unique within a tile: encoding pairs with this + radix then gives distinct survey-wide IDs and an exact inverse through + :func:`split_tile_unique_id`. + Parameters ---------- tile_id : int From 8fc8dd41b7ec07060a2b40fd1ec679e95d1cfc04 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:43:35 +0200 Subject: [PATCH 05/43] docs(utilities): present-tense comments in field_corners_extractor.py Co-Authored-By: GPT-6.1 Sol --- .../utilities/field_corners_extractor.py | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/shapepipe/utilities/field_corners_extractor.py b/src/shapepipe/utilities/field_corners_extractor.py index a9b4e6c40..da49bc47c 100644 --- a/src/shapepipe/utilities/field_corners_extractor.py +++ b/src/shapepipe/utilities/field_corners_extractor.py @@ -297,12 +297,12 @@ def process_single_header(args): def get_done_ccds(self): """Get Done CCDs. - Read the set of CCD IDs already present in the output file. Resume is - keyed on individual CCD IDs, not exposure numbers: a CCD counts as done - only if its own row is present. This keeps resume correct when a write - was interrupted mid-exposure (the missing CCDs are filled in) and when - a rerun uses an expanded ``--ccd_list`` (the newly requested CCDs are - added), and it never duplicates a row. + @sc [label:schema] footprint-resume-ccd-ids + Resume is keyed on individual CCD IDs, not exposure numbers, so missing + CCDs in a partially written exposure and an expanded ``--ccd_list`` + remain eligible. A row's first column marks its CCD as done; the rest + of the row is not validated. If reading the IDs fails, return an empty + set. Returns ------- @@ -371,9 +371,7 @@ def run(self, args=None): print(f"{n} header files found") - # On resume, read the CCD IDs already written so we can skip them - # per-CCD (not per-exposure): a partially written exposure is completed - # rather than skipped, and no row is ever duplicated. + # Skip IDs returned by get_done_ccds; see its resume contract. done_ccds = set() if resume: done_ccds = self.get_done_ccds() From 180a63855b4bafa665a87684ff33b42e75b2ae5b Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:43:35 +0200 Subject: [PATCH 06/43] docs(utilities): present-tense comments in final_cat.py Co-Authored-By: GPT-6.1 Sol --- src/shapepipe/utilities/final_cat.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/shapepipe/utilities/final_cat.py b/src/shapepipe/utilities/final_cat.py index 644236377..b00a8039e 100644 --- a/src/shapepipe/utilities/final_cat.py +++ b/src/shapepipe/utilities/final_cat.py @@ -5,11 +5,10 @@ * HDF5, as ``make_cat.write_final_cat`` writes it: one dataset per column, in column order, a vector column as a 2-D dataset with one row per object; -* FITS, a binary table (``RESULTS``, HDU 1), as catalogues made before the - HDF5 format are. +* FITS, a binary table (``RESULTS``, HDU 1). The format is decided per file from its content (the HDF5 signature), not its -name, so a campaign or a hand-made directory that mixes the two reads as one. +name, so callers can read a directory containing both formats. """ From 8466badef84d8c938542d215cce2191a8f7bd406 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:43:36 +0200 Subject: [PATCH 07/43] docs(utilities): present-tense comments in header_downloader.py Co-Authored-By: GPT-6.1 Sol --- src/shapepipe/utilities/header_downloader.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/shapepipe/utilities/header_downloader.py b/src/shapepipe/utilities/header_downloader.py index 21b9726b2..ca8ea787f 100644 --- a/src/shapepipe/utilities/header_downloader.py +++ b/src/shapepipe/utilities/header_downloader.py @@ -213,10 +213,10 @@ def get_fits_header(self, expnum, client): print(f"Could not create symlink from {link_source}: {e}") # Fall through to download if symlink fails - # Download from VOSpace atomically: copy to a temp file in the same - # directory, then rename on success. An interrupted transfer leaves - # only the temp file behind, so resume never treats a partial download - # as complete. + # @sc [label:custody] header-download-publication + # Copy to a .part file in the destination directory and rename only + # after success: resume treats an existing destination as complete. + # A caught failure removes the partial file; interruption can leave it. tmp_dest = f"{dest}.part" try: client.copy(source, tmp_dest, head=True) From 3c42ef2384fe5c942f25880b095e85b25b388d7f Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:43:36 +0200 Subject: [PATCH 08/43] docs(utilities): present-tense comments in mask_query.py Co-Authored-By: GPT-6.1 Sol --- src/shapepipe/utilities/mask_query.py | 69 +++++++++------------------ 1 file changed, 23 insertions(+), 46 deletions(-) diff --git a/src/shapepipe/utilities/mask_query.py b/src/shapepipe/utilities/mask_query.py index d0920f27e..3a7e91bd1 100644 --- a/src/shapepipe/utilities/mask_query.py +++ b/src/shapepipe/utilities/mask_query.py @@ -1,46 +1,33 @@ """MASK QUERY. -The one healsparse lookup in ShapePipe. +Shared healsparse lookup at object sky positions. -ShapePipe does not generate or rasterize masks. Sky-fixed masks are supplied -as healsparse maps and are consumed by *querying them at object positions*: -every object gets its mask value(s) as catalogue columns, and rejection happens -at the catalogue level, never at the pixel level. The only mask that still -reaches pixels is the per-exposure instrument flag image delivered with it. - -Two callers share the primitive defined here: - -* ``make_cat`` writes one ``MASK_`` column per configured map, carrying - the map value verbatim (no interpretation, no filtering); -* ``mask_query`` writes a single integer ``MASK_EXT`` column onto the exposure - SExtractor catalogue, combining the configured maps into "clean (0) or - flagged (nonzero)" so that ``setools`` — whose expression language has no - bitwise operators — *could* cut on ``MASK_EXT == 0``. The shipped selection - does not; the column is carried for measurement (see that module). +``query_map`` returns map values verbatim; ``flag_positions`` combines maps +into a clean (0) or flagged (nonzero) integer. Catalogue column definitions +and selection policy belong to the callers in ``modules/make_cat_package`` +and ``modules/mask_query_package``. Partial reads ------------- -Never read a whole map. The UNIONS products are ~550 MB each and ``mask_query`` -runs PER CCD, so a full read would cost ~22 GB of I/O per 40-CCD exposure to +Read only the coverage pixels needed by the catalogue. The UNIONS products +are ~550 MB each and the mask-query runner operates per CCD, so a full read +would cost ~22 GB of I/O per 40-CCD exposure to answer questions about a 0.06 deg² footprint. Instead the coverage index is read first (``HealSparseCoverage``, a few hundred kB), the coverage pixels the catalogue actually touches are computed with ``hpgeom.angle_to_pixel`` at the map's ``nside_coverage``, and only those get loaded. Every queried position falls inside exactly one coverage pixel and all of them -are requested, so the padding by ``hpgeom.neighbors`` is insurance rather -than necessity — at ``nside_coverage=128`` a coverage pixel of the star map -is ~23 kB, so the padding costs under a megabyte and buys immunity to any -edge convention we did not think of. ``test_partial_read_matches_full`` is -what actually holds the two paths equal. - -Measured on the DR6 star-body map — the bit-2 rung of the ladder the configs -name ``mask_ugriz_nside131072_n4.hsp``, run against the staged single-band copy -``mask_r_nside131072_n4.hsp``, 583 MB, ``nside_coverage=128`` — for 2000 +are requested. Padding with ``hpgeom.neighbors`` guards against edge +conventions; at ``nside_coverage=128`` a coverage pixel of the star map is +~23 kB, so this costs under a megabyte for a CCD-sized query. +``test_partial_read_matches_full`` checks equality with a full-map read. + +Measured on the DR6 star-body map's single-band copy +``mask_r_nside131072_n4.hsp`` (583 MB, ``nside_coverage=128``), for 2000 positions in one CCD-sized box, one process each: partial 0.102 s / 157 MiB -peak RSS, full 12.8 s / 3364 MiB, identical values. Per 40-CCD exposure that -is ~4 s against ~8.5 min of map reading, and the query adds ~0.15 GB to a rule -already asking for 16 GB. +peak RSS, full 12.8 s / 3364 MiB, identical values. Extrapolated to a 40-CCD +exposure, map reading takes ~4 s rather than ~8.5 min. Coverage -------- @@ -49,22 +36,16 @@ ``-1`` for integer maps. ``make_cat`` passes that sentinel through verbatim, which is the documented off-map flag for the final catalogue. -Coverage is reported from the COVERAGE MASK, not ``valid_mask=True``. For a +@sc [label:schema] mask-coverage-index +Coverage is reported from the coverage mask, not ``valid_mask=True``. For a boolean map — which is what the UNIONS per-bit products are — healsparse stores only the ``True`` pixels, so ``valid_mask`` returns the value itself and cannot tell "inside the footprint and clean" from "outside it entirely". -The coverage mask can, at ``nside_coverage`` resolution, which is the scale the -question is asked at anyway: does this map reach this exposure at all? +The coverage mask distinguishes them at ``nside_coverage`` resolution. -``flag_positions`` treats off-coverage as **not flagged**, for both map kinds. -This makes the integer case agree with the boolean case (whose sentinel is -literally ``False``) rather than diverge from it, and it keeps a map whose -coverage does not reach an exposure from silently rejecting every star on it. -That is also why the all-off-coverage case is logged as a WARNING: it is -indistinguishable from "nothing is masked here" in the output column, so it has -to be distinguishable in the log. +See ``flag_positions`` for the off-coverage flag convention and warning. -:Author: Claude Fable 5, for PR #847 +:Author: Claude Fable 5 """ @@ -235,9 +216,6 @@ def flag_positions(paths, ra, dec, bits=None, w_log=None): (``value & bits``); ``0`` where the value is zero or the position is outside coverage. - Contributions are combined with a bitwise OR, so the returned flag is zero - for a clean object and carries the union of the bits that fired otherwise. - Parameters ---------- paths : list @@ -272,8 +250,7 @@ def flag_positions(paths, ra, dec, bits=None, w_log=None): contribution = values.astype(np.int64) else: integer = values.astype(np.int64) - # Off-coverage: the sentinel, negative by healsparse convention. - # Zeroed rather than OR-ed in, see this module's docstring. + # Exclude negative sentinels from the OR; see the flag contract. integer = np.where(integer < 0, 0, integer) if bits is not None: integer &= bits From 09882f68c1a9f56a2f79eec30497045e87090de5 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:43:36 +0200 Subject: [PATCH 09/43] docs(testing): present-tense comments in conftest.py Co-Authored-By: GPT-6.1 Sol --- conftest.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/conftest.py b/conftest.py index f481d46d1..596f1ef28 100644 --- a/conftest.py +++ b/conftest.py @@ -3,7 +3,8 @@ Markers, environment detection, and the candide skip policy live here so every test module — wherever it sits in the tree — sees the same rules. -Markers (also declared in ``pyproject.toml`` so ``--strict-markers`` is on): +See ``pyproject.toml`` for marker declarations and pytest defaults. +Markers used by the suite include: * ``slow`` — heavy compute (minutes), not part of the fast inner loop. * ``candide`` — needs the candide cluster and/or its real on-disk data; @@ -42,10 +43,7 @@ # Candide detection # --------------------------------------------------------------------------- # -# Candide compute / login nodes are named c01, c03, n22..n36, etc. The login -# host this suite is most often driven from is ``c03``. We match the candide -# node-name families rather than a fixed list so new nodes are covered, and -# allow an explicit override for CI or odd hostnames. +# Match Candide node-name families rather than a fixed list of hosts. _CANDIDE_HOST_RE = re.compile(r"^(c\d{2}|n\d{2})$", re.IGNORECASE) @@ -54,7 +52,7 @@ def on_candide(): The check is, in order: an explicit ``SHAPEPIPE_ON_CANDIDE`` override (``1``/``0``), then the hostname against the candide node-name families - (``c0x`` login, ``nXX`` compute; whole bare hostname, so ``c6.nibi.sharcnet`` + (``cXX`` login, ``nXX`` compute; whole bare hostname, so ``c6.nibi.sharcnet`` does not match). Cheap, import-safe, no cluster calls. """ override = os.environ.get("SHAPEPIPE_ON_CANDIDE") @@ -106,9 +104,8 @@ def pytest_collection_modifyitems(config, items): def artifacts_dir(): """Directory where guardrail tests drop plots + status summaries. - The artifacts SEAM: a later GitHub Pages / status step publishes from - here. Created on demand so a clean checkout has nothing to commit until - a test actually emits. + Create ``tests/_artifacts`` when the fixture is requested. Tests write + plots and status summaries here for inspection or publication. """ from pathlib import Path From 550c55d5d052d726236dd2e95b1b64f8ff4cdf58 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:44:21 +0200 Subject: [PATCH 10/43] docs(psfex): present-tense comments in psfex_interp.py Co-Authored-By: GPT-6.1 Sol --- src/shapepipe/modules/psfex_interp_package/psfex_interp.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/shapepipe/modules/psfex_interp_package/psfex_interp.py b/src/shapepipe/modules/psfex_interp_package/psfex_interp.py index 8ebca71a5..5e7877c9c 100644 --- a/src/shapepipe/modules/psfex_interp_package/psfex_interp.py +++ b/src/shapepipe/modules/psfex_interp_package/psfex_interp.py @@ -108,7 +108,7 @@ def _fourth_moments(image, moms, wcs=None): that motivates the sky-coordinate measurement. When ``wcs`` is ``None`` the computation is done consistently in the pixel frame instead (the axes are then the CCD axes and the result is *not* frame-invariant); this path exists - only for the legacy pixel-frame branch. + only for the pixel-frame branch. Parameters ---------- @@ -711,8 +711,7 @@ def _galcat_wcs(self): Read the CCD image WCS from the SExtractor FITS_LDAC galaxy catalogue. The original image header is stored as a card list in the ``LDAC_IMHEAD`` - HDU (HDU 1); this is the same header the (now retired) pixel-to-world - conversion reconstructed to rotate shapes. + HDU (HDU 1). Returns ------- From 17df6ec20a3ad6fef6948baeeef508446d459d50 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:44:22 +0200 Subject: [PATCH 11/43] docs(make-cat): present-tense comments in make_cat.py Co-Authored-By: GPT-6.1 Sol --- .../modules/make_cat_package/make_cat.py | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/shapepipe/modules/make_cat_package/make_cat.py b/src/shapepipe/modules/make_cat_package/make_cat.py index e6640d006..2c3749b43 100644 --- a/src/shapepipe/modules/make_cat_package/make_cat.py +++ b/src/shapepipe/modules/make_cat_package/make_cat.py @@ -315,17 +315,15 @@ def _save_ngmix_data(self, ngmix_cat_path, moments=False): plus four OBJECT/SHEAR-less per-object metadata columns (``NGMIX[m]_MCAL_FLAGS``, ``NGMIX_N_EPOCH``, ``NGMIX_MCAL_TYPES_FAIL``, ``NGMIX_NEIGHBOUR_FLAG`` — the last a blend - flag set when the coadd seg stamp held a non-central footprint, - shapepipe#776). The galaxy is the implicit default object - and carries NO ``OBJECT`` token (``NGMIX_G1_NOSHEAR``, dropping the - ``GAL`` segment carried by the pre-#761 names). The explicit PSF + flag set when the coadd segmentation stamp contains a non-central + footprint). The galaxy is the implicit default object and carries no + ``OBJECT`` token (``NGMIX_G1_NOSHEAR``). The explicit PSF objects are ``PSF_ORIG`` (the original image PSF, fit by :func:`ngmix.average_original_psf`) and ``PSF_RECONV`` (the metacal reconvolution kernel, fit by :func:`ngmix.average_multiepoch_psf`); - see those functions for what each PSF family IS. ``PSF_ORIG`` and - ``PSF_RECONV`` are independent fits of different PSFs, no longer the - single aliased value of the pre-fix code (shapepipe#749). + see those functions for the definitions of each PSF family. The two + families contain independent fits of different PSFs. Parameters ---------- @@ -366,8 +364,7 @@ def _save_ngmix_data(self, ngmix_cat_path, moments=False): self._w_log.warning(warning) ngmix_mcal_types_fail = ngmix_cat_file.get_data()["mcal_types_fail"] - # Per-object blend flag (shapepipe#776): the seg stamp held a - # non-central footprint. Galaxy-only (non-moments), like N_EPOCH. + # Per-object blend flag: galaxy-only (non-moments), like N_EPOCH. ngmix_neighbour_flag = ngmix_cat_file.get_data()["neighbour_flag"] # Needed in both moments and non-moments modes (used unconditionally # below), so read them outside the branch. From 3069816d10b7d482db15714b62fb3a3c341fe74e Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:44:22 +0200 Subject: [PATCH 12/43] docs(starcat): present-tense comments in merge_starcat.py Co-Authored-By: GPT-6.1 Sol --- .../merge_starcat_package/merge_starcat.py | 88 +++++++------------ 1 file changed, 33 insertions(+), 55 deletions(-) diff --git a/src/shapepipe/modules/merge_starcat_package/merge_starcat.py b/src/shapepipe/modules/merge_starcat_package/merge_starcat.py index 5e579f6dd..58f35d5b5 100644 --- a/src/shapepipe/modules/merge_starcat_package/merge_starcat.py +++ b/src/shapepipe/modules/merge_starcat_package/merge_starcat.py @@ -345,8 +345,8 @@ def process(self): model_var.append(model_var_val) model_var_size.append(model_var_val.size) - # ONE ARRAY PER CATALOGUE PER COLUMN (see _stack): the per-value - # python lists this replaces cost ~10x the input bytes. + # Keep one array per catalogue per column (see _stack). + # Per-value Python lists cost ~10x the input bytes. # positions pos = starcat_j[self._hdu_table].data["GLOB_POSITION_IMG_LIST"] x.append(np.asarray(pos[:, 0])) @@ -510,7 +510,7 @@ def process(self): # Write file output.save_as_fits(data, sex_cat_path=self._input_file_list[0][0]) - # MKDEBUG; Implement this in file_io + # Record the input catalogue type in the output table header. if self._input_cat_type: with fits.open(out_path, mode="update") as hdu_list: header = hdu_list[1].header @@ -574,8 +574,8 @@ def __init__( ("HSM_RHO4_STAR", "HSM_RHO4_STAR"), ("HSM_FLAG_PSF", "HSM_FLAG_PSF"), ("HSM_FLAG_STAR", "HSM_FLAG_STAR"), ) - # Present in psfex_interp output, absent from pix2wcs-converted files - # (MKDEBUG); zero-filled when missing rather than failing the merge. + # Present in psfex_interp output, absent from pix2wcs-converted files; + # zero-filled when missing rather than failing the merge. _OPTIONAL = (("MAG", "MAG"), ("SNR", "SNR"), ("ACCEPTED", "ACCEPTED")) def _ccd_nb(self, path): @@ -587,11 +587,10 @@ def process(self): Process merging. - TWO PASSES, AND NEITHER HOLDS THE CAMPAIGN TWICE. The first reads only - the FITS HEADER of every input — NAXIS2, the row count — and never - touches a data block; the second allocates the output columns once, at - their exact final length, and fills them slice by slice. Peak memory is - therefore ONE output plus ONE input catalogue. + The first pass reads row counts (``NAXIS2``) and column dtypes from + FITS headers without touching data blocks. The second allocates each + output column at its final length and fills it slice by slice. Peak + memory is one output plus one input catalogue. @sc [label:schema] psfex-starcat-columns-strict Every ``HSM_*`` column is read by name with no fallback, so the set @@ -599,33 +598,24 @@ def process(self): ``PSFExInterpolator._write_output_validation`` writes (``test_hsm_column_seams``). - What this replaces, in two steps, is instructive about the cost of the - obvious code. Accumulating each column into a python LIST OF VALUES — - ``x += list(data["X"])`` — turned 4 bytes of float32 payload into a - 32-byte object plus an 8-byte pointer, measured at ~10x the input bytes - end to end and putting a full-survey merge (~20k exposures x 40 CCDs) at - ~400 GB. Accumulating one ARRAY PER CATALOGUE and concatenating once - brought that to ~5.5x. This pass structure removes what was left of the - accumulation: there are no chunks, and no concatenate that must hold its - inputs and its result at the same time. - - ``self._input_file_list`` MUST BE ITERABLE TWICE, which the module - runner's list is. A one-shot generator is not, and would silently merge - nothing on the second pass — hence the explicit row-count check below. + Per-value Python lists measure ~10x the input bytes (~400 GB for + ~20k exposures x 40 CCDs); arrays accumulated before concatenation + measure ~5.5x. Two passes avoid retaining both concatenation inputs + and their result. + + @sc [label:operations] starcat-two-pass-input-stability + Inputs must remain unchanged across both passes, and + ``self._input_file_list`` must support repeated iteration. The final + row-count check rejects a mismatch rather than writing unfilled rows. """ self._w_log.info( f"Merging {len(self._input_file_list)} star catalogues" ) # --- pass 1: row counts and dtypes, from headers alone -------------- - # THE OPTIONAL COLUMNS ARE A PER-FILE QUESTION, NOT A PER-MERGE ONE. - # A pix2wcs-converted catalogue has no MAG/SNR/ACCEPTED while an - # ordinary one does, and a merge can be handed both. Deciding from the - # first file alone got it wrong in both directions: converted-first - # zero-filled the real values of every ordinary file behind it, and - # ordinary-first raised KeyError on the first converted one. So the - # dtype comes from ANY file that carries the column, and pass 2 asks - # each file for itself. + # Optional MAG/SNR/ACCEPTED columns can differ between inputs: + # pix2wcs catalogues lack them. Take the dtype from any file carrying + # the column; pass 2 zero-fills only files that lack it. names, dtypes, opt_dtypes, n_total = [], None, {}, 0 for name in self._input_file_list: try: @@ -634,10 +624,9 @@ def process(self): hdu = starcat_j[self._hdu_table] n_rows = hdu.header["NAXIS2"] # ColDefs.dtype describes the table without reading it. - # NOTE: it is the RAW storage dtype and ignores TSCAL/TZERO, - # so a scaled column would be allocated narrower than the - # values .data returns. Latent, not live: no validation_psf - # column is scaled. Read the dtype off .data if one ever is. + # This is the raw storage dtype, ignoring TSCAL/TZERO. + # Validation columns must be unscaled; scaled columns need + # the dtype of .data to preserve their decoded values. cols = hdu.columns.dtype if dtypes is None: dtypes = cols @@ -658,8 +647,8 @@ def process(self): data = {out: np.empty(n_total, dtype=dtypes[col]) for out, col in self._COLUMNS} for out, col in self._OPTIONAL: - # A column no file carries still gets a column, zero-filled, in the - # positional dtype the old code used for it. + # Columns absent from every file use X's dtype and are zero-filled + # in pass 2. data[out] = np.empty(n_total, dtype=opt_dtypes.get(col, dtypes["X"])) # CCD_NB is one string per catalogue, repeated over its rows; its width # is the widest CCD number in the campaign, which pass 1 already knows. @@ -682,7 +671,7 @@ def process(self): for out, col in self._COLUMNS: data[out][sl] = data_j[col] for out, col in self._OPTIONAL: - # THIS file's schema, not the merge's: zero-fill only the files + # Use this file's schema: zero-fill only the files # that actually lack the column. data[out][sl] = data_j[col] if col in have else 0 data["CCD_NB"][sl] = self._ccd_nb(name[0]) @@ -698,8 +687,7 @@ def process(self): f"merge_starcat: pass 1 counted {n_total} rows, pass 2 filled " f"{at} — is the input list iterable more than once?") - # Prepare output FITS catalogue - # MKDEBUG: SEx_cat=True -> False + # Prepare a plain FITS catalogue without copying a SExtractor HDU. out_path = f"{self._output_dir}/full_starcat-0000000.fits" output = file_io.FITSCatalogue( out_path, @@ -707,18 +695,13 @@ def process(self): SEx_catalogue=False, ) - # `data` was built by the two passes above (size stored as T = 2 - # sigma^2); every column is already an array of its final length. - - # Write file - # MKDEBUG for psf conv (pix2WCS) files do not write as SExtractorCat; - # we do not want to copy the first input data content to HDU #1. - # sex_cat_path=self._input_file_list[0][0], + # Write the final-length arrays as a plain FITS table without copying + # an input SExtractor HDU (size stored as T = 2 sigma^2). output.save_as_fits( data, overwrite=True, ) - # MKDEBUG; Implement this in file_io + # Record the input catalogue type in the output table header. if self._input_cat_type: with fits.open(out_path, mode="update") as hdu_list: header = hdu_list[1].header @@ -859,13 +842,8 @@ def process(self): ra.append(np.asarray(data_j["XWIN_WORLD"])) dec.append(np.asarray(data_j["YWIN_WORLD"])) - # PRE-EXISTING BUG, LEFT ALONE DELIBERATELY: these four REBIND the - # accumulators initialised above rather than appending to them, so - # only the LAST input file's ellipticities reach the output while - # every other column carries the whole merge. Setools is not wired - # to any workflow path today; fixing it is its own change with its - # own verification, and doing it silently inside a memory rewrite - # would bury it. + # These assignments retain only this file's ellipticities; + # the other columns accumulate rows from every input. m11, m20, m02 = self.get_moments(data_j) eps1, eps2 = self.get_ellipticity(m11, m20, m02, "epsilon") chi1, chi2 = self.get_ellipticity(m11, m20, m02, "chi") From 9ce29fd2401d1947763e68b2ca54166dfc6e9871 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:44:22 +0200 Subject: [PATCH 13/43] docs(vignetmaker): present-tense comments in vignetmaker modules Co-Authored-By: GPT-6.1 Sol --- .../vignetmaker_package/vignetmaker.py | 19 +++++++------------ src/shapepipe/modules/vignetmaker_runner.py | 4 ++-- 2 files changed, 9 insertions(+), 14 deletions(-) diff --git a/src/shapepipe/modules/vignetmaker_package/vignetmaker.py b/src/shapepipe/modules/vignetmaker_package/vignetmaker.py index 586dae28e..84f23596f 100644 --- a/src/shapepipe/modules/vignetmaker_package/vignetmaker.py +++ b/src/shapepipe/modules/vignetmaker_package/vignetmaker.py @@ -25,10 +25,8 @@ def get_stamps(image, positions, rad): The image is zero-padded by ``rad`` on every side and a ``(2 * rad + 1, 2 * rad + 1)`` stamp is sliced around the integer pixel - nearest each position (``numpy.round``). The stamp values are - bit-identical to the ``sf_tools.image.stamp.FetchStamps`` this replaces - for in-bounds positions; unlike ``FetchStamps``, an out-of-bounds centre - raises rather than being silently wrapped modulo the image shape. + nearest each position (``numpy.round``). An out-of-bounds centre raises + rather than wrapping modulo the image shape. Parameters ---------- @@ -445,14 +443,11 @@ def _get_stamp_me(self, image_dirs, image_pattern): output_dict[id_tmp][index]["VIGNET"] = final_list[j][1][ where_res[0] ] - # Sub-pixel [row, col] offset of the coadd centroid from - # the integer pixel this stamp was extracted around, plus - # that integer pixel itself. Both ride with the vignette - # (no sidecar file); OFFSET is consumed by the ngmix "wcs" - # (coadd) centroid source so the extraction and the - # centroid prior share one rounding (see #767), and - # INT_POS + OFFSET reconstruct the absolute exposure - # centroid for diagnostics. + # @sc [label:coupling] vignette-centroid-rounding + # Store the [row, col] OFFSET and INT_POS from extraction + # with the vignette. The ngmix "wcs" centroid prior must + # share that rounding; INT_POS + OFFSET reconstructs the + # absolute exposure centroid for diagnostics. output_dict[id_tmp][index]["OFFSET"] = final_list[j][3][ where_res[0] ] diff --git a/src/shapepipe/modules/vignetmaker_runner.py b/src/shapepipe/modules/vignetmaker_runner.py index a31d818ae..503fd7dcc 100644 --- a/src/shapepipe/modules/vignetmaker_runner.py +++ b/src/shapepipe/modules/vignetmaker_runner.py @@ -106,7 +106,7 @@ def vignetmaker_runner( # Multi-epoch exposures if config.has_option(module_config_sec, "ME_IMAGE_EXP_DIR"): - # v2.0: locate runner output dirs via the $SP_EXP tree + # Locate runner output directories in the configured exposure tree. exp_base_dir = config.getexpanded( module_config_sec, "ME_IMAGE_EXP_DIR" ) @@ -128,7 +128,7 @@ def vignetmaker_runner( ) image_dirs.append(dirs) else: - # v1: run-log based lookup via symlinked exposure run dirs + # Locate symlinked exposure output directories via the run log. modules = config.getlist(module_config_sec, "ME_IMAGE_DIR") image_dirs = [] for module in modules: From 908d9553a8a11c7773768697ae35e3e80e0f830c Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:44:23 +0200 Subject: [PATCH 14/43] docs(mccd): present-tense comments in shapepipe_auxiliary_mccd.py Co-Authored-By: GPT-6.1 Sol --- src/shapepipe/modules/mccd_package/shapepipe_auxiliary_mccd.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/shapepipe/modules/mccd_package/shapepipe_auxiliary_mccd.py b/src/shapepipe/modules/mccd_package/shapepipe_auxiliary_mccd.py index 170e746b9..b4cd9ad2d 100644 --- a/src/shapepipe/modules/mccd_package/shapepipe_auxiliary_mccd.py +++ b/src/shapepipe/modules/mccd_package/shapepipe_auxiliary_mccd.py @@ -462,8 +462,7 @@ def shapepipe_write_output( ): r"""Write ShapePipe Output. - Save interpolated PSFs dictionary to FITS file. The saved files are - compatible with the previous ShapePipe's standard. + Save interpolated PSFs and optional shapes to a SExtractor FITS catalogue. Parameters ---------- From 745d980be0d16570bfea7ab44fbe9c3b542324e7 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:44:23 +0200 Subject: [PATCH 15/43] docs(merge): present-tense comments in merge_sep_cats.py Co-Authored-By: GPT-6.1 Sol --- .../modules/merge_sep_cats_package/merge_sep_cats.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/shapepipe/modules/merge_sep_cats_package/merge_sep_cats.py b/src/shapepipe/modules/merge_sep_cats_package/merge_sep_cats.py index b0be88944..03ef20e99 100644 --- a/src/shapepipe/modules/merge_sep_cats_package/merge_sep_cats.py +++ b/src/shapepipe/modules/merge_sep_cats_package/merge_sep_cats.py @@ -127,8 +127,7 @@ def process(self): cat0.open() list_ext_name = cat0.get_ext_name() - # Inupt ngmix files sometimes have not all sheared versions - # (HDUs 1 - 5 = 1M, 1P, 2M, 2P, NOSHEAR) due to IO errors + # Require all five shear HDUs (1M, 1P, 2M, 2P, NOSHEAR). if len(list_ext_name) < 6: raise IndexError( f"Input ngmix catalogue {input_file} has only" @@ -137,10 +136,8 @@ def process(self): list_col_name = cat0.get_col_names() - # Some older input ngmix catalogues have multiple of 5 HDUs - # if reprocessed and not deleted but appended. - # The following log message should be replaced by raising - # and error in future + # Appending repeated results can leave extra HDUs. Read only the + # primary HDU and the first five shear tables. if len(list_ext_name) > 6: wmsg = f"Cropping input HDUs from {len(list_ext_name)} to 5" self._w_log.info(wmsg) From c207fcee4eaf25e7da89104bf5b4bb8914098db1 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:44:24 +0200 Subject: [PATCH 16/43] docs(setools): present-tense comments in setools.py Co-Authored-By: GPT-6.1 Sol --- src/shapepipe/modules/setools_package/setools.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/shapepipe/modules/setools_package/setools.py b/src/shapepipe/modules/setools_package/setools.py index 7c82c715f..f2a84fe1a 100644 --- a/src/shapepipe/modules/setools_package/setools.py +++ b/src/shapepipe/modules/setools_package/setools.py @@ -666,12 +666,7 @@ def _make_rand_split(self): cat_size = len(np.where(mask)[0]) n_keep = int(np.ceil(cat_size * ratio)) - # Deterministic split, seeded from the unit's file number: the - # train/validation assignment is a pure function of the input - # catalogue, so the PSF star sample (and everything downstream - # of the PSF model) is reproducible run-to-run. An unseeded - # np.random here made the shear catalogue non-reproducible - # upstream of ngmix's own position seeding. + # See the split-seeded-by-file-number contract above. seed = int( re.sub(r"\D", "", self._file_number_string) or 0 ) % (2 ** 32) From 43af58876c55bde6fa51b48160c2c24ceddcfdca Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:44:24 +0200 Subject: [PATCH 17/43] docs(fake-psf): present-tense comments in fake_interp_runner.py Co-Authored-By: GPT-6.1 Sol --- src/shapepipe/modules/fake_interp_runner.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/shapepipe/modules/fake_interp_runner.py b/src/shapepipe/modules/fake_interp_runner.py index bc167f924..0186cbbcb 100644 --- a/src/shapepipe/modules/fake_interp_runner.py +++ b/src/shapepipe/modules/fake_interp_runner.py @@ -7,8 +7,7 @@ (image simulations, true PSF) this runner stands where ``psfex_interp_runner`` and ``mccd_interp_runner`` stand for the real data. It writes the same ``galaxy_psf`` SqliteDict, taken from the simulation's PSF dictionary instead -of a fitted model. ``fake_psf_runner`` is the same module under its original -name, used by the legacy bash job scripts. +of a fitted model. Bash job scripts use the equivalent ``fake_psf_runner``. :Author: Martin Kilbinger From d7fe78fe198861c3ecec67f140a34a26d04acc94 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:44:25 +0200 Subject: [PATCH 18/43] docs(mask-query): present-tense comments in mask-query modules Co-Authored-By: GPT-6.1 Sol --- src/shapepipe/modules/mask_query_package/__init__.py | 2 +- .../modules/mask_query_package/mask_query.py | 11 ++++------- src/shapepipe/modules/mask_query_runner.py | 2 +- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/shapepipe/modules/mask_query_package/__init__.py b/src/shapepipe/modules/mask_query_package/__init__.py index fed9dcd92..30d2a53dd 100644 --- a/src/shapepipe/modules/mask_query_package/__init__.py +++ b/src/shapepipe/modules/mask_query_package/__init__.py @@ -2,7 +2,7 @@ This package contains the module for ``mask_query``. -:Author: Claude Fable 5, for PR #847 +:Author: Claude Fable 5 :Parent module: ``sextractor_runner`` diff --git a/src/shapepipe/modules/mask_query_package/mask_query.py b/src/shapepipe/modules/mask_query_package/mask_query.py index aff212b5a..1222ee5fd 100644 --- a/src/shapepipe/modules/mask_query_package/mask_query.py +++ b/src/shapepipe/modules/mask_query_package/mask_query.py @@ -2,7 +2,7 @@ Class to flag SExtractor detections against external healsparse masks. -:Author: Claude Fable 5, for PR #847 +:Author: Claude Fable 5 """ @@ -91,12 +91,9 @@ def process(self): ori_cat.open() data = ori_cat.get_data() - # A CCD SExtractor found nothing on is tolerated all along this chain - # (setools' ~0.2% attrition, psfex_interp's floor=0 warn), so it must - # not be an error here either. An empty LDAC table has no columns to - # index, so read the positions only when there are rows, and still - # publish an output file — a missing sexcat_ext would look to the file - # handler like a crash rather than like an empty CCD. + # Empty CCD catalogues still need an output file so the file handler + # can distinguish an empty detection result from a failed module. + # Read position columns only when the table has rows. if len(data) == 0: ra = np.zeros(0) dec = np.zeros(0) diff --git a/src/shapepipe/modules/mask_query_runner.py b/src/shapepipe/modules/mask_query_runner.py index f4bc97941..5f2290393 100644 --- a/src/shapepipe/modules/mask_query_runner.py +++ b/src/shapepipe/modules/mask_query_runner.py @@ -2,7 +2,7 @@ Module runner for ``mask_query``. -:Author: Claude Fable 5, for PR #847 +:Author: Claude Fable 5 """ From 94e32a2b73b0e710acf302422939efb0aeabf047 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:44:25 +0200 Subject: [PATCH 19/43] docs(get-images): present-tense comments in get_images_runner.py Co-Authored-By: GPT-6.1 Sol --- src/shapepipe/modules/get_images_runner.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/shapepipe/modules/get_images_runner.py b/src/shapepipe/modules/get_images_runner.py index 28e81b7d5..024f989e9 100644 --- a/src/shapepipe/modules/get_images_runner.py +++ b/src/shapepipe/modules/get_images_runner.py @@ -27,10 +27,8 @@ def get_images_runner( """Define The Get Images Runner.""" # Read config file section - # Copy/download method. getexpanded, not get: the workflow's ini sets - # RETRIEVE = $SP_RETRIEVE (a run-config key since d64f88ed), and plain get - # returns the literal "$SP_RETRIEVE", which then fails the check below. - # RETRIEVE_OPTIONS a few lines down already reads the same way. + # Expand environment variables such as RETRIEVE = $SP_RETRIEVE before + # validating the copy/download method. retrieve_method = config.getexpanded(module_config_sec, "RETRIEVE") retrieve_ok = ["vos", "symlink"] if retrieve_method not in retrieve_ok: From 8348324ce324e49db2b742ccad0597ccaa9796a3 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:44:54 +0200 Subject: [PATCH 20/43] docs(workflow): present-tense comments in ranges and reports Co-Authored-By: GPT-6.1 Sol --- workflow/scripts/ngmix_range.py | 142 ++++++++------------------ workflow/scripts/run_report.py | 175 ++++++++++---------------------- 2 files changed, 96 insertions(+), 221 deletions(-) diff --git a/workflow/scripts/ngmix_range.py b/workflow/scripts/ngmix_range.py index 3bb816f5b..173cb51f7 100644 --- a/workflow/scripts/ngmix_range.py +++ b/workflow/scripts/ngmix_range.py @@ -1,10 +1,8 @@ #!/usr/bin/env python3 """Materialise the ngmix chunk partition once per tile, then serve it. -TWO MODES, ONE WRITER. The partition is computed ONCE, by tile_vignets — the -first member of the fused tile_shape group — and written to the group's -node-local scratch; each of the n_chunks chunk shells then only LOOKS UP its -own row:: +``tile_vignets`` computes the partition and writes it to the ``tile_shape`` +group's node-local scratch. Each chunk shell reads its own row:: # tile_vignets, once (see tile.smk's TILE_NGMIX_RANGES) ngmix_range.py --run-dir $SP_RUN --n-chunks 8 --write $SP_LOCAL/ngmix_ranges.json @@ -13,95 +11,51 @@ eval "$(ngmix_range.py --read $SP_LOCAL/ngmix_ranges.json --chunk 3)" # -> export NGMIX_ROW_MIN=751; export NGMIX_ROW_MAX=1125 -The file is group-internal plumbing, NOT DAG currency: it lives on $SP_LOCAL and -dies with the group job, exactly like the vignette store. It is deliberately not -a rule output — a chunk can only ever read the file its own group job wrote. - -Read mode does NOT fall back to recomputation when the file is absent; it exits -non-zero. The fallback would restore the very failure this design removes (see -DETERMINISM, below). - -The range is still only knowable at EXECUTION time (PRD D4) — a params function -cannot compute it, because params evaluate before the sexcat exists. - -The ranges are 1-based ROW positions in the sexcat, not NUMBER values: ngmix -selects its chunk by row (``ngmix_package.ngmix.chunk_rows``), so covering -[1, N] processes every object exactly once however NUMBER is ordered or -spaced (an external detection catalogue keeps its own NUMBER). The bounds are -CLOSED, never ID_OBJ_MAX = -1 — ngmix treats ``id_obj_max <= 0`` as -unbounded, so an open-ended last chunk silently -re-measures the whole tile instead of its share; rule tile_ngmix carries the -straggler that taught this. - -CHUNKS ARE BALANCED BY EPOCH-WEIGHTED COST, NOT BY OBJECT COUNT. ngmix fits an -object jointly across every exposure it lands on, so its cost scales with -(object, epoch) pairs, not objects: the campaign measured 0.2714 CPU-s per -pair with an intercept consistent with zero, plus ~0.05 CPU-s per-object of -setup. The old equal-count split therefore produced chunks whose cost varied -1.6x within a single tile, tracking their epoch counts at R^2 0.91-1.000. Those -chunks are siblings in the fused tile_shape group job, which ends when its -slowest member does, so the spread was pure wall clock: 7.7 hours over the -34-tile smk-g4 campaign, and on tile 196.307 the slowest chunk ran -26.3 min past the median and took the group to 97.4% of its wall limit. - -Re-splitting all 34 of that campaign's sexcats: the old boundaries leave the -slowest chunk at 1.131x-1.627x its tile's median predicted cost on tile 200.302; the new ones at 1.0000x-1.0002x, and the sum over tiles of -slowest-chunk cost falls 132,875 -> 113,710 predicted CPU-s, 14.4%. +The file lives on ``$SP_LOCAL`` for the group job's lifetime and is not a rule +output; see ``workflow/rules/tile.smk`` for group scratch ownership. Ranges are +computed at execution time because Snakemake params evaluate before the sexcat +exists. + +@sc [label:coupling] ngmix-range-row-partition +Ranges are closed, 1-based sexcat row positions, not NUMBER values, matching +``ngmix_package.ngmix.chunk_rows``. They must cover [1, N] exactly once. +Never use a nonpositive upper bound: ngmix treats it as unbounded and can +re-measure the whole tile. + +Chunks balance epoch-weighted cost because ngmix fits each object jointly +across its exposures and the group waits for its slowest chunk. Measurements +on a 34-tile nibi run give 0.2714 CPU-s per (object, epoch) pair plus ~0.05 +CPU-s of per-object setup. Equal-count chunks vary in cost by 1.6x within a +tile; epoch-weighted splitting reduces the sum of predicted slowest-chunk +costs from 132,875 to 113,710 CPU-s (14.4%). Moving the boundaries does not change measurements: ngmix's RNG is seeded per object from its sky position, so which chunk an object falls in cannot change its measurement (see ``ngmix_package.ngmix.position_seed``). -DETERMINISM HERE IS CORRECTNESS, NOT TIDINESS. A tile's chunk ranges are a -PARTITION of its catalogue rows: if two chunks disagree about the boundaries, -objects are silently measured twice or silently dropped and nothing downstream -notices — merge_sep_cats concatenates whatever it is given. - -SINGLE-WRITER IS WHY THAT NO LONGER DEPENDS ON AGREEMENT. Every chunk reads the -same file, produced by one process from one read of the sexcat, so sibling -chunks cannot disagree even in principle — the boundaries are a fact of the -group job rather than a computation eight processes each have to land on. The -mechanism that closes is a real one: this script used to run independently in -each chunk's shell, and a group holds its chunks open for hours (smk-g4: -6,236-7,762 s of elapsed per chunk), reading — as every job did before the launch -code snapshot (bin/sp) — the LIVE checkout, so an edit landed mid-flight was read -by some chunks and not others. Observed once, live — the transcript is at -tile.smk's range_hash, which is the home for that history. - -The rest of the determinism discipline stays, because it is what makes the -written file trustworthy rather than merely shared: integer arithmetic end to -end (weights scale to milli-epochs so no float ever decides a boundary), and NO -FALLBACK — if the EPOCH extensions cannot be read, or the ranges file is absent -in read mode, this script exits non-zero. A fallback would be the worst -available behaviour precisely because it would apply only to the processes that -hit the failure, shredding the tile's coverage instead of failing it. - -What remains uncovered is the cross-TIME case, and NGMIX_RANGE_HASH (Snakefile) -is what covers it: a RESUME that reruns some chunks after an edit to this script -would otherwise mix old and new boundaries within one tile. +@sc [label:coupling] ngmix-range-single-writer +All sibling chunks must read the partition written once by ``tile_vignets``. +Read mode must fail rather than recompute missing ranges: disagreeing chunk +boundaries silently duplicate or drop objects, and ``merge_sep_cats`` merely +concatenates the results. + +Integer weights make partitioning reproducible across attempts. Missing EPOCH +extensions fail write mode. See the Snakefile's ``NGMIX_RANGE_HASH`` for +invalidation across resumes after splitter edits, and ``bin/sp`` for launch +code snapshots. """ import argparse import json from pathlib import Path -# Weights are integers in milli-epochs (one epoch = 1000) so every boundary is -# decided by exact integer comparison, identically on every run of the splitter -# — which is now what makes the written file REPRODUCIBLE across attempts and -# resumes, rather than what makes eight siblings agree. +# Integer milli-epoch weights make boundary comparisons exact and reproducible. MILLI_EPOCH = 1000 -# The per-object setup cost, expressed in epochs so one integer weight carries -# both terms of the measured cost law: 0.05 CPU-s of setup / 0.2714 CPU-s per -# (object, epoch) = 0.184 epoch-equivalents. It is what keeps the zero-epoch -# objects (13 of 35,298 on tile 186.307) from weighing nothing — they still -# cost ~6% of a typical object, and a chunk handed thousands of them at no extra cost -# would be a straggler of a new kind. Only the RATIO of the two costs moves a -# boundary, which makes this robust: refitting on 186.307's eight measured -# chunk CPU times with the setup term held fixed gives 0.2619 per GEOMETRIC -# epoch (see object_epochs for why that is below 0.2714), i.e. ALPHA 0.191, -# and every boundary on that tile then moves by at most one object. Zeroing -# ALPHA entirely moves them by at most 44. +# Setup cost in epoch-equivalents: 0.05 / 0.2714 = 0.184. This gives +# zero-epoch objects nonzero weight (~6% of a typical object). On tile 186.307, +# refitting with geometric epochs gives 0.191 and moves boundaries by at most +# one object; zeroing the setup term moves them by at most 44. ALPHA_MILLI_EPOCHS = 184 @@ -109,7 +63,7 @@ def row_ranges(epochs, n_chunks: int) -> list[tuple[int, int]]: """Split catalogue rows ``1..len(epochs)`` into ``n_chunks`` closed ranges. ``epochs[i]`` is row ``i + 1``'s geometric epoch count. The ranges are - CONTIGUOUS — ``NGMIX_ROW_MIN``/``NGMIX_ROW_MAX`` is an interval, not a set — + contiguous — ``NGMIX_ROW_MIN``/``NGMIX_ROW_MAX`` is an interval, not a set — and tile ``[1, n_obj]`` exactly, so every object is measured once. The objective is the slowest chunk, not the average one, because the @@ -119,12 +73,9 @@ def row_ranges(epochs, n_chunks: int) -> list[tuple[int, int]]: which is arbitrary but fixed, and fixed is the property that matters. With ``n_obj < n_chunks`` the first ``n_obj`` chunks take one object each - and the remainder are EMPTY, written ``(n_obj + 1, n_obj)``: ``lo = hi+1`` - is the canonical empty closed interval and preserves the chain - ``ranges[k][0] == ranges[k-1][1] + 1``. Deliberately not ``(1, 0)``, which - is what the old equal-count split emitted here — ``ID_OBJ_MAX = 0`` is - ngmix's unbounded sentinel, so each empty chunk would have re-measured the - ENTIRE tile. + and the remainder are empty, written ``(n_obj + 1, n_obj)``. This preserves + ``ranges[k][0] == ranges[k-1][1] + 1`` and keeps the upper bound positive + as required by the module's row-partition contract. """ if n_chunks < 1: raise ValueError(f"n_chunks must be >= 1, got {n_chunks}") @@ -194,22 +145,19 @@ def object_epochs(run_dir: Path): ``LDAC_OBJECTS`` row order and ``CCD_N < 0`` where the object misses that exposure. Summing ``CCD_N >= 0`` across them reproduces the sexcat's geometric - ``N_EPOCH`` — checked row by row against the pre-change catalogue for 186.307, - ``run_sp_tile_Mc/.../final_cat-186-307.fits``, all 35,298 of them, 7 extensions, + ``N_EPOCH`` — verified on tile 186.307: 35,298 rows, 7 extensions, 116,727 pairs, mean 3.31. The post-process is upstream of the whole tile_shape group, so the extensions always exist by the time ngmix runs; their absence is a broken tile, not a case to accommodate. - This is a GEOMETRIC count and mildly over-states the work, because ngmix - fits only epochs with a validated PSF (the final catalogue's N_EPOCH) and + This geometric count mildly overstates the work, because ngmix + fits only epochs with a validated PSF and drops epochs it cannot fit. The same catalogue's NGMIX_N_EPOCH is lower for 2,203 of the 35,298 objects and never higher: 113,947 pairs against - 116,727, 2.4%. That gap is the whole of the ~3.4% by which this cost model - over-predicts the tile's measured chunk times — refit on NGMIX_N_EPOCH the - law is 0.2681 CPU-s per pair at R^2 0.997, against 0.2619 at R^2 0.973 on - the geometric count. The better number is unavailable before ngmix runs and - is not worth wanting anyway: splitting on it moves boundaries by up to 177 - objects and improves the slowest chunk by 1.07%. + 116,727, a 2.4% difference. A fit using NGMIX_N_EPOCH gives 0.2681 CPU-s + per pair at R^2 0.997, against 0.2619 at R^2 0.973 using geometric counts. + Fitted epoch counts are unavailable before ngmix; on this tile they improve + the predicted slowest chunk by only 1.07%. Only the EPOCH extensions are read. ``LDAC_OBJECTS`` carries a 10 kB VIGNET per row (376 MB on 186.307) and pulling it in would cost more than diff --git a/workflow/scripts/run_report.py b/workflow/scripts/run_report.py index bd846d34d..86a5367b9 100644 --- a/workflow/scripts/run_report.py +++ b/workflow/scripts/run_report.py @@ -1,13 +1,11 @@ #!/usr/bin/env python3 """``sp report`` — the run's success/failure tables, read from the manifests. -NOT a DAG node. A report rule that declared all tiles' outputs as inputs would be -a descendant of every job, so one hard failure under --keep-going would poison -its cone and the report would never run — the exact scenario it exists for. So it -is a plain script, runnable at any time including mid-run; the Snakefile's -onsuccess/onerror hooks call it so every invocation ends with one. +This is a plain script, not a DAG node: depending on all tile outputs would +prevent reporting after a hard failure under ``--keep-going``. It can run +mid-run; the Snakefile's onsuccess/onerror hooks call it at invocation end. -It reads two things and nothing else (PRD D3): +It reads declared units and recorded verdicts: * the **index** (``run_index.sqlite``) — the units the run declared, and the tile->exposure edges that let an exposure failure be blamed on the tiles it @@ -15,24 +13,16 @@ * the **verdicts** written by ``completeness.py check`` — per-runner found/expect and log-scraped failure reasons — in the unit's ``logs/`` (every run) and ``manifests/`` (successes only; completeness.py - argues the split). + defines the record placement). -Both dirs are read here and the status comes from the file BODY, so a failed -stage speaks through its log while a successful one is corroborated by two -identical files. A unit with neither ran nothing. Also read, for continuity with -stores written before this convention: ``manifests/.failed.json``, which -is where failure evidence used to go. +Both directories are read and status comes from the file body. The manifest +glob also accepts ``.failed.json`` records. No record means "not run" +for reporting purposes; it is not evidence that no process executed. -A reclaimed exposure has neither dir: ``clean_exposure`` deleted both, after -copying the manifests into ``/cleaned.json``. That tombstone is read as the -unit's record and the unit is reported as **cleaned** — not "not run", and it -blocks no tile. - -A reclaimed TILE is the same story with one asymmetry: ``clean_tile`` deletes -``logs/`` but cannot empty ``manifests/``, because two of those manifests are -currency other mechanisms read (SURVIVING_TILE_STAGES). So the "records on disk -mean a rebuilt chain" test takes that survivor set explicitly — see -``absorb_tombstones``. +Reclaimed units' records come from ``cleaned.json`` tombstones. See +``clean_exposure.py`` and ``clean_tile.py`` for reclamation and surviving +manifests, and ``absorb_tombstones`` for the generation boundary. Cleaned +exposures block no tile. No disk scanning: counting products is the *check's* job, done once at the moment the products were fresh. A unit with no manifest for a stage is "not run" — which @@ -41,10 +31,9 @@ Records are discovered by glob (``tiles/*/*/manifests/*.json`` and ``tiles/*/*/logs/*.json``), not by constructed path: units are found rather than named, so a store holding a unit the index never heard of still reports. The -depth is FIXED at two, because the sharded layout is exactly two levels -(``tiles///``) — a ``**`` walked the whole tree, including every -``output/`` a run has not reclaimed yet, to find records that can only ever be -at one depth. +depth is fixed at two, matching ``tiles///`` and avoiding a +recursive walk of unreclaimed ``output/`` trees. With an index, tallies cover +indexed units; without one, they cover units discovered from records. """ import argparse @@ -67,50 +56,32 @@ TILE_STAGES.insert(TILE_STAGES.index("tile_detect"), "tile_get_catalogue") EXP_STAGES = ["exp_get_images", "exp_split", "exp_psf"] -# exp_persist is DELIBERATELY NOT in that list. This report disk-scans the -# scratch run_dir, and exp_persist's manifest is the one exposure manifest that -# lives on products_dir instead — that placement is what makes it survive -# clean_exposure. Listed here it would read as "not run" for every exposure in -# the campaign. Reporting on the persisted products means scanning the second -# root, which is a report this one does not yet do. - -# The manifests clean_tile leaves on disk (workflow/scripts/clean_tile.py names -# the mechanism that owns each). Their presence is therefore NOT evidence that a -# tile's chain was rebuilt, which absorb_tombstones needs to know -# to read a reclaimed tile's record out of its tombstone. +# exp_persist is excluded: its manifest lives on products_dir, while this +# report reads only run_dir. Listing it would mark every exposure "not run". + +# See clean_tile.py for ownership of these surviving manifests. Their +# presence alone is not evidence of a rebuilt chain (absorb_tombstones). SURVIVING_TILE_STAGES = frozenset({"tile_vignets", "tile_find_exposures"}) STATUSES = ("complete", "warn", "failed", "not_run") def _rank(m: dict) -> int: - """How BAD a record is, as a position in STATUSES; unknown sorts worst.""" + """Severity as a position in STATUSES; unknown statuses sort worst.""" return STATUSES.index(m["status"]) if m.get("status") in STATUSES else len(STATUSES) def keep_worst(records: dict, stage: str, m: dict) -> None: - """Collapse the several records of one stage into the WORST of them. - - ONE rule, used by both readers in this module, and that is the point. - Several files map to one (unit, stage) either way: on disk, a stage's - manifest and its byte-identical log, plus the eight ngmix chunks, which all - carry ``stage: "tile_ngmix"`` under per-chunk filenames; inside a tombstone, - those same eight chunks again, keyed by file stem. - - absorb_tombstones used to resolve that collision by first-key-wins - (``setdefault`` over sorted stems), so only ``tile_ngmix_1`` survived and a - ``warn`` on any other chunk disappeared the moment the tile was reclaimed — - ``tile_ngmix ok 0 warn 1`` before the clean, ``ok 1 warn 0`` after, and the - tile silently left the "tiles not complete" table. The data was in the - tombstone the whole time; only the reader dropped it (found in review, - reproduced on 186.307 with chunk 5 flipped to warn). - - What this does NOT fix, because it is not a reclamation bug: a stage's - per-runner ``products`` aggregate is taken from the single surviving record, - so tile_ngmix attrition is counted over one chunk of eight. That is true - before and after a clean, identically — it is the price of collapsing the - chunks to one stage row, and it is the same on both paths by construction - now. + """Collapse a stage's records to the one with the worst status. + + @sc [label:operations] report-worst-stage-status + Use the same severity reduction for on-disk and tombstone records so a + warning or failure in any ngmix chunk survives reclamation. Chunks share + ``stage: "tile_ngmix"`` even though their filenames differ. + + Product counts come from the retained record only, not the sum of chunks; + ngmix attrition therefore describes one chunk, not a whole tile. Equal + severity keeps the first record encountered. """ prev = records.get(stage) if prev is None or _rank(m) > _rank(prev): @@ -120,7 +91,7 @@ def keep_worst(records: dict, stage: str, m: dict) -> None: def load_manifests(run_dir: Path, sub: str) -> dict: """``{unit: {stage: verdict}}`` for one store (``tiles`` or ``exp``). - Reads BOTH of a unit's record dirs: ``manifests/`` (the rules' declared + Reads both of a unit's record dirs: ``manifests/`` (the rules' declared outputs, success-only) and ``logs/`` (the rules' ``log:``, written every run and never deleted by snakemake, so this is where a failure survives). @@ -131,7 +102,7 @@ def load_manifests(run_dir: Path, sub: str) -> dict: filename: ngmix chunks share a stage under per-chunk filenames, and a log names the same stage as the manifest beside it. - Several files therefore map to one (unit, stage), and the WORST status wins. + Several files therefore map to one (unit, stage), and the worst status wins. That is what collapses the ngmix chunks to one entry, and it is why a successful stage's two byte-identical records cost nothing while a failure always speaks. A body with no ``stage`` field is skipped: it is not one of @@ -157,39 +128,15 @@ def absorb_tombstones(run_dir: Path, sub: str, manifests: dict, survivors: frozenset = frozenset()) -> set: """Fill in reclaimed units from their ``cleaned.json``; return their ids. - A cleaned exposure has neither ``manifests/`` nor ``logs/``; every manifest - was copied verbatim into the tombstone first. Read them back, or the report - inverts the truth exactly when reclamation works: the exposure shows as "not - run" and blocks the very tiles whose completion authorised the deletion. - - Manifests on disk win if both exist — that is a re-built chain, and the - tombstone is then a stale record of the previous generation. - - ``survivors`` is what makes that test work for TILES. ``clean_tile`` cannot - empty ``manifests/`` the way ``clean_exposure`` does: two of the manifests - there are currency other mechanisms read (SURVIVING_TILE_STAGES below), so a - reclaimed tile ALWAYS has records on disk. Without this argument the - "manifests win" guard fires on every cleaned tile, the tombstone is ignored, - and a fully reclaimed campaign reports as one that ran two stages and - stopped. A unit counts as REBUILT — and keeps the guard — iff it has a record - for some stage that is not a survivor; the default empty set reproduces the - exposure test exactly. - - KNOWN AND DELIBERATE: the guard is all-or-nothing, so a PARTIALLY rebuilt - cleaned tile loses its whole tombstone record. Force a rerun that restores - tile_detect..tile_make_cat but not the prepare stages (which only the - prepare invocation produces) and tile_get_images / tile_uncompress read - "not run" though they did run and nothing invalidated them. - - Not fixed, because the obvious fix is wrong rather than long. Filling - per-stage from the tombstone would, on a unit whose chain is rebuilding, - report the PREVIOUS generation's "complete" for a stage whose manifest is - absent precisely because it is mid-rerun or failed — a stale complete is - invisibly wrong where "not run" is visibly incomplete, and the same hazard - reaches exposures, where the mixing would be silent and campaign-wide. The - all-or-nothing guard is a generation boundary and is worth more than the - cosmetics. A correct fix needs a per-stage notion of which generation a - record belongs to, which nothing here records today. + @sc [label:provenance] report-tombstone-generation-boundary + Absorb a tombstone only if no on-disk record belongs to a stage outside + ``survivors``. Such a record marks a rebuilt chain; mixing in tombstone + stages could report a previous generation's success during a rerun or + failure. Surviving manifests alone do not mark a rebuilt tile. + + This guard is all-or-nothing. A partially rebuilt tile can report prepare + stages as "not run" even when they executed before reclamation. Per-stage + recovery would need generation identifiers, which these records lack. """ cleaned = set() for path in sorted((run_dir / sub).glob("*/*/cleaned.json")): @@ -201,18 +148,14 @@ def absorb_tombstones(run_dir: Path, sub: str, manifests: dict, continue if any(s not in survivors for s in manifests.get(unit, {})): continue - # Collapse the tombstone's per-stem records to one per stage FIRST, - # by the same rule the on-disk path uses (keep_worst), and only then - # merge. Reversing those two steps is what lost a warning chunk. + # Reduce chunk records by severity before merging with live records. absorbed: dict = {} for key, m in (tomb.get("manifests") or {}).items(): if not isinstance(m, dict): continue keep_worst(absorbed, m.get("stage", key), m) for stage, m in absorbed.items(): - # setdefault, not assignment: the surviving on-disk records are the - # live ones and stay authoritative, even though the tombstone's - # copies of them are byte-identical today. + # Surviving on-disk records remain authoritative. manifests[unit].setdefault(stage, m) cleaned.add(unit) return cleaned @@ -242,15 +185,11 @@ def tally_level(units, stages, manifests, cleaned=frozenset()) -> dict: exactly, including a warn on any one of the eight ngmix chunks (keep_worst is what makes that true on the tombstone path as well as on disk). - The per-runner ``products`` aggregate is NOT a per-chunk total: reading it - as a whole-tile figure over-states completeness by 8x (see ``keep_worst``). + See ``keep_worst`` for the single-record product-count limitation. """ per_stage = {} for stage in stages: - # All five status keys are unit-id LISTS, "complete" included: it used - # to be a bare int, which made it the one key a caller had to special- - # case. The emitted JSON gains the complete-unit list; the printed - # counts are len() of it. + # All five status keys hold unit-ID lists; printed counts use len(). t = {"complete": [], "warn": [], "failed": [], "not_run": [], "cleaned": []} agg = defaultdict(lambda: {"found": 0, "expect": 0, "by_unit": {}}) for u in units: @@ -354,9 +293,7 @@ def main() -> None: tile_m = load_manifests(args.run_dir, "tiles") exp_m = load_manifests(args.run_dir, "exp") - # Reclaimed units speak through their tombstones (D5, S5). Tiles pass the - # survivor set: clean_tile leaves two manifests on disk on purpose, and - # without that argument they would read as a rebuilt chain. + # See absorb_tombstones for the survivor-aware generation boundary. cleaned_exp = absorb_tombstones(args.run_dir, "exp", exp_m) cleaned_tiles = absorb_tombstones(args.run_dir, "tiles", tile_m, SURVIVING_TILE_STAGES) @@ -378,20 +315,10 @@ def main() -> None: "exposures": unit_rows(exps, EXP_STAGES, exp_m), } - # Blame propagation: a BLOCKING exposure blocks every tile that reads it. - # Without this, a tile stalled at tile_vignets looks like its own failure. - # - # Blocking means "failed" or "never ran" — NOT "warn". Warn is the expected - # per-CCD attrition (setools rejecting a sparse CCD, psfex_interp short an - # epoch); it is present in essentially every exposure at production scale, so - # counting it here made every exposure block every tile and the table said - # nothing. - # Judged over ALL the exposure's stages, not just the first bad one, so an - # exposure that warns early and fails late still blocks. - # A CLEANED exposure never blocks: its store is gone precisely because every - # consuming tile already had its vignets. Its absorbed manifests are read - # above, so a cleaned exposure that genuinely failed still shows in the - # tables — it just does not get to hold complete tiles hostage. + # An exposure blocks its consuming tiles if any stage is missing or failed, + # not merely warned: per-CCD attrition is expected at production scale. + # Check every stage so an early warning cannot hide a later failure. + # Cleaned exposures never block; see clean_exposure.py for deletion gates. def _blocks(unit): if unit in cleaned_exp: return False From 7c7bfaf3650df3164c40bbd988d0df2e306c5c70 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:45:59 +0200 Subject: [PATCH 21/43] docs(workflow): present-tense comments in Snakefile Co-Authored-By: GPT-6.1 Sol --- workflow/Snakefile | 782 ++++++++++++++------------------------------- 1 file changed, 232 insertions(+), 550 deletions(-) diff --git a/workflow/Snakefile b/workflow/Snakefile index 20780e1be..1037a79d9 100644 --- a/workflow/Snakefile +++ b/workflow/Snakefile @@ -2,33 +2,31 @@ Design / rationale: CosmoStat/shapepipe#848, D1-D5. -`sp run` is TWO snakemake invocations over this one Snakefile: +`sp run` uses two Snakemake invocations over this Snakefile: SP_PHASE=prepare snakemake prepare_all_tiles # Git -> Uz -> Fe, per tile SP_PHASE=compute snakemake all # everything else They are two because the exposure job set is *data-derived*: it comes from the tiles' find_exposures output, and a Snakemake DAG is fixed at parse time. The -join between them is the tile<->exposure index, built HERE at parse time of -invocation 2 (build_index.build(), imported — there is no `sp index` verb) and -loaded into plain dicts. The index is never a rule input, so appending tiles and -rebuilding it changes which jobs exist without invalidating completed work; it -ACCUMULATES across invocations, so a later clean_exposure (S5) sees every -consuming tile of the whole campaign, not just this tile list. - -Neither invocation reads the checkout: `sp run` snapshots the code into the -campaign's state dir and points snakemake at THAT tree, so `workflow.basedir` -below is the snapshot and mid-campaign edits are inert. The mechanism and its -rationale live in bin/sp. - -The atom (D2): one rule == one `shapepipe_run` on one unit; its single declared -output is its MANIFEST (`/manifests/.json`), written by -`completeness.py check`. Product files are not declared — a missing CCD is often -legitimate, and at DR6 scale per-CCD declaration means millions of paths. - -Failure evidence rides on the rule's `log:` (`/logs/.json`): -the manifest says "this stage succeeded", the log says "here is what happened" -(the contract is argued in completeness.py's docstring). +join between them is the tile<->exposure index, built by build_index.build() +at parse time of the compute invocation. The head process loads it into dicts; +job parses query only the edges they need. The index is not a rule input, so +rebuilding it changes the job set without itself invalidating completed work. +It accumulates across invocations so clean_exposure sees every consuming tile +in the campaign, not just the current tile list. + +`sp run` points Snakemake at a campaign code snapshot, so `workflow.basedir` +below is that snapshot. See bin/sp for snapshot creation and reuse. + +ShapePipe stage rules run one `shapepipe_run` on one unit (D2). Most declare a +success manifest (`/manifests/.json`) written by +`completeness.py check`, rather than per-CCD products: missing CCDs can be +legitimate, and per-CCD declarations would mean millions of paths at DR6 scale. +See rules/tile.smk for temporary chunk outputs and final-catalogue publication. + +Stage logs (`/logs/.json`) retain failure evidence separately +from success manifests; see scripts/completeness.py for the verdict contract. """ import fnmatch @@ -41,18 +39,12 @@ import sys from pathlib import Path from snakemake.exceptions import WorkflowError -# Explicit rather than relying on the name snakemake injects into this -# namespace: the one place we log at parse time is a branch that only a -# non-default keep list reaches, and a NameError there would be found by a user. +# Import the logger explicitly for parse-time warnings. from snakemake.logging import logger -# Resolved relative to THIS file, not the working directory: snakemake runs with -# --directory on /scratch (bin/sp) so .snakemake/ state never lands on /project. -# -# config.yaml is always loaded; SP_RUN_CONFIG (bin/sp) is merged on top, then -# the `input_types:` entry for input_type and the `machines:` entry for -# (machine, input_type) fill whatever is still unset, the machine entry winning -# (scripts/run_config.py, shared with bin/sp and container.py). +# Resolve paths relative to this file; bin/sp sets a separate working directory. +# Load config.yaml, then overlay SP_RUN_CONFIG. See scripts/run_config.py for +# the defaults shared with bin/sp and container.py. sys.path.insert(0, str(Path(workflow.snakefile).parent / "scripts")) import run_config # noqa: E402 @@ -148,12 +140,9 @@ if INPUT_TYPE == "image_sims": # --- paths ----------------------------------------------------------------- -# RUN_DIR is the scratch root: bulk intermediates, sized so a batch finishes -# inside the purge window. PRODUCTS_DIR is the persistent root -# for the durable, low-volume products — the final catalogues, the index, the -# report. Snakemake's own state is the one durable-looking thing that stays on -# scratch (bin/sp explains why: tens of thousands of small, hot metadata writes -# against a backed-up, file-count-limited filesystem). +# RUN_DIR holds bulk intermediates on scratch; PRODUCTS_DIR holds durable +# products. INDEX_DB has its own configured path. See bin/sp for placement of +# Snakemake's execution state. INPUTS = config["inputs"] OUTPUTS = config["outputs"] RUN_DIR = Path(OUTPUTS["run_dir"]) @@ -162,16 +151,12 @@ RUN_DIR = Path(OUTPUTS["run_dir"]) # false; refuse_one_root_cleaners enforces it. PRODUCTS_DIR = Path(OUTPUTS.get("products_dir") or RUN_DIR) INDEX_DB = Path(OUTPUTS["index_db"]) -# The campaign's NAME is the run config's `run:` — the same name `$run` expands -# to in the paths above. The two campaign-level merges label their output with -# it (`final_cat_.hdf5` and the group inside it holding the per-tile -# datasets, `full_starcat_.hdf5`). REQUIRED in run_config.py, so an unset -# `run:` has already refused above. +# One campaign name for all merged products and their internal groups. +# See workflow/CONTRACTS: campaign-name-is-run. CAMPAIGN = config["run"] SCRIPTS = Path(workflow.basedir) / "scripts" -# The config chain is the repo's committed directory (D2). The configs and -# rules that set their environment variables must be versioned together. There -# is no separate `config_src` setting; input_type picks between the two dirs. +# input_type selects the committed config chain. Version the configs together +# with the rules that supply their environment variables. CONFIG_DIR = Path(workflow.basedir) / "config" / { "data": "cfis", "image_sims": "cfis_image_sims"}[INPUT_TYPE] # `sp run`'s code snapshot (bin/sp), a sibling of workflow.basedir inside @@ -197,17 +182,11 @@ try: except ValueError as err: raise WorkflowError(str(err)) from None -# SP_PHASE is set by bin/sp and NOWHERE else: `prepare`/`compute` on the two -# invocations of `sp run`, `passthrough` on direct commands (`sp --unlock`, `sp -# --dag`, `sp exp_psf ...`). It gates the two parse-time side effects — the index -# build and the report hooks — so that a passthrough parse never mutates durable -# state or dies on a threshold it was not asked about. -# -# Requiring it to be SET is the point of the check below: a bare `snakemake` -# would otherwise parse as a passthrough, find `rule all` gated on an index it -# never built, and exit 0 on an empty DAG. Jobs re-parse this file under the -# slurm executor and inherit the head process's environment, so a submitted job -# always carries the launching phase. +# bin/sp exports prepare/compute for `sp run`, or passthrough for direct +# commands such as `sp --unlock`. Only compute builds the index and installs +# report hooks. Requiring an explicit phase prevents a bare Snakemake invocation +# from succeeding with an empty DAG and an unbuilt index. Submitted jobs inherit +# the launching phase when the SLURM executor re-parses this file. PHASE = os.environ.get("SP_PHASE", "") if not PHASE: raise WorkflowError( @@ -218,12 +197,8 @@ if PHASE not in ("prepare", "compute", "passthrough"): raise WorkflowError( f"SP_PHASE={PHASE!r} is not one of prepare, compute, passthrough.") -# DEDUPED, order preserved. The tile list is appended to by hand across a -# campaign, so a tile can appear twice — harmless for a per-tile target, which -# is the same path requested twice, but not for the campaign-level merges: their -# fingerprint counts what is in this list and the job derives a deduped set from -# the same file (build_index.campaign_tiles), so a duplicate line would make the -# two disagree about a set they must name identically. +# Deduplicate while preserving order, matching build_index.campaign_tiles so +# merge fingerprints and job-time discovery count the same tiles. with open(config["tile_list"]) as f: TILES = list(dict.fromkeys(ln.strip() for ln in f if ln.strip())) @@ -241,21 +216,14 @@ scattergather: NGMIX_CHUNKS = workflow._scatter["ngmix"] # --- parse-time index build + load (D1) ------------------------------------ -# The COMPUTE invocation's parse IS the index build, and it runs UNCONDITIONALLY -# there — no "some Fe output exists" guard. That guard used to make a -# totally-failed prepare produce an empty index, an empty DAG and a green exit 0; -# with it gone, zero Fe outputs means a missing fraction of 1.0, which trips the -# SP_MISSING_THRESHOLD gate. -# -# In every other phase (prepare, or unset for a passthrough invocation) the parse -# builds NOTHING and only loads whatever index is already on disk. +# The compute head process builds the index unconditionally: zero Fe outputs +# gives a missing fraction of 1.0 and trips the configured missing threshold +# unless it allows complete failure. The ready-tile check below also rejects an +# empty compute DAG. Prepare and passthrough only read an existing index. # -# is_main_process matters as much as PHASE: the SLURM executor re-invokes -# snakemake inside every job, that re-invocation parses this file again, and it -# inherits SP_PHASE=compute from the submitting environment. Without the guard, -# every one of the run's jobs re-runs the build — hundreds of concurrent sqlite -# writers on Lustre, which previously caused "database is locked" errors -# (2026-07-31). Job parses only LOAD the index below. +# Submitted jobs inherit SP_PHASE=compute but must not rebuild the index: +# hundreds of concurrent SQLite writers on Lustre fail with "database is locked". +# workflow.is_main_process keeps the build in the head process. if PHASE == "compute" and workflow.is_main_process: build_index.build( TILES, RUN_DIR, INDEX_DB, @@ -264,15 +232,10 @@ if PHASE == "compute" and workflow.is_main_process: # EXP: exposure base-id -> original name (2605805 -> 2605805p; the name goes # verbatim into the fabricated per-unit exp_numbers list so get_images matches # .fits.fz in the store). TILE_EXP: tile -> [exp_ids]. -# EXP_TILES is the inverse edge — the CAMPAIGN-WIDE consumer set clean_exposure -# is keyed on (D5). -# -# SLURPED ONLY IN THE HEAD PROCESS. The executor re-parses this file inside every -# job (~800 per run), and each of those parses needs at most its own unit's edges -# — reading the whole campaign's tile_exposures table there is pure waste that -# grows with the campaign rather than with the job. Job parses get the same three -# lookups backed by memoised single-row queries instead (the table's primary key -# indexes both), so every input function returns exactly what it would have. +# EXP_TILES is the campaign-wide consumer set used by clean_exposure (D5). +# Load the full index only in the head process; job parses use cached queries +# for their own units. The primary key indexes tile lookups; exposure-consumer +# lookups scan the table and are needed only by head-process cleanup. EXP, TILE_EXP, EXP_TILES = {}, {}, {} if INDEX_DB.exists() and workflow.is_main_process: # timeout=60: Lustre lock handoffs are slow; the default 5 s trips on @@ -292,7 +255,7 @@ if workflow.is_main_process: return EXP[exp] def tile_exposures(tile): - """This tile's exposure base-ids, sorted as the index stores them.""" + """This tile's exposure base-ids in table-scan order.""" return TILE_EXP.get(tile, []) def exp_consumers(exp): @@ -316,9 +279,7 @@ else: def tile_exposures(tile): if _JOB_CON is None: return [] - # ORDER BY rowid, not by exp_id: the eager slurp above is a table scan, - # so its lists are in INSERTION order, and the two paths must hand the - # DAG the same list and not merely the same set. + # Match the head process's table-scan order, not exposure-id order. return [r[0] for r in _JOB_CON.execute( "SELECT exp_id FROM tile_exposures WHERE tile_id = ? ORDER BY rowid", (tile,))] @@ -384,47 +345,35 @@ def exp_manifest(exp, stage): return f"{exp_dir(exp)}/manifests/{stage}.json" def prod_exp_dir(exp): - """The exposure's dir on the PERSISTENT root — where exp_persist writes. - - Sharded identically to the scratch one, so the two trees read as the same - campaign seen from two filesystems, exposure side as well as tile side.""" + """The exposure's persistent directory, with the same shard as on scratch.""" return f"{PRODUCTS_DIR}/exp/{exp[:2]}/{exp}" def prod_exp_manifest(exp, stage): - """A manifest that must SURVIVE reclamation, so it is not in the exposure's - scratch manifests/ dir (clean_exposure deletes that wholesale).""" + """A manifest outside the scratch tree deleted by clean_exposure.""" return f"{prod_exp_dir(exp)}/manifests/{stage}.json" def forest_dir(tile): return f"{tile_dir(tile)}/exp_forest" def final_cat(tile): - """The campaign's science product, and its tile-finished marker. - - It lives on the PERSISTENT root, not the scratch one, and both halves of - that sentence are load-bearing. As a product: it is what the campaign is - for, and a 60-day purge must not eat it. As a marker: tile_finished() keys - on this path to cut a finished tile's exposure edges (D5), so if a purge - could remove it, finished tiles would re-declare inputs against exposure - stores that reclamation deleted — the rerun avalanche the cut exists to - prevent, arriving by way of the purge instead. + """The tile's durable science product and tile_finished() marker. + + Keep it outside scratch: losing the marker to a purge would reconnect a + finished tile to reclaimed exposure stores. See rules/tile.smk's + tile_finished() for the dependency cut (D5). """ return f"{PRODUCTS_DIR}/tiles/{tile[:2]}/{tile}/final_cat-{tile}.hdf5" def unit_num(unit): """$SP_UNIT_NUM: ShapePipe's image-number convention, dot -> dash, leading dash (tile ``210.282`` -> ``-210-282``; exposure ``2605805`` -> ``-2605805``). - The RULES do this transform; the configs just interpolate $SP_UNIT_NUM into - NUMBER_LIST (the `set_config_number_list` mechanism that replaced the retired - -e/--exclusive flag, #746).""" + The rules transform the number; configs interpolate $SP_UNIT_NUM into + NUMBER_LIST via set_config_number_list.""" return "-" + unit.replace(".", "-") -# Content hash of completeness.py, computed once at parse time and carried as a -# param on every rule: the default rerun-triggers' `code` trigger hashes only the -# rule's own shell string, NOT external scripts it calls — without this, a fix to -# the count table silently leaves stale manifests in place (bitten live). Scoped -# to completeness.py alone, the one script every shell line runs; build_forest.py -# gets its own hash, on the forest rule only. +# External scripts are not covered by the rule's shell-code rerun trigger. +# Carry their content hashes in params so script edits invalidate their callers; +# completeness.py is shared by the ShapePipe stage rules. def script_hash(name): """The 12-hex fingerprint of one script under workflow/scripts/.""" return path_hash(SCRIPTS / name) @@ -433,21 +382,13 @@ def script_hash(name): def path_hash(path): """The 12-hex fingerprint of any file the rules depend on but do not own. - A rule whose behaviour comes from more than its own script needs all of it - in one trigger. final_cat_merge is the case: what it writes is decided by - scripts/python/create_final_cat.py (the column extraction) and by - CONFIG_DIR's final_cat.param (which columns), and NEITHER is under - workflow/scripts/ nor a declared input. Without them in the hash, this PR's - own edits to both would have left every finished campaign's hdf5 untouched - and nothing would have said so. - - A MISSING FILE IS NOT A PARSE ERROR. This runs at module level, so raising - here kills EVERY invocation of the workflow — `sp --unlock`, `sp report`, - a dry run — over a file only one rule needs. Worse, it kills them with a - bare FileNotFoundError, which is exactly the diagnosis merge_final_cat.py - already carries and would print if the job were allowed to reach it. So the - hash degrades to a sentinel and says so once; the parse survives, the rule - still exists, and the job fails with the message written for it. + Include external implementation and configuration files in params hashes: + final_cat_merge depends on create_final_cat.py and final_cat.param as well + as its workflow script. + + An unreadable file returns a sentinel and warns in the head process rather + than blocking every workflow invocation at parse time. The job that needs + the file supplies its own diagnostic when it runs. """ path = Path(path) try: @@ -475,40 +416,23 @@ MERGE_FINAL_HASH = ":".join(( path_hash(Path(workflow.basedir).parent / "scripts" / "python" / "create_final_cat.py"), path_hash(CONFIG_DIR / "final_cat.param"))) -# ngmix_range.py earns a hash for a stronger reason than the others. What it -# emits is not a stale RESULT but a stale BOUNDARY, and a tile's eight chunks are -# a PARTITION of its catalogue rows: resume a tile across an edit to the split and -# the chunks that already succeeded keep the old ranges while the reruns take the -# new ones, so within one tile some objects are measured twice and others by -# nobody. merge_sep_cats concatenates whatever it is handed, so the tile -# completes green with a corrupt catalogue and no error anywhere. -# -# ACROSS TIME IS ALL THIS GUARDS NOW. The within-group version of the same -# corruption — eight sibling chunks each computing the split and disagreeing — is -# structurally closed: the partition is materialised ONCE per tile, by -# tile_vignets, and the chunks only look their row up (tile.smk's -# TILE_NGMIX_RANGES, which also keeps the incident transcript). So this hash buys -# REPRODUCIBILITY of the file across attempts and resumes, not agreement between -# siblings. It therefore fingerprints the script that PRODUCES the file, and -# is used by both tile_vignets and tile_ngmix because either rule can produce -# the file. -# -# Land this hash, and any later edit to the split, at a campaign boundary on a -# fresh root: a resume across it re-measures the tile, and the failure modes a -# mid-campaign params change can reach are catalogued at tile.smk's range_hash. +# Chunk ranges must remain one partition across attempts: mixing boundaries +# can omit objects or measure them twice without a merge error. tile_vignets +# writes the partition once; tile_ngmix only reads its row. Both carry this +# hash so a splitter edit also schedules the producer, not just its readers. +# See rules/tile.smk's TILE_NGMIX_RANGES and range_hash notes. +# Change params only at a campaign boundary on a fresh root; see +# workflow/CONTRACTS: unit-pre-changes-at-campaign-boundary. NGMIX_RANGE_HASH = script_hash("ngmix_range.py") # --- exposure reclamation (D5, S5) ----------------------------------------- def flag(value, default=False): - """Truthiness for a config value that may arrive as a STRING. + """Parse boolean config values, including strings from --config. - `--config clean=false` delivers the string "false", and every non-empty - string is truthy in Python — a plain bool() read that as ON and scheduled - the deletions the user had just switched off. YAML booleans pass through - unchanged; only strings are parsed, and an unparseable one is an error, not - a guess. + Python treats the non-empty string "false" as true, so strings need explicit + parsing to avoid enabling deletions. Reject unrecognised strings. """ if value is None: return default @@ -522,19 +446,13 @@ def flag(value, default=False): return bool(value) -# `clean:` in config.yaml gates the whole mechanism. Off => the rule generates no -# jobs at all (nothing requests a tombstone); flipping it on later reclaims -# RETROACTIVELY, because the exposures already cleaned are exactly the ones with -# a tombstone, so the missing tombstones schedule exactly the clean jobs. -# Only ever active under SP_PHASE=compute: the prepare and passthrough parses -# read no index of their own and must schedule no deletions. +# Request cleanup tombstones only when clean is enabled in the compute phase. +# Enabling it also requests cleanup for eligible exposures from earlier runs. CLEAN = flag(config.get("clean", False)) and PHASE == "compute" -# Reclamation REQUIRES the `input` rerun-trigger to be off (see tile.smk's -# tile_finished cascade note). The gate is structural rather than asserted: CLEAN -# needs SP_PHASE=compute, only bin/sp sets it, and bin/sp always launches with -# profiles/nibi — so the incompatible combination is unreachable. A runtime -# assertion is impossible (workflow.dag_settings is None until the DAG is built). +# Reclamation requires the `input` rerun trigger to be off; see rules/tile.smk's +# tile_finished note and the profiles selected by bin/sp. DAG settings are not +# available at this parse-time site, so the requirement is not asserted here. # Tiles that must not pin an exposure's store. See config.yaml: a permanently # failed tile otherwise holds every exposure it touches (~80) forever, because @@ -544,8 +462,10 @@ CLEAN_IGNORE_TILES = set(config.get("clean_ignore_tiles") or []) def tombstone(exp): - """The clean_exposure output. Lives BESIDE manifests/ and logs/, not inside - either: the clean job deletes both wholesale, and `sp report` scans it.""" + """The clean_exposure output, outside the directories cleanup deletes. + + sp report reads this tombstone after manifests/ and logs/ are removed. + """ return f"{exp_dir(exp)}/cleaned.json" @@ -557,25 +477,15 @@ def clean_consumers(exp): def clean_targets(): - """Which exposures this invocation may clean. - - An exposure is eligible only when every consuming tile is either in this - run's scope or has already produced its vignets on disk. Without that test, - requesting a tombstone for an exposure shared with a LATER batch would drag - that batch's whole tile chain into this DAG through the clean rule's input — - cleanup would otherwise expand the scope. Ineligible - exposures are simply skipped; the invocation that finishes their last - consumer picks them up. Deferral, never loss. - - Consumer sets are the IGNORE-FILTERED ones (clean_consumers), so a tile in - `clean_ignore_tiles` neither gates eligibility nor appears in the job's - input — which is the purpose of that list. - - HEAD PROCESS ONLY. This feeds `rule all` at module level, so it runs on every - parse — including the ~800 per-job re-parses under the slurm executor, none of - which can ever schedule `all`. Each of those was stat()ing the whole - campaign's tile_vignets manifests for nothing. The empty list a job parse - returns costs it exactly the target it could not have used anyway. + """Cleanup targets that do not pull out-of-scope tiles into this DAG. + + Every non-ignored consumer must be ready in this invocation or already have + its vignets manifest. Otherwise defer the exposure to an invocation that + finishes its remaining consumers. clean_ignore_tiles is filtered by + clean_consumers for both this test and the clean rule's inputs. + + Only the head process schedules all; job parses skip this campaign-wide + walk and its filesystem checks. """ if not CLEAN or not workflow.is_main_process: return [] @@ -583,9 +493,7 @@ def clean_targets(): for exp, raw in EXP_TILES.items(): if not raw: continue - # An empty set AFTER filtering means every consumer is ignored: nothing - # is left that could ever read this exposure, so it is eligible now. - # all() of an empty set is True, so this case is eligible. + # All consumers ignored: all([]) makes the exposure eligible. tiles = clean_consumers(exp) if all(t in READY_SET or Path(tile_manifest(t, "tile_vignets")).exists() for t in tiles): @@ -593,15 +501,9 @@ def clean_targets(): return sorted(out) # --- persisted exposure products (D5) -------------------------------------- -# The keep list is config, not a rule input, and it is READ HERE so that exactly -# one place converts it into the form the rule carries. -# -# OPTIONAL RETENTION, and only that. What star_cat_merge needs — every CCD's -# psf_validation — is packed by exp_persist whatever this list says -# (persist_exp.py's ALWAYS argues why: provenance for the merged catalogue, and -# a cheap tile append later). So an EMPTY list is a coherent instruction and not -# a switch that turns persistence off: the tar then holds the star catalogue's -# inputs and nothing else, and exp_persist still runs for every exposure. +# This list controls optional retention, not whether exp_persist runs. +# An empty list still retains the required star-catalogue inputs; see +# scripts/persist_exp.py's ALWAYS for the retention policy. PERSIST_EXP = list(config.get("persist_exp") or []) # Whether exposures have PSF products to persist at all. psf_model=fake (the @@ -617,11 +519,9 @@ PERSISTS_PSF = PSF_MODEL != "fake" def refuse_unpersistable_psf(psf_model): """Refuse a PSF model whose products the persistence path cannot read. - exp_persist packs PSFEx products (persist_exp.py maps psf_model to - *.psf) and star_cat_merge reads PSFEx's per-CCD validation table in HDU 2 - (merge_star_cat.py). MCCD writes fitted_model-.npy and one - exposure-wide table in HDU 1, so an MCCD campaign would run every exposure - and then fail at star_cat_merge. Refusing at parse time costs nothing. + See persist_exp.py for supported product patterns and merge_star_cat.py for + the per-CCD PSFEx validation reader. MCCD's exposure-wide products require a + different persistence table and reader, so reject that model before jobs run. """ if psf_model == "mccd": raise WorkflowError( @@ -633,12 +533,8 @@ def refuse_unpersistable_psf(psf_model): refuse_unpersistable_psf(PSF_MODEL) -# The keep list names PRODUCTS (`psf_model`), not globs (`*.psf`); the -# catalogue that maps one to the other lives in persist_exp.py, which is also -# what the rule runs, so there is one definition and not a copy here. -# UNKNOWN NAMES DIE AT PARSE TIME, listing the valid ones — a typo in a keep -# list would otherwise be a silently-empty keep or a per-exposure failure an -# hour into a campaign. +# Resolve keep entries through persist_exp.py's product catalogue. Reject +# unknown entries at parse time rather than failing once per exposure. import persist_exp as _persist # noqa: E402 for _entry in PERSIST_EXP: @@ -649,24 +545,11 @@ for _entry in PERSIST_EXP: def persist_targets(): - """Which exposures this invocation must pack PSF products off scratch for. - - `rule all` requests these DIRECTLY rather than reaching them only through - clean_exposure. Persistence and reclamation are different concerns — the - /scratch purge takes the store whether or not `clean:` is on — and hanging - the copy off the clean rule alone would mean a campaign run with clean:false - persists nothing and loses everything at the purge. + """Head-process persistence targets for the ready tiles' live exposures. - Scope is the ready tiles' exposures, which `all` already builds through the - tile chain, so nothing new is pulled into the DAG by asking. - - EXCEPT AN EXPOSURE WHOSE STORE IS GONE. Its exp_psf manifest is not there, - so requesting its persist manifest would make the DAG rebuild the whole - exposure chain from VOS — the avalanche tile.smk's reclaimed-edge cut exists - to prevent, arriving through a new target instead. exp_store_reclaimed() - below is that test, and what it does NOT test is the tombstone. - - HEAD PROCESS ONLY, for the same reason as clean_targets() above. + all requests persistence independently of cleanup because a scratch purge + can remove stores even with clean disabled. Reclaimed stores are excluded + by exp_store_reclaimed() to avoid rebuilding their exposure chains. """ if not workflow.is_main_process: return [] @@ -675,15 +558,10 @@ def persist_targets(): @functools.lru_cache(maxsize=1) def persist_manifests(): - """persist_targets() without the head-process guard, memoised. - - The guard on persist_targets() is a cost decision, not a correctness one: - `rule all` reads it at MODULE level, so a job parse would pay a whole - campaign's index walk for a target it can never schedule. star_cat_merge - reads the same list through an INPUT FUNCTION, which snakemake evaluates - only for parses that actually build that job — the head process, and the one - merge job's own re-parse under the slurm executor, which genuinely needs it. - So this half carries no guard and the memo keeps either parse to one walk. + """Memoised persistence manifests, available in head and job parses. + + Unlike persist_targets(), this helper has no head-process guard so an input + function can use it during a submitted job's parse without losing edges. """ return sorted(prod_exp_manifest(e, "exp_persist") for e in psf_exposures() if not exp_store_reclaimed(e)) @@ -699,105 +577,52 @@ def psf_exposures(): def exp_store_reclaimed(exp): - """True when this exposure's PSF products exist ONLY on the persistent root. - - The one condition both the persist target list and star_cat_merge's edge - choice turn on, and it deliberately does NOT read the tombstone. - - THE TOMBSTONE ALONE IS NOT THE EVIDENCE. It says clean_exposure ran, and - clean_exposure is only one of the two ways a scratch store disappears. - - WHAT THE TEST HAS TO SEPARATE is a store that is GONE from one that has not - been BUILT yet, and no single file says that. This runs at parse time, before - any exp_psf job of a fresh campaign has run, so "the exp_psf manifest is - missing" alone would skip every exposure of a new campaign and persist - nothing at all. The question is therefore: is there evidence this exposure - once had a store? Two files carry it, and either will do: - - * the TOMBSTONE — clean_exposure ran, so the store was built and reclaimed, - and whatever was going to be packed was packed before it went; - * a PERSISTED MANIFEST with no exp_psf manifest beside it — exp_persist - ran, so the store existed, and it is not there now. This is the purge - case, and it is the one the tombstone cannot see: /scratch is purged on - a 60-day window whether or not this workflow reclaimed anything, and it - leaves nothing behind. Keying on the tombstone alone meant that after a - purge, or on any campaign run with `clean: false`, every exposure looked - live, its persist manifest was requested, its exp_psf manifest was not - there, and snakemake rebuilt the entire exposure chain from VOS. - - An exposure with a LIVE store and a manifest is not reclaimed and is still - asked for, which is what lets an edit to `persist_exp:` re-pack in seconds - rather than be silently ignored — the whole reason exp_persist is a rule of - its own. An exposure with neither file is asked for too: either it has not - run yet, or it was purged having saved nothing, and only the DAG can tell - those apart by trying. + """True when there is evidence that a built scratch store is reclaimed. + + A cleanup tombstone is sufficient. A persisted manifest with no scratch + exp_psf manifest also detects a purge, which leaves no tombstone. Missing + exp_psf alone is insufficient: an unbuilt exposure also lacks it. + + Live stores remain eligible for repacking when persist_exp changes. With + neither tombstone nor persisted manifest, the DAG must try to build the + exposure; this test cannot distinguish an unbuilt store from a purged, + unsaved one. It does not verify that the persisted products still exist. """ return (Path(tombstone(exp)).exists() or (Path(prod_exp_manifest(exp, "exp_persist")).exists() and not Path(exp_manifest(exp, "exp_psf")).exists())) # --- the campaign-level merges --------------------------------------------- -# Two rules, one job each per campaign, both writing to the persistent root, and -# both the LAST link of a chain whose per-unit half the workflow already had: -# the exposure side ends in one `full_starcat_.hdf5` (every CCD's PSF -# validation catalogue — the rho/tau statistics input) and the tile side in one -# `final_cat_.hdf5` (every tile's final catalogue — the shear -# catalogue sp_validation reads). Until they existed the workflow's product set -# was two files short of what the old `combine_runs.bash` + `create_final_cat.py` -# chain delivered, and every campaign ended with a manual merge. -# -# NEITHER IS A LOCALRULE, and the arithmetic runs the opposite way from -# exp_persist's. Those rules are ~20k jobs of seconds each, so submitting them -# costs more in scheduling latency than the work; these are ONE job each over the -# whole campaign — ~800k catalogues stacked in memory, or ~20k catalogues read -# end to end at DR6 scale. That is a compute job, and it belongs on a node. +# One submitted job per campaign writes full_starcat_.hdf5 (PSF validation +# for rho/tau statistics); another writes final_cat_.hdf5 (shear catalogue). +# Both stream per-unit inputs to the persistent root. Campaign-wide I/O belongs +# on a compute node, unlike the short per-exposure persistence jobs. # -# NEITHER PUTS ITS INPUT PATHS IN ITS SHELL. `{input}` at DR6 scale is ~20k paths -# in a single argv entry, an order of magnitude over Linux's 128 KiB -# MAX_ARG_STRLEN, and the job would die on exec. So each rule's `input` is the -# DAG EDGE (what must exist first) and each script rediscovers the same set from -# the tile list and the index; what travels is a FINGERPRINT of that set's unit -# ids, on `params`, which is what makes the merge rerun when the set changes and -# not otherwise. -# The scripts' docstrings argue the rediscovery — it is also what lets the merges -# cover exposures whose scratch stores reclamation has since taken. +# Input paths order the DAG but do not travel in shell arguments: ~20k paths +# can exceed Linux's 128 KiB MAX_ARG_STRLEN limit for one argument. Each merge +# script discovers its inputs from the tile list and index; see its docstring. +# A params fingerprint of unit IDs detects changes to the set without reacting +# to manifest-to-product edge changes after reclamation. def unit_fingerprint(units): - """A short digest of a set of UNIT IDS, for a merge rule's `params`. - - `params` is a rerun trigger and a set of ids is not: appending a tile grows - the set, moves the digest and reruns the merge, while a rerun over the same - set leaves it alone. Sorted before hashing because the ORDER is not part of - what changed. - - IDS RATHER THAN THE RULE'S `input` PATHS. A path can change while the set - does not — star_cat_merge's edge for one exposure flips from its manifest to - its tar when the store is reclaimed — and a merge that reruns over identical - content on every reclamation pass is a rerun trigger firing on bookkeeping. - The ids are also exactly what the job derives on its own side, so both - halves agree on the set and on how it is named. + """Count and digest of unit IDs for a merge rule's params trigger. + + Sort before hashing so order does not matter. Callers supply unique IDs; + this function does not deduplicate them. See the campaign-level merge note + above for why the trigger uses IDs rather than input paths. """ joined = "\n".join(sorted(str(u) for u in units)) return f"{len(units)}:{hashlib.md5(joined.encode()).hexdigest()[:12]}" -# NO GATE ON THE KEEP LIST. star_cat_merge used to exist only when -# `persist_exp:` named something validation_psf-shaped, which made the -# campaign's star catalogue an opt-in and a typo away from silently absent. -# exp_persist now packs psf_validation unconditionally, so the merge is -# requested whenever the campaign has a persisted exposure at all, and -# star_cat_targets() below is the only condition left. - - def full_starcat(): """The campaign's merged star catalogue — the rho/tau statistics input. - hdf5, one dataset per exposure, named for the campaign exactly as the shear - catalogue beside it is. The old flat FITS table it replaces was called - `full_starcat-0000000.fits` and sp_validation still opens that name; - CosmoStat/sp_validation#340 moves its readers to this file, the same - migration that retires the `patches/` key on the galaxy side.""" + HDF5, one dataset per exposure. sp_validation reads it once + CosmoStat/sp_validation#340 lands; until then it opens the flat + full_starcat-0000000.fits. + """ return f"{PRODUCTS_DIR}/full_starcat_{CAMPAIGN}.hdf5" @@ -814,37 +639,17 @@ def prod_exp_tar(exp): @functools.lru_cache(maxsize=1) def star_cat_inputs(): - """What star_cat_merge waits for: every exposure of TILES_READY whose PSF - products are on the persistent root, live and reclaimed alike. - - THE SAME SET merge_star_cat.py derives at job time, and that equality is - load-bearing — the fingerprint on `params` is taken over THIS list, so - anything the job stacked that was not in it would be rows no rerun trigger - could see. The job states the rule from its own side: same tile list, same - index, exp_persist manifest present on the persistent root. By the time it - runs, every exposure below has one. - - RECLAIMED EXPOSURES BELONG IN THE STAR CATALOGUE. Carrying their PSF - products off scratch is exactly what exp_persist is for, and a merge that - dropped them would shrink the campaign's star catalogue every time - reclamation ran. But their exp_psf manifest is gone, so REQUESTING their - exp_persist manifest rebuilds the whole exposure chain from VOS — the - avalanche persist_targets() drops them to avoid. - ancient() DOES NOT HELP: it suppresses the timestamp comparison, not the - missing input, and snakemake schedules the chain anyway. Measured on smk-g6 - with one reclaimed exposure given a manifest by hand: the dry run grew - exp_get_images, exp_split, exp_psf and exp_persist jobs. - - So a reclaimed exposure is depended on through its TAR instead. The tar is - not a declared output of any rule (exp_persist declares only its manifest, - deliberately — persist_exp.py says why), so a tar that exists is a DAG leaf: - snakemake requires it and builds nothing. A live exposure keeps its manifest - edge, which is what orders the merge after the packing; its tar does not - exist yet, so it could not serve as the edge. - - An exposure reclaimed by a workflow PREDATING exp_persist has neither tar nor - manifest and is in no set at all. Nothing short of rebuilding its chain from - VOS recovers it; the merge reports how many exposures it found. + """Star-merge dependencies for live and reclaimed PSF exposures. + + Live stores use exp_persist manifests to order packing before the merge. + Reclaimed stores use existing tars as undeclared DAG leaves; requesting + their manifests would rebuild the exposure chain. ancient() suppresses + timestamp comparisons, not missing-input dependencies: a dry run measured + on a nibi run still scheduled retrieval, splitting, fitting and persistence. + + See durable_edge() for absent-product handling and merge_star_cat.py for + job-time discovery. The params fingerprint uses star_cat_exposures(), + not these dependency paths. """ return [p for exp in psf_exposures() for p in durable_edge(exp, "exp_persist", prod_exp_tar(exp))] @@ -863,15 +668,11 @@ def durable_edge(exp, stage, product): @functools.lru_cache(maxsize=1) def star_cat_exposures(): - """The exposure IDs star_cat_merge stacks — what its fingerprint is taken - over. - - THE IDS, NOT THE PATHS, and the difference is a rerun. An exposure's edge - FLIPS from its manifest to its tar the moment its store is reclaimed, so a - fingerprint over paths moves on every reclamation pass and reruns the merge - over content that did not change. The ids move only when the set does, which - is what the trigger is for. It is also what merge_star_cat.py derives on the - job side, so the two agree on the set AND on how it is named. + """Exposure IDs for the star merge's rerun fingerprint. + + Include live exposures whose persistence jobs can still run, and reclaimed + exposures with persisted manifests. See the campaign-level merge note for + why the fingerprint excludes dependency paths. """ return [e for e in psf_exposures() if Path(prod_exp_manifest(e, "exp_persist")).exists() @@ -922,62 +723,24 @@ def exposure_maps_targets(): # --- sizing the two merges (D4) --------------------------------------------- -# MEASURED, not guessed, and measured as a SLOPE rather than a single number: -# these are the only two rules whose one job's footprint grows with the whole -# campaign, so a constant is wrong by however much the campaign is not the one -# it was tuned on. -# -# Both slopes were measured on this login node, inside the campaign container, -# against synthetic tars for the star side and against smk-g6's real -# catalogues for the tile side. Peak RSS is getrusage(RUSAGE_CHILDREN). -# -# STAR SIDE, AND IT IS FLAT IN THE CAMPAIGN. The merge writes one hdf5 dataset -# per exposure and reads one exposure at a time, so it is sized on the LARGEST -# exposure's members — ~2 MB — not on the campaign's. What follows is the -# history of how that came to be true, because the numbers are the argument. -# -# The FITS full_starcat this replaced was one flat table, so the job held the -# whole campaign. Two fixture points, 20 and 80 exposures of 40 CCDs x 400 stars -# (1.6 MB of members per exposure, against the 2.0 MB measured on smk-m2), -# across the rewrites this PR made to MergeStarCatPSFEX: -# -# input members python lists arrays+concat two passes -# 32.3 MB 383 MB 238 MB 221 MB -# 129.0 MB 1313 MB 740 MB 661 MB -# slope 10.1x 5.5x 4.8x -# -# The tenfold was one python float object (32 bytes) plus a list pointer (8) per -# 4 bytes of float32 payload. Arrays per catalogue removed that; counting rows -# from the headers and filling a preallocated array removed the rest. What -# remained at 4.8x was the OUTPUT: file_io writes every float column as FITS 1D, -# so float32 became a float64 table astropy then buffered. -# -# Per-exposure hdf5 removes the term entirely rather than shrinking it — and -# with it the ~240 GB a DR6-scale flat table would have wanted. Those -# improvements stay upstream regardless: the module runner still merges to one -# FITS table, and they are its fix. -# -# TILE SIDE, and it is the reassuring one. Two points against real smk-g6 -# catalogues, 2 tiles (73.9 MB in, largest 39.6 MB) and 6 tiles (235.5 MB in, -# largest 47.7 MB): peak RSS 129 MB and 139 MB. FLAT IN THE NUMBER OF TILES — -# the merge holds one catalogue at a time — so it is sized on the LARGEST tile, -# not the total, at ~3x it plus the interpreter. -# THE CEILING ON ANY REQUEST, and it is not a formatting nicety: a mem_mb above -# the partition maximum is a job SLURM will never schedule and snakemake will -# never diagnose — it sits PENDING with a reason nobody reads while the campaign -# looks alive. The two merge formulas grow with the campaign, so at some size -# they WILL cross it; capping turns "silently never runs" into "runs on the -# biggest node there is, and possibly dies with a diagnosable OOM". +# Both merges stream one unit at a time, so size memory from the largest unit, +# not the campaign total. The star side reads ~2 MB of validation members per +# exposure; STAR_MEM_FACTOR reserves six times that payload plus the base. +# For the tile side, measurements on a nibi run inside the campaign container +# give peak RSS (getrusage(RUSAGE_CHILDREN)) of 129 MB for 2 tiles (73.9 MB total, +# 39.6 MB largest) and 139 MB for 6 tiles (235.5 MB total, 47.7 MB largest). +# This supports ~3x the largest tile; FINAL_MEM_FACTOR reserves 4x plus the base. # -# Nibi's standard compute node is 766 GB (192 cores, 4 GB/core); 750000 leaves -# room for the OS and the slurm accounting overhead. Override with `max_mem_mb:` -# for a cluster with smaller nodes, or to reserve headroom. +# Cap requests at a schedulable node size: over-limit SLURM jobs can stay pending +# indefinitely. A capped job may OOM, but that failure is diagnosable. +# Nibi's standard node has 766 GB; 750000 MB leaves OS and accounting headroom. +# Set max_mem_mb for smaller nodes or additional headroom. MAX_MEM_MB = int(config.get("max_mem_mb", 750_000)) _capped_warned = set() def capped_mem(mb, rule): - """min(mb, MAX_MEM_MB), and say so ONCE at parse time when it bites.""" + """Cap memory at MAX_MEM_MB and warn once per rule in the head process.""" mb = int(mb) if mb > MAX_MEM_MB: if rule not in _capped_warned and workflow.is_main_process: @@ -993,14 +756,12 @@ def capped_mem(mb, rule): STAR_MEM_BASE_MB = 500 # interpreter + astropy + h5py, rounded up -STAR_MEM_FACTOR = 6 # x the LARGEST exposure's members +STAR_MEM_FACTOR = 6 # x the largest exposure's members FINAL_MEM_BASE_MB = 800 -FINAL_MEM_FACTOR = 4 # x the LARGEST tile; ~3 measured -# What one unit costs when its product is not on disk yet to be measured — a -# fresh campaign sizes its merge before anything has been packed or made. The -# exposure figure is psf_validation's alone (the only members the star merge -# reads), not a whole tar's; both are the measured medians in config.yaml's -# persist_exp block and the D5 notes. +FINAL_MEM_FACTOR = 4 # x the largest tile; ~3 measured +# Fallback payload sizes before products exist: validation members only for +# exposures, final catalogues for tiles. See config.yaml's persist_exp notes +# and the D5 measurements in CosmoStat/shapepipe#848. EXP_BYTES_DEFAULT = 2_000_000 # The product whose members star_cat_merge stacks — named once, here and in # merge_star_cat.py, and resolved through persist_exp.py's catalogue. @@ -1018,32 +779,17 @@ def _size(path, default): def star_cat_max_bytes(): - """The LARGEST exposure's psf_validation members — what sizes the merge. - - The star merge holds ONE exposure at a time now that its output is hdf5 - with a dataset per exposure, so its memory is flat in the campaign exactly - as the tile side's is. Sizing on the total would ask a node for a campaign's - worth of memory to hold ~2 MB. - """ + """Largest exposure's validation payload, or the default for an empty set.""" return max(_star_cat_exposure_bytes() or [EXP_BYTES_DEFAULT]) def star_cat_bytes(): - """Total bytes of the members the star merge will actually read. - - THE TAR'S SIZE IS THE WRONG NUMBER, and increasingly wrong as the keep list - grows: the merge reads the psf_validation members and nothing else, while - the tar also holds whatever `persist_exp:` retains. With the default - retention that is 2.4x too much, and with the star_* products on it is ~36x - — a memory request that misses by more than an order of magnitude, and one - that would jump the moment an exposure got packed, since an unpacked one - contributed the per-exposure default instead. So the MANIFEST is read and - only the psf_validation members are counted; persist_exp records the product - each member came from, exactly so this is answerable without opening a tar. - - One json parse per exposure at DAG build, and only for the parse that builds - this job. An exposure not yet packed has no manifest and contributes the - measured default, which is the psf_validation figure and not the tar's. + """Total validation-member bytes, using defaults for unreadable manifests. + + Count members through persistence manifests, not tar sizes: optional + retention inflates the tar to 2.4x the validation payload with the default + keep list, or ~36x with star_* products. These bytes describe I/O volume; + star_cat_max_bytes() supplies the largest-unit memory estimate. """ return sum(_star_cat_exposure_bytes()) @@ -1059,10 +805,8 @@ def _star_cat_exposure_bytes(): continue try: body = json.loads(manifest.read_text()) - # By product name or, for a manifest written before that field - # existed or by a raw-glob keep list, by file name — the same test - # merge_star_cat.is_member() applies, so the sizing counts exactly - # the members the job will read. + # Match by product or filename, including manifests without a + # product field. See merge_star_cat.is_member() for the reader test. out.append(sum(f["bytes"] for f in body["files"] if f.get("product") == STAR_CAT_PRODUCT or fnmatch.fnmatch(f["name"], STAR_CAT_PATTERN))) @@ -1072,7 +816,7 @@ def _star_cat_exposure_bytes(): def final_cat_max_bytes(): - """The LARGEST tile catalogue the hdf5 merge will read — what sizes it.""" + """Largest tile catalogue size, with a fallback for unavailable products.""" return max([_size(final_cat(t), TILE_BYTES_DEFAULT) for t in TILES_READY] or [TILE_BYTES_DEFAULT]) @@ -1080,12 +824,9 @@ def final_cat_max_bytes(): def star_cat_targets(): """`full_starcat` when there is anything to stack into it, else nothing. - One way to get nothing, and it is a state rather than an error: every - exposure in scope is already tombstoned — a - campaign resumed after reclamation, whose exposures were cleaned by a - workflow that predates exp_persist and therefore left neither tar nor - manifest to read. A rule with an empty input list would still be a JOB, and - it would write an empty star catalogue over a good one. + Skip the merge when no dependencies are available, including fake-PSF + runs and reclaimed exposures without tars. An empty-input merge could + overwrite a good star catalogue with an empty one. """ if not workflow.is_main_process: return [] @@ -1099,13 +840,9 @@ def final_cat_targets(): return [final_cat_hdf5()] # --- tile reclamation (D5) -------------------------------------------------- -# A separate flag from `clean:` (config.yaml carries the full -# argument): exposure reclamation costs nothing but a rebuild if a tile is -# appended later, while tile reclamation destroys the per-tile AUDIT TRAIL that -# the campaign's cost model was derived from. They are not the same decision and -# must not be made by the same switch. Gated on SP_PHASE=compute exactly as -# CLEAN is, and read through flag() for the same reason (`--config -# clean_tiles=false` arrives as the string "false"). +# Tile cleanup has a separate switch because it removes the per-tile audit +# trail; see config.yaml for retention policy. Like CLEAN, it is compute-only +# and parses string booleans through flag(). CLEAN_TILES = flag(config.get("clean_tiles", False)) and PHASE == "compute" @@ -1140,24 +877,13 @@ def tile_tombstone(tile): def clean_tile_targets(): - """Which tiles this invocation may clean: the in-scope ones, and no test. - - No eligibility test, because a tile's store has no consumer outside that - tile (clean_tile.py argues the asymmetry with clean_targets()). Every ready - tile is eligible the moment its own final_cat exists, and the rule's input - is exactly that. - - The restriction to TILES_READY is therefore scope containment, not - eligibility, and it runs the opposite way from the exposure case: an - OUT-OF-SCOPE tile's tombstone would drag that tile's final_cat, its - tile_make_cat job and the whole fused tile_shape group into this DAG, where - the `params` rerun-trigger can find a finished tile out of date and rerun it - against reclaimed exposure stores (see the TILE_LOCAL warning in tile.smk). - In scope the edge is free: `rule all` already requests the same final_cat. - Nothing is lost by skipping an out-of-scope tile — the tile list only ever - grows, so a later invocation has it in scope. - - HEAD PROCESS ONLY, for the same reason as clean_targets() above. + """Head-process cleanup targets restricted to ready, in-scope tiles. + + The clean rule waits on final_cat; no other tile consumes this store. + Requesting out-of-scope tombstones would pull those tiles' shape groups + into the DAG, risking params-triggered reruns against reclaimed exposures. + See scripts/clean_tile.py for the cleanup boundary and rules/tile.smk's + TILE_LOCAL warning for rerun hazards. """ if not CLEAN_TILES or not workflow.is_main_process: return [] @@ -1174,50 +900,24 @@ _THREAD_CAPS = " ".join( def unit_pre(stage, unit, *, exp_name=None, forest=None, env=None, pre_run=()): - """The unit-furniture + environment prologue, as bash. - - Returned as a rule ``params`` value, NEVER inlined into the ``shell:`` - string: snakemake formats a shell string ONCE, so a ``{output}``/``{threads}`` - placeholder inside a params value would survive literally — and, conversely, - the literal ``${SP_NGMIX_CHUNK}`` braces this prologue needs would blow up - that formatting if they lived in the shell string. Params values are - substituted after formatting, so both hazards go away together. - - What it materialises (the proven v2.0 isolation-by-work-dir-content, NOT - -e/--exclusive): - * ``output/``, ``manifests/`` and ``logs/`` — the last two are a SIBLING - pair, not one dir with two naming conventions, because clean_exposure - deletes both wholesale (clean_exposure.py says why). The benchmark tsv - is deliberately outside both: it measures the job, not the store, so - reclamation must not eat it. - * tile: ``tile_numbers.txt`` (dot format — what get_images reads); - * exposure: a fabricated pseudo-Fe ``exp_numbers-000-000.txt`` holding the - ORIGINAL exposure name from the index (``2605805p``), so get_images - matches ``.fits.fz`` in the store — the bare base id matches - nothing. Written UNCONDITIONALLY: an exists-guard once pinned a stale - pre-fix file with the bare id. - There is no per-unit ``cfis`` symlink any more: $SP_CONFIG points straight at - the committed config dir. - - It also exports the configured input roots as ``SP_INPUT_TILES``, - ``SP_INPUT_EXPOSURES`` and (data only) ``SP_INPUT_MASKS``, the retrieve - mode as ``SP_RETRIEVE`` and the PSF choice as ``SP_PSF`` for the committed - ini chain (plus ``PSF_DICT`` for the image-simulation true PSF). - - Every line here is part of each rule's ``params.pre`` and so of the - ``params`` rerun trigger: a line added for every rule reruns every finished - unit of a campaign on its next ``sp run``, so lines are added only at a - campaign boundary. The image-simulation lines are conditional, so a data - prologue does not carry them. - - ``tile_numbers.txt`` carries the number in the form the INPUT files are - named with, because get_images substitutes it verbatim into - INPUT_FILE_PATTERN: dot format for the survey tiles (``CFIS.210.282.r``), - dash format for the simulations (``CFIS_simu_image-210-282``). - - Finally it ``rm -rf``s this stage's own fixed run dir — ShapePipe's - FileHandler raises on an existing run dir, and it is how a rerun never sees - stale products (D2: the job clears its run dir at start). + """Build the unit's directory and environment prologue as bash. + + Return it as params.pre rather than embedding it in shell: Snakemake + substitutes params without recursively formatting their contents, preserving + literal bash braces. Rule placeholders such as {output} belong in sp_shell. + + Create output/, manifests/ and logs/, then rewrite the image-number list: + tiles use dots for survey filenames and dashes for simulation filenames; + exposures use the original indexed name (2605805p), not the base ID. + SP_CONFIG points to the committed config chain. Export input roots, retrieval + and PSF choices, plus any caller-supplied environment and pre-run commands. + + WARNING: emitted lines are params rerun triggers. Change them only at a + campaign boundary on a fresh root; see workflow/CONTRACTS: + unit-pre-changes-at-campaign-boundary. + + Finally remove this stage's fixed run directory so FileHandler can create + it and a rerun cannot read stale stage products (D2). """ # The stage table already knows both halves of "where this stage writes": # its level and its run dir. Taking them from there rather than from an @@ -1264,25 +964,16 @@ def unit_pre(stage, unit, *, exp_name=None, forest=None, env=None, def sp_shell(stage, config_name, *, check_args="", post=""): """The rule's shell string: prologue, one shapepipe_run, one completeness check. - EVERY rule that runs shapepipe_run goes through here, including the two that - need a little more than the plain body — they pass it rather than restating - it, so the rc composition below has exactly one definition: - * ``check_args`` — extra flags for the completeness check. tile_vignets - points it at the NODE-LOCAL run root (``--run-dir "$SP_LOCAL"``) and keeps - the manifest's unit field the tile ID rather than the local root's - basename (``--unit``), which also keeps the manifest content independent - of an ephemeral path the mtime rerun-trigger reads. - * ``post`` — bash between the check and the ``exit``. tile_make_cat - publishes its final_cat there, guarded on ``rc``. - - ``{threads}``, ``{output}`` and ``{log}`` are placeholders HERE and nowhere - else (see unit_pre). ``-b {threads}`` makes SMP fork width and cpus_per_task - one number by construction (D4). - - The rc is CAPTURED rather than ``&&``-ed onto, so the check still runs when - shapepipe_run failed, and it is passed back as ``--job-rc "$rc"`` so the - verdict composes the count checks with shapepipe_run's own exit status. Both - halves are argued in completeness.py's docstring. + check_args supplies extra completeness flags; tile_vignets uses it to check + the node-local root while identifying the manifest by tile ID. post supplies + bash after the check; tile_make_cat publishes final_cat there, guarded on rc. + + Format rule placeholders here, not inside params.pre (see unit_pre). + -b {threads} matches ShapePipe's SMP batch size to the rule's CPU request. + + Capture ShapePipe's exit status so completeness still runs after failure. + Pass it as --job-rc to combine process status with product counts; see + scripts/completeness.py's docstring for the verdict contract. """ return ( "{params.pre}\n" @@ -1306,17 +997,10 @@ include: "rules/tile.smk" # they would be ~20k (clean_exposure) or ~23k (clean_tile) sbatch submissions at # DR6 scale for work shorter than the scheduling latency. # -# Both are DAG LEAVES, so neither constrains a `group:` label. (A mid-chain -# localrule would: a local job cannot be fused into a submitted group. The old -# star-catalogue rules were exactly that, and they are gone with the internal -# mask generation.) -# -# exp_persist joins them for the same arithmetic — one tar of a few MB per -# exposure, ~20k of them at DR6 scale, each far shorter than the scheduling -# latency that would submit it (exposure.smk argues the placement in full). It -# sits mid-chain between exp_psf and clean_exposure, but both of those are -# outside every group already (exp_psf is heavy, clean_exposure is local), so it -# adds no new grouping constraint. +# Both clean rules are terminal jobs outside submitted groups. A local job +# cannot be fused into a submitted group. +# exp_persist is also local because packing a few MB is shorter than submission +# latency. See rules/exposure.smk for its placement outside groups. localrules: all, prepare_all_tiles, clean_exposure, clean_tile, exp_persist @@ -1338,10 +1022,8 @@ rule prepare_all_tiles: [tile_manifest(t, "tile_find_exposures") for t in TILES] # --- report hooks ----------------------------------------------------------- -# run_report is NOT a DAG node (a descendant of every job would be poisoned by -# any hard failure — the exact case it exists for). It is a standalone script, -# emitted automatically at the end of the COMPUTE invocation, runnable any time -# via `sp report`. +# Run the report outside the DAG so job failures cannot block it. Compute-phase +# success and error hooks invoke it; `sp report` also runs it independently. def _report(status): shell(f"SP_TILE_DETECTION='{TILE_DETECTION}' " f"python {SCRIPTS}/run_report.py --run-dir {RUN_DIR} " From efd47039db572032ab101c84f5631f3f9b63d14a Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:46:46 +0200 Subject: [PATCH 22/43] docs(workflow): present-tense comments in exposure.smk Co-Authored-By: GPT-6.1 Sol --- workflow/rules/exposure.smk | 238 ++++++++++-------------------------- 1 file changed, 62 insertions(+), 176 deletions(-) diff --git a/workflow/rules/exposure.smk b/workflow/rules/exposure.smk index 12ad09a6f..ec8159cc8 100644 --- a/workflow/rules/exposure.smk +++ b/workflow/rules/exposure.smk @@ -7,51 +7,31 @@ reads fixed ``$SP_RUN/output/run_sp_exp_*`` INPUT_DIRs, so nothing resolves a run log. There is no `prepare_exposures` aggregation target: these chains hang off the compute DAG (`all` <- final_cat <- tile chain <- exposure manifests). -NO MASK RULE, and that is the design (PR #847). ShapePipe generates no masks. -The only mask that reaches pixels is the instrument flag image delivered with -the exposure, which ``exp_split`` splits per CCD alongside image and weight and -SExtractor reads directly. Sky-fixed masks are healsparse maps, queried once per -object: ``mask_query`` (inside exp_psf's config chain) writes ``MASK_EXT`` onto -each CCD's SExtractor catalogue, carried for transparency and measurement -(selection's only mask cut is ``IMAFLAGS_ISO``; imposing ``MASK_EXT`` is -opt-in, see ``star_selection.setools``), and ``make_cat`` writes the per-band -``MASK_`` columns on the tile side. Neither needs a rule, a -star catalogue, or a network fetch — hence no ``star_catalogue`` / ``exp_star_cat`` -here, and no ``exp_mask``. +There is no mask-generation rule. ``exp_split`` splits the delivered instrument +flag image per CCD alongside image and weight; SExtractor reads it directly. +Sky-fixed masks are queried within the PSF and tile config chains, not fetched +or rasterized by a separate rule. See ``config/cfis/star_selection.setools`` +for star-selection mask cuts and ``config/cfis/config_tile_Mc.ini`` for the +catalogue mask columns. -``exp_maps`` exports that flag image, with the CCD footprints, into a -HealSparse fragment so the survey footprint can subtract it; nothing in this -workflow reads it back. +``exp_persist`` and ``exp_maps`` write to the persistent root. Retention policy +and tar layout are documented in workflow/scripts/persist_exp.py; the exposure +map format is documented in workflow/scripts/exp_maps.py. -``exp_persist`` and ``exp_maps`` write to the PERSISTENT root. ``exp_persist`` -packs the PSF products named by `persist_exp:` into one tar per exposure off -/scratch before the purge (or clean_exposure) can take them. It is a separate -rule from exp_psf precisely so that editing that list costs a re-pack and not a -four-hour refit; the full -argument is in workflow/scripts/persist_exp.py. +NO temp() on exposure products: exposures overlap ~7-10 tiles each, so their +consumer set spans the campaign, not one invocation. ``clean_exposure`` uses +the accumulating index to reclaim them. A temp() would delete an exposure as +soon as this invocation's readers finished, causing destructive reruns of +neighbouring tiles when the tile list grows. -NO temp() anywhere in this file, ever (D5). Exposures overlap tiles by -construction (~7-10 tiles each), so their consumer set closes over the CAMPAIGN, -not over one invocation — reclamation here is clean_exposure's job (S5), driven -by the accumulating index. A temp() here would delete an exposure the moment -this invocation's readers finished and cascade destructive reruns across spatial -neighbours the next time a tile is appended. - -NO GROUPING. The ``exp_short`` group existed to fuse exp_split and exp_mask — -two rules whose medians were 1:28 and 1:54, both well under the 15-minute floor -Alliance policy asks us to bundle away — into one sbatch per exposure. With -exp_mask gone there is nothing to fuse: a group of one rule submits exactly the -job the ungrouped rule submits, and the label would only obscure that. The -composition rules, should a second short rule ever appear here, are in -prepare.smk's docstring. exp_get_images stays separate for the same reason it -always did (a download, retried on its own), and exp_psf is heavy (16 GB, 4 h) -and never fuses with a short rule. +The exposure rules are ungrouped: exp_get_images is a separately retried +download, exp_split has no adjacent short compute rule to fuse with, and +exp_psf is a heavy job (16 GB, 4 h). Group resource composition is documented +in prepare.smk. NUMBER_LIST ($SP_UNIT_NUM, see unit_num in the Snakefile) is set only for -exp_split, whose numbering scheme IS the exposure id; never for get_images / -exp_psf, whose per-CCD or download numbering would turn tolerated per-CCD -attrition into a whole-exposure hard failure. It is a property of the committed -configs (config_exp_Sp.ini alone carries the entry). +exp_split, whose numbering scheme is the exposure id. The implementing config +is config_exp_Sp.ini; get_images and exp_psf use different numbering schemes. """ rule exp_get_images: @@ -90,17 +70,14 @@ rule exp_split: shell: sp_shell("exp_split", "config_exp_Sp.ini") -# SExtractor -> mask_query (MASK_EXT) -> setools star selection -> PSFEx model -# -> psfex_interp, per CCD. Under SP_PSF=psfex, setools_runner is mandatory -# (expect=80) and only psfex_interp_runner is :warn; under SP_PSF=mccd every -# runner in the chain is :warn (see completeness.py's COMPLETENESS table). +# PSFEx: SExtractor -> mask_query -> setools -> PSFEx -> psfex_interp, per CCD. +# For the fake PSF, this stage runs SExtractor only. Failure policy lives in +# completeness.py's COMPLETENESS table. # -# MCCD instead fits ONE focal-plane model per exposure after the per-CCD -# stages: ~85 min single-threaded for ~2500 stars (SKiLLS star sim, 8.3 GB), -# and only 1.75x faster on 8 BLAS threads (which the thread caps forbid anyway). -# With 8 cores reserved the fit would idle 7 of them for its whole length, so -# the mccd chain takes 2: the per-CCD stages run 2 wide (minutes), the fit is -# unchanged, and the exposure reserves a quarter of the core-hours. +# The MCCD branch reserves 2 cores: a measured ~85 min focal-plane fit for +# ~2500 stars uses one thread (8.3 GB), with only 1.75x speedup on 8 BLAS threads. +# The per-CCD stages run 2 wide without reserving 8 cores for the serial fit. +# MCCD campaigns are refused by Snakefile::refuse_unpersistable_psf. rule exp_psf: input: rules.exp_split.output.manifest @@ -114,10 +91,8 @@ rule exp_psf: threads: 2 if PSF_MODEL == "mccd" else 8 retries: 2 benchmark: - # BESIDE manifests/, not inside it: clean_exposure deletes manifests/ - # wholesale, and this tsv is the measured-memory feed for mem_mb sizing - # (D4). Inside manifests/ it died with the first reclamation and took - # the campaign's only record of exp_psf's real footprint with it. + # Keep the memory-sizing benchmark outside manifests/, which + # clean_exposure deletes wholesale. f"{EXP_DIR}/exp_psf.benchmark.tsv" resources: mem_mb = lambda wc, attempt: 16000 * attempt, @@ -126,33 +101,11 @@ rule exp_psf: sp_shell("exp_psf", f"config_exp_{PSF_MODEL}.ini") -# --- persistence (D5) ------------------------------------------------------- -# The counterpart of reclamation, and it must come first in the DAG: this packs -# the exposure's keepable PSF products into one tar on the persistent root, and -# clean_exposure below takes its manifest as an input so the store is never -# reclaimed before the keepers have left /scratch. The purge would take them -# anyway — that, not clean_exposure, is what this rule exists for -# (persist_exp.py's docstring argues both halves, and config.yaml's -# `persist_exp:` block carries the keep list and its candidates). -# -# A LOCALRULE (declared in the Snakefile), by exactly the arithmetic that made -# clean_exposure one: the body is a `tar` of a few MB from one shared filesystem -# to another, seconds of work, and one sbatch per exposure would be ~20k -# submissions at DR6 scale for jobs shorter than the scheduling latency. The -# grouping constraint that binds mid-chain localrules (this file's docstring) -# does not bite here: exp_persist's only neighbours are exp_psf, which is too -# heavy to ever fuse, and clean_exposure, which is local itself. -# -# ONE DECLARED OUTPUT, AND IT IS A MANIFEST, NOT THE TAR OR A directory(). The -# tar is not declared: a directory output would attest that a directory exists, -# where what we want written down is WHICH files were packed and how big each was — -# the provenance a rho-statistics run months from now needs in order to know -# what it is reading. The manifest is byte-stable, so a no-op rerun does not -# move its mtime and does not make clean_exposure look out of date. -# -# THE KEEP LIST RIDES ON params. That is the entire reason this is not three -# lines of tar appended to exp_psf's shell: `params` is a rerun trigger, so -# adding a pattern reruns the packing and leaves the PSF chain alone. +# --- persistence ----------------------------------------------------------- +# Packs PSF products into a tar on the persistent root; see persist_exp.py for +# retention, provenance and byte-stable manifest semantics. This is a localrule +# (Snakefile): seconds of packing do not warrant ~20k SLURM submissions at DR6. +# The keep list is a params rerun trigger, so an edit re-packs without refitting. rule exp_persist: input: rules.exp_psf.output.manifest @@ -162,9 +115,7 @@ rule exp_persist: # name collision, both of which it reports on stderr and neither of which # has a per-CCD verdict worth a completeness record. params: - # Only the OPTIONAL retention list travels: psf_validation is packed - # by persist_exp.py whatever this says. It still rides on params, so - # adding a product re-packs (seconds) rather than re-fitting the PSF. + # Optional products only; persist_exp.py always packs psf_validation. patterns = " ".join(f"--pattern '{p}'" for p in PERSIST_EXP), exp_dir = lambda wc: exp_dir(wc.exp), dest = lambda wc: f"{prod_exp_dir(wc.exp)}/psf", @@ -214,52 +165,23 @@ rule exp_maps: " --fragment '{params.fragment}' --manifest {output.manifest}" -# --- reclamation (D5) ------------------------------------------------------- -# The one exception to "no reclamation in this file": clean_exposure OWNS -# exposure-level deletion, and it is a real job, not temp() bookkeeping, because -# an exposure's consumer set closes over the CAMPAIGN. The index supplies that -# set (EXP_TILES, accumulated across invocations); the input is every consuming -# tile's tile_vignets manifest — vignets is the last stage that reads exposure -# products, everything after it reads tile-level files. -# -# What the job deletes, and why a late append still behaves, is argued in -# clean_exposure.py's docstring; params.consumers is what makes a grown consumer -# set stale (same file). -# -# The tile side reads the exposure manifests through ancient() and cuts the -# reclaimed edges of finished tiles (see tile.smk), which is what keeps this -# deletion from rebuilding every neighbouring tile. This rule's OWN inputs are -# deliberately not ancient: a tile that really did rebuild its vignets must -# reschedule the cleans of the exposures it read. -# -# A localrule (declared in the Snakefile). Local execution serialises the cleans -# under local-cores, which costs nothing at rmtree speed and never blocks the -# compute chains (this rule is in none of them). +# --- reclamation ----------------------------------------------------------- +# Reclaims exposure stores after their in-scope consumers finish tile_vignets, +# the last tile stage that reads exposure products. Deletion and consumer-set +# staleness are documented in clean_exposure.py; tile.smk owns reclaimed-edge +# handling. Inputs here are not ancient: rebuilt vignets must reschedule clean. +# This is a localrule (Snakefile), serialized under local-cores. rule clean_exposure: input: - # ONLY the consumers this invocation may actually build. A consumer that - # is out of scope had its vignets manifest checked for existence at parse - # time (clean_targets' eligibility test) — declaring it here as well would - # pull that finished tile's whole chain into the DAG, where a rebuilt - # shared exposure then reruns it. That is how one damaged tile reached its - # spatial neighbours. In-scope consumers keep their edge: they may run in - # this DAG, so the clean must be ordered after them. + # In-scope consumers only; clean_targets checks out-of-scope vignets + # at parse time. Declaring them here would pull finished tiles back + # into the DAG. See workflow/CONTRACTS: + # clean-exposure-waits-on-persist-iff-psf. lambda wc: [tile_manifest(t, "tile_vignets") for t in clean_consumers(wc.exp) if t in READY_SET], - # The keepers must be off /scratch before the store goes. No keep list - # removes this edge: exp_persist always packs the star catalogue's - # inputs. Only psf_model=fake does, which has no PSF products to keep - # (PERSISTS_PSF, Snakefile). - # - # A LIVE exposure is asked for its exp_persist manifest: the thing to - # build, and what orders this rule after the pack. A RECLAIMED one - # (exp_store_reclaimed, Snakefile) is asked for its TAR if it has one — - # on the persistent root, no rule's declared output, hence a leaf that - # requires nothing — and for nothing if it has none. Naming the - # manifest there reopens the reclaimed chain: a `persist_exp:` edit - # changes exp_persist's params, the manifest reruns, and it sits behind - # exp_psf's manifest, which went with the store, so snakemake rebuilds - # the exposure from VOS. + # Preserve-before-delete ordering, gated by PERSISTS_PSF. durable_edge + # (Snakefile) uses the manifest for a live store and the durable product + # for a reclaimed store; the custody contract above governs this edge. lambda wc: (durable_edge(wc.exp, "exp_persist", prod_exp_tar(wc.exp)) if PERSISTS_PSF else []), # The exposure maps' fragment, by the same rule. @@ -281,49 +203,16 @@ rule clean_exposure: # --- the campaign's star catalogue ------------------------------------------ -# ONE job per campaign: every exposure's every CCD's `validation_psf--.fits`, -# collected into `/full_starcat_.hdf5`, one dataset per -# exposure. That file is the rho/tau statistics input; the old bash chain built -# a flat FITS table with `combine_runs.bash psf` + a `merge_starcat_runner` -# pass, and the workflow emitted neither. sp_validation still opens the FITS -# name today — CosmoStat/sp_validation#340 moves its readers to this file, the -# same migration that retires the `patches/` key on the tile side. -# -# ONE DATASET PER EXPOSURE, NOT ONE TABLE, and it is the same decision as the -# tile side's: it makes the file RECONCILABLE. A flat table had to be restacked -# from every exposure the campaign had ever seen to add one — ~40 GB of members -# at DR6 scale to add ~2 MB — and held the whole campaign in memory while it did -# so. Reconciled, an append reads the appended exposures and nothing else, and -# the job holds one exposure at a time. hdf5_reconcile.py is the shared -# machinery; merge_star_cat.py argues the format and the tar reading. -# -# THE INPUT IS star_cat_inputs() (Snakefile): every exposure of TILES_READY whose -# PSF products are on the persistent root — the live ones through the exp_persist -# manifest edge `rule all` already requests, the RECLAIMED ones through their TAR, -# which no rule declares and which therefore requires nothing to be built. That -# asymmetry is not a flourish; requesting a reclaimed exposure's manifest -# rebuilds its whole chain from VOS, and ancient() does not prevent it (measured -# — the Snakefile carries the numbers). Nothing new enters the DAG either way. It -# is read through an INPUT FUNCTION rather than at module level so that only a -# parse which actually builds this job pays for the walk. -# -# THE PATHS DO NOT REACH THE SHELL, and that is not a style choice: ~20k manifest -# paths is an order of magnitude over Linux's 128 KiB MAX_ARG_STRLEN for a single -# argv entry, so `{input}` here would be a job that dies on exec at DR6 scale. -# The job is handed the two small files the Snakefile itself started from — the -# tile list and the index — and derives THE SAME SET from them; `params.inputs` -# carries that set's FINGERPRINT, which is the rerun trigger. The equality is -# the point: a job that stacked anything the fingerprint did not see would be -# rows no rerun trigger could notice, which is what a glob over products_dir -# would have given on a root shared with an earlier, larger tile list. -# -# NOT A LOCALRULE. exp_persist is local because it is 20k jobs of seconds; this -# is one job that reads the campaign's tars end to end. Its MEMORY is flat in -# the campaign (one exposure at a time) and sized on the largest exposure; its -# RUNTIME is the total. +# One compute job per campaign writes /full_starcat_.hdf5, +# the rho/tau statistics input. merge_star_cat.py owns the reconciled format +# and tar reading; star_cat_inputs/targets (Snakefile) select durable inputs +# and suppress the job when none remain. sp_validation reads this format once +# CosmoStat/sp_validation#340 lands; until then it opens the flat FITS catalogue. # -# NO JOB AT ALL when every exposure in scope is tombstoned with no tar left -# behind: star_cat_targets() (Snakefile) simply does not request the output. +# Input paths are DAG edges, not shell arguments: ~20k paths exceed Linux's +# 128 KiB limit for one argv entry. The script derives the same exposure set +# from the tile list and index; params.inputs fingerprints that set for reruns. +# See star_cat_inputs (Snakefile) for the matching-set constraint. rule star_cat_merge: input: lambda wc: star_cat_inputs() @@ -339,18 +228,15 @@ rule star_cat_merge: script_hash = MERGE_STAR_HASH threads: 1 resources: - # Sized on the campaign's own member bytes, slope and intercept - # measured (the Snakefile's sizing block carries both points, and the - # ceiling this rule runs into at DR6 scale). Still * attempt, because a - # measured slope on synthetic tars is not a guarantee about real ones. - # Sized on the LARGEST exposure, not the total: the merge holds one - # exposure at a time (the Snakefile's sizing block carries the history). + # Memory scales with the largest exposure, held one at a time; + # Snakefile's sizing block owns the measurements. Retry scaling allows + # for real catalogues exceeding the measured synthetic footprint. mem_mb = lambda wc, attempt: capped_mem(attempt * ( STAR_MEM_BASE_MB + STAR_MEM_FACTOR * star_cat_max_bytes() // 1_000_000), "star_cat_merge"), - # ~2 min per GB of members on the measurement above, doubled, over a - # floor that covers the fixed cost of opening ~40 members per exposure. + # Runtime scales with total member bytes: ~2 min/GB measured, doubled, + # plus a floor for opening the per-CCD members. runtime = lambda wc, attempt: attempt * ( 30 + 4 * star_cat_bytes() // 1_000_000_000) shell: From 98f4ecd0e0ba6cd1bf10252992ee4ce6bfc10c63 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:46:51 +0200 Subject: [PATCH 23/43] docs(launcher): present-tense comments in sp Co-Authored-By: GPT-6.1 Sol --- workflow/bin/sp | 180 +++++++++++++++--------------------------------- 1 file changed, 54 insertions(+), 126 deletions(-) diff --git a/workflow/bin/sp b/workflow/bin/sp index cb6754108..e75370849 100755 --- a/workflow/bin/sp +++ b/workflow/bin/sp @@ -8,14 +8,11 @@ # Snapshots the code into the state dir first (see "the # launch code snapshot" below) and runs out of the copy, so # editing the checkout mid-campaign cannot reach the jobs. -# Two snakemake invocations over one Snakefile: -# 1. PREPARE snakemake prepare_all_tiles -# 2. COMPUTE snakemake all <- its PARSE builds the index -# ARGS (--jobs, -n, --forcerun, ...) pass through to BOTH. -# FILE is a user-specified config file that can be used -# to add and/or overwrite entries in the default config -# file workflow/config.yaml (read automatically) -# sp report [ARGS...] emit run_report.json now (mid-run is fine). +# Two Snakemake invocations: prepare_all_tiles, then all. +# See workflow/Snakefile for index construction between phases. +# ARGS (--jobs, -n, --forcerun, ...) pass through to both. +# FILE overrides workflow/config.yaml (read automatically). +# sp report [ARGS...] emit run_report.json (mid-run is fine). # sp container VERB manage the image every job runs inside: pull, status, # sandbox, exec, resolve. `sp container --help` documents # the layers and the resolution order. @@ -23,17 +20,14 @@ # Anything else is passed straight through to snakemake with the same profile and # state dir (the escape hatch: `sp --unlock`, `sp exp_psf ...`, `sp --dag`). # -# It also loads the apptainer module (snakemake resolves `apptainer` via PATH at -# job runtime) and activates the snakemake venv on the shared /project FS (the -# executor re-invokes it inside jobs, so it cannot live on a node-local path). -# One entry point, so a fresh tmux or a restart after a crash always launches -# with the right state. +# It ensures apptainer is on PATH and activates the Snakemake venv on shared +# /project storage: the executor re-invokes Snakemake inside compute jobs. # # Typical use, from the repo root: # # SP_PROFILE=candide workflow/bin/sp run -c ~/my_run.yaml # a campaign # SP_PROFILE=candide workflow/bin/sp run -c ~/my_run.yaml -n # dry run -# SP_PROFILE=candide workflow/bin/sp report -c ~/my_run.yaml # status now +# SP_PROFILE=candide workflow/bin/sp report -c ~/my_run.yaml # status # # Run config: -c FILE, also --config-file. Read after workflow/config.yaml and # merged on top; anything still unset comes from the machines: entry for @@ -62,13 +56,9 @@ VENV="${SP_SNAKEMAKE_ENV:-/project/def-mjhudson/cdaley/snakemake-env}" SCRIPTS="$HERE/scripts" PROFILE="${SP_PROFILE:-nibi}" -# -c is consumed here, not forwarded: snakemake's own --configfile merges -# differently, and the jobs and the snapshot need it as SP_RUN_CONFIG. The scan -# stops at `--` and at the command after `container exec`, so a -c meant for -# the delegated command (`sp container exec python -c ...`) or for snakemake's -# own escape hatch (`sp -- ...`) is left alone. `--config` is NOT sp's: keeping -# it free lets snakemake's own `--config k=v` (what the Snakefile's flag() -# reads) pass through untouched. +# Consume -c as SP_RUN_CONFIG for shared config resolution and snapshotting. +# Stop at -- or container exec so delegated commands keep their own -c. +# Pass --config k=v through to Snakemake unchanged. RUN_CONFIG="${SP_RUN_CONFIG:-}" _sp_args=() while [ $# -gt 0 ]; do @@ -81,9 +71,7 @@ while [ $# -gt 0 ]; do -c=*|--config-file=*) RUN_CONFIG="${1#*=}"; shift ;; container) - # Everything from `exec`'s own command on is the delegated command's, - # not sp's: `container.py` reads SP_RUN_CONFIG, so its own -c is never - # sp's run config. + # container.py reads SP_RUN_CONFIG; exec arguments belong to its command. _sp_args+=("$1"); shift [ "${1:-}" = exec ] && { _sp_args+=("$@"); break; } ;; @@ -103,12 +91,9 @@ fi [ -d "$REPO/profiles/$PROFILE" ] || { echo "sp: no profile $REPO/profiles/$PROFILE (SP_PROFILE=$PROFILE)" >&2; exit 2; } -# snakemake resolves `apptainer` via PATH at job runtime, so all this has to -# do is put it there. candide already has it in /usr/bin and ships no -# modulefile for it, so probing for one only produces a scary ERROR line; -# skip the module entirely when apptainer is already usable. Both streams are -# dropped because the message arrives on stderr under environment-modules and -# can arrive on stdout under Lmod. +# Load a module only if apptainer is absent from PATH; candide supplies it in +# /usr/bin without a modulefile. Suppress both module output streams because +# environment-modules and Lmod report errors on different streams. if ! command -v apptainer >/dev/null 2>&1; then module load apptainer/1.4.5 >/dev/null 2>&1 || true command -v apptainer >/dev/null 2>&1 || { @@ -127,9 +112,8 @@ fi # Snakefile resolves it. cfg() { python "$SCRIPTS/run_config.py" "$HERE/config.yaml" "${SP_RUN_CONFIG:-}" "$1"; } RUN_DIR="$(cfg outputs.run_dir)"; INDEX_DB="$(cfg outputs.index_db)" -# `sp container`, `sp cancel` and the help forms (bare `sp`, -h, --help) need -# neither a campaign nor its state dir; every other verb does. Checked here, before the state dir is created from -# RUN_DIR: with `run:` unset, RUN_DIR still holds a literal `$run`. +# Container, cancel and help need no campaign state. Check other commands +# before mkdir: an unset run leaves a literal $run in RUN_DIR. case "${1:-}" in container|cancel|""|-h|--help) NEEDS_CAMPAIGN=0 ;; *) NEEDS_CAMPAIGN=1 ;; esac if [ "$NEEDS_CAMPAIGN" = 1 ]; then if [ -z "$(cfg run)" ]; then @@ -140,50 +124,33 @@ if [ "$NEEDS_CAMPAIGN" = 1 ]; then fi fi -# Snakemake state (.snakemake: metadata, locks, incomplete markers) lives NEXT TO -# THE RUN on /scratch, never on the persistent root — the one exception to D5's -# "durable, low-volume products go on /project". It is neither: one small, hot -# metadata file per output, rewritten on every job, which at DR6 is >170k files -# of churn against a backed-up filesystem with a 1M-inode group quota. It is also -# reconstructible — losing it costs a re-parse, not a re-run. (A hard 27/27 TiB -# group quota killed a metadata write mid-run, live. The quota has since eased; -# the placement is right on its own merits.) -# --directory only moves state: all data paths are absolute, and the Snakefile +# Default state location: beside the run on /scratch, not the persistent root. +# DR6 produces >170k frequently rewritten metadata files; keeping that churn +# off backed-up /project avoids pressure on its ~1M-inode group quota. +# --directory moves state, not data: data paths are absolute, and the Snakefile # resolves its own configfile. STATE_DIR="${SP_STATE_DIR:-${RUN_DIR}-state}" [ "$NEEDS_CAMPAIGN" = 0 ] || mkdir -p "$STATE_DIR" # --- the launch code snapshot ---------------------------------------------- -# THE ONE HOME for this concept; everything else points here. +# SLURM jobs re-parse the Snakefile and read scripts/configs when they start, +# potentially hours after launch. sp run copies code to $STATE_DIR/code so +# checkout edits cannot give jobs in one campaign different implementations. # -# WHY. A campaign is not one process. The SLURM executor re-invokes snakemake on -# every job's node, so each job RE-PARSES the Snakefile, and each shell reads -# workflow/scripts/*.py and workflow/config/cfis/* when it STARTS -- hours after -# launch, out of whatever the checkout contains THEN. Editing the checkout -# mid-campaign therefore fed different code to different jobs of the same run -# (the 186.307 partition incident: an edit landed four seconds into a tile split -# it into two disagreeing halves). +# @sc [label:custody] launch-snapshot-self-contained +# The snapshot includes workflow/, src/, scripts/ and profiles/. Dereference +# config symlinks into example/ so jobs need no live checkout. Workflow paths +# resolve from workflow.basedir, including the path-loaded final catalogue +# script; rewrite the profile's PYTHONPATH to the snapshot's src/. Copy the +# external run config too, so every job parse reads the launch configuration. # -# WHAT. `sp run` copies the code it is about to launch into $STATE_DIR/code and -# runs the campaign entirely out of that copy: the Snakefile, the rules, the -# scripts, the ini chain (symlinks DEREFERENCED -- workflow/config/cfis points -# into example/, and the copy must be self-contained), src/, the repo's own -# scripts/ (final_cat_merge loads scripts/python/create_final_cat.py by path -- -# it is a script, not an installed module, and the hdf5 layout it defines must -# be pinned to the campaign like everything else here), and the profile. -# Every workflow-internal path hangs off `workflow.basedir`, which IS the -# snapshot, so they all follow it for free; the profile's PYTHONPATH pin is the -# one that cannot (YAML splices nothing) and is rewritten below. -# -# CONSEQUENCE, and the reason the old "never edit workflow/ while a campaign -# runs" warnings are gone: mid-campaign edits to the checkout are inert. A change -# takes effect on the next `sp run`, and not before. -# -# LIFECYCLE: one snapshot per campaign state dir, REFRESHED WHOLESALE on every -# `sp run` -- a new `sp run` IS the relaunch, and snakemake's params/code rerun -# triggers decide what the new code invalidates. Nothing accumulates, and the -# path is stable, so a later `sp --unlock` reaches the same workflow that took -# the lock. +# @sc [label:hazard] launch-snapshot-refresh-boundary +# Every sp run refreshes the same snapshot path wholesale. Relaunch only after +# prior jobs using that state directory have stopped: queued or running jobs +# can otherwise read the refreshed code. Snakemake rerun triggers determine +# which finished products the relaunch invalidates (see workflow/CONTRACTS, +# unit-pre-changes-at-campaign-boundary). Other verbs reuse the existing copy, +# so sp --unlock reaches the workflow that took the lock. SNAPSHOT="$STATE_DIR/code" snapshot_code() { mkdir -p "$SNAPSHOT" @@ -196,12 +163,8 @@ snapshot_code() { find "$SNAPSHOT" -name __pycache__ -type d -prune -exec rm -rf {} + fi - # The profile's `apptainer-args:` hardcodes one checkout's src/ in its PYTHONPATH - # pin and CANNOT interpolate anything (snakemake escapes a literal `$` before the - # string reaches any shell -- the post-mortem is on that line). So the pin is - # rewritten HERE, in the snapshot's copy, and `sm` points --profile at that copy. - # The checked-in profile keeps its literal checkout path and stays the source of - # truth for the FLAGS; exactly one path is substituted, by one line of sed-work. + # Rewrite only PYTHONPATH in the generated profile; the checked-in profile + # owns the flags. Variable expansion limits are documented in profiles/nibi. python - "$SNAPSHOT/profiles/$PROFILE/config.yaml" "$SNAPSHOT/src" <<'PY' import pathlib, re, sys f, src = pathlib.Path(sys.argv[1]), sys.argv[2] @@ -213,20 +176,10 @@ if not n: f.write_text(text) PY - # `tile_store_root:` in the run config moves the node-local tile store - # (tile.smk's tile_local(): the vignette + WCS stores of each tile_shape group) - # off the profile's `/local/scratch` bind, for this campaign only. It is the - # same substitution as the pin above -- the BIND SOURCE of `/local/scratch` in - # the snapshot's profile -- and deliberately not a path in tile_local(): that - # function's output is a params rerun trigger, so a path there would reschedule - # every finished tile of a resumed campaign (its warning block says how badly), - # while a bind is not a trigger at all (a dry run after changing it schedules - # only the missing tiles). Why a campaign wants it: candide binds the node's - # 31 GB /tmp, which a few dense image-sim tile stores or failed groups' orphans - # fill, after which every later group on that node fails in seconds. A shared - # directory (NFS) trades ngmix read latency for room; the campaign-unique store - # names (LOCAL_TAG) keep concurrent campaigns apart in one root. Profiles with - # no `X:/local/scratch` bind (nibi's `--bind /local`) get one appended. + # Move the tile store by rewriting the bind source, not tile_local()'s params: + # params changes invalidate finished tiles (workflow/CONTRACTS). This bind is + # not a rerun trigger. Append it if the profile has no X:/local/scratch bind. + # See workflow/config.yaml for tile_store_root's capacity/latency trade-off. python - "$SNAPSHOT/profiles/$PROFILE/config.yaml" "$(cfg tile_store_root)" <<'PY' import os, pathlib, re, sys f, root = pathlib.Path(sys.argv[1]), sys.argv[2] @@ -282,24 +235,13 @@ PY # declared tiles allowed to be missing their exposure list (default 0.0). export SP_MISSING_THRESHOLD="${SP_MISSING_THRESHOLD:-0.0}" -# WHICH code a verb runs. `sp run` refreshes the snapshot and runs out of it; -# every other verb and the escape hatch run out of the EXISTING snapshot when the -# state dir has one -- `sp --unlock` and `sp report` must speak for the campaign -# that is actually on disk, not for a checkout that has moved since. Before the -# first `sp run` there is no snapshot and the live checkout is all there is. -# (`sp container` is the exception: it is interactive tooling about the image, -# not about a campaign, and stays on the checkout.) +# Use the campaign snapshot when present; otherwise use the checkout. +# Container tooling bypasses this helper because it is not campaign-specific. code_root() { [ -f "$SNAPSHOT/workflow/Snakefile" ] && echo "$SNAPSHOT" || echo "$REPO"; } -# --snakefile pins the workflow to one tree: sp must work from any cwd (an sbatch -# head job starts in the submission directory, not the repo), and the SLURM -# executor re-invokes snakemake with this same path inside every job -- which is -# exactly what makes the snapshot stick. -# -# SP_PHASE is REQUIRED by the Snakefile (a bare `snakemake` would build an empty -# DAG and exit 0). `sp run` sets prepare/compute on its two invocations; every -# other verb and the escape hatch fall through to `passthrough`, which parses the -# index without building it and schedules no side effects. +# Absolute --snakefile works from any cwd and pins executor job parses to the +# same tree. Set the phase required by the Snakefile: run uses prepare/compute; +# other commands default to passthrough (see the Snakefile's SP_PHASE gate). sm() { local root; root="$(code_root)" # The run config the Snakefile reads: the snapshot's copy when this run has @@ -318,11 +260,8 @@ case "$cmd" in run) shift snapshot_code - # PREPARE failing is NOT fatal to the run: keep-going means a failed tile - # poisons only its own cone, and the tiles that lost their exposure list are - # dropped at the compute parse. The real gate is SP_MISSING_THRESHOLD, which - # the compute parse's index build enforces over the WHOLE tile list — so we - # record the failure and go on rather than letting `set -e` abort here. + # Continue after prepare failure so compute can apply SP_MISSING_THRESHOLD + # to the whole tile list. Retain prepare's failure for the final exit code. prep_rc=0 SP_PHASE=prepare sm prepare_all_tiles "$@" || prep_rc=$? if [ "$prep_rc" -ne 0 ]; then @@ -344,10 +283,7 @@ case "$cmd" in fi ;; report) - # Out of the snapshot when there is one, so a manual report reads the trees - # with the same script the campaign's own onsuccess/onerror hooks use (they - # call it through the Snakefile's SCRIPTS, which is the snapshot). Read-only - # either way -- this is consistency, not safety. + # Use the same snapshot script as the campaign's automatic report hooks. shift SP_TILE_DETECTION="$(cfg tile_detection)" \ python "$(code_root)/workflow/scripts/run_report.py" \ @@ -355,21 +291,13 @@ case "$cmd" in --status manual "$@" ;; container) - # The image layer, outside snakemake entirely: the script is stdlib-only so - # it runs on the bare host, and the apptainer module is already loaded above. - # Deliberately the CHECKOUT's copy, not the snapshot's: `sp container` is - # about the image you are working with now (status compares against HEAD, and - # the snapshot has no .git), and `exec` should hand you the environment your - # NEXT run will have. It reads the checked-in profile's apptainer-args, so a - # running campaign whose snapshot predates an edit to that line gets the old - # value while `exec` shows the new one -- the only place the two can differ, - # and it differs in the direction that matters (jobs stay pinned). + # Run host-side container tooling from the checkout: status compares against + # HEAD, and exec uses the checked-in profile rather than a campaign snapshot. shift python "$SCRIPTS/container.py" "$@" ;; cancel) - # Kept only because it is two lines: scancel this workflow's jobs by name - # before an --unlock. Not part of the design surface. + # Cancel the user's jobs whose names match the supplied regular expression. run="${2:?usage: sp cancel }" squeue --me --noheader --format='%i %j' \ | awk -v r="$run" '$2 ~ r {print $1}' | xargs -r scancel From 283fb0ded055dd125f97751cd1d3342f3eae9d49 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:46:58 +0200 Subject: [PATCH 24/43] docs(profiles): present-tense comments in cluster configs Co-Authored-By: GPT-6.1 Sol --- profiles/candide/config.yaml | 42 ++++------ profiles/nibi/config.yaml | 152 +++++++---------------------------- 2 files changed, 47 insertions(+), 147 deletions(-) diff --git a/profiles/candide/config.yaml b/profiles/candide/config.yaml index 2932e64eb..bc82c8ee2 100644 --- a/profiles/candide/config.yaml +++ b/profiles/candide/config.yaml @@ -12,41 +12,33 @@ default-resources: # comp nodes have TmpDisk=0, so the tile_shape group (--tmp, see tile.smk's # TILE_SLURM_EXTRA) lands on pscomp; everything else may use either. slurm_partition: "comp,pscomp" - # n09/n17/n36: excluded in the sp_validation candide profile; n23: slurm jobs - # failed there (sp_validation 2b40c99). + # n09/n17/n36 follow sp_validation's candide exclusions; n23 has measured + # SLURM job failures (sp_validation commit 2b40c99). slurm_extra: "'--exclude=n09,n17,n23,n36'" software-deployment-method: [apptainer] -# /local/scratch is where tile.smk puts the node-local tile store (TILE_LOCAL, -# ~5.6 GB per real-data tile). candide has no /local, and its node-local /scratch -# is writable only per job (/scratch/$USER/$SLURM_JOB_ID, made by the prolog), -# which this line cannot name: snakemake passes it through literally, and -# --cleanenv drops SLURM_JOB_ID inside the container. So the node's /tmp (local -# disk, ~29 GB free, sticky) is bound there instead. That holds ~5 concurrent -# tile stores per node, too few for dense image-sim tiles: such a campaign sets -# `tile_store_root:` in its run config, and bin/sp replaces this bind's source in -# the launch snapshot (see workflow/config.yaml). The PYTHONPATH pin is rewritten by bin/sp to the -# launch snapshot's src/; the value here only has to be a checkout's src/. +# Bind node /tmp (~29 GB free, local disk) to tile.smk's /local/scratch: +# candide has no /local, and its writable /scratch path requires a job ID. +# Variable expansion limits are documented in profiles/nibi/config.yaml. +# The disk holds ~5 real-data tile stores (~5.6 GB each); dense image-sim runs +# can set tile_store_root in the run config (workflow/config.yaml). +# PYTHONPATH names a checkout's src/; workflow/bin/sp rewrites the launch copy. apptainer-args: "--cleanenv --env OMP_NUM_THREADS=1 --env MALLOC_ARENA_MAX=2 --env PYTHONPATH=/n17data/mkilbing/astro/repositories/github/shapepipe/src --bind /home,/automnt,/n17data,/n23data1,/n09data --bind /tmp:/local/scratch" latency-wait: 60 # NFS: wait for outputs to appear after a job keep-going: true # a failed job poisons only its cone; siblings run on -# Transient node-level failures are the common case on a shared cluster: -# a fused tile_shape group died at joblib's fork_exec with BlockingIOError -# [Errno 11] because the node was out of process slots, killing one tile of -# 39 outright. Without this, attempt is always 1, so every -# `mem_mb = lambda wc, attempt: N * attempt` in tile.smk is dead code and a -# one-off OOM or EAGAIN is fatal. Safe for the fused group: a retry -# resubmits the same member set, tile_vignets included (tile.smk's -# "one sharp edge" note). tile_ngmix keeps its own rule-level retries: 2. +# Retry transient node failures, including measured process-slot exhaustion +# (BlockingIOError, Errno 11) in tile_shape. Retries also activate attempt-scaled +# memory requests. See tile.smk for group retry semantics; tile_ngmix sets 2 +# retries at rule level. retries: 2 rerun-incomplete: true # re-do jobs left incomplete by an unclean death show-failed-logs: true printshellcmds: true rerun-triggers: [mtime, params, code, software-env] -# The tile_shape group (tile.smk) sets its own slurm_extra (TILE_SLURM_EXTRA, -# the --tmp floor), which REPLACES the default above and so drops the node -# excludes: its jobs landed on n09 and n17. Restate both, identically on all -# four members (a group's string resources must agree). Only slurm_extra is -# overridden; the rules keep their own threads and attempt-scaled mem_mb. +# @sc [label:coupling] candide-group-slurm-extra +# tile_shape's rule-level slurm_extra replaces the profile default. Set both +# the --tmp floor and node exclusions identically on all four member rules: +# a group's string resources must agree. Leave rule threads and attempt-scaled +# memory intact. set-resources: tile_vignets: slurm_extra: "'--tmp=16000 --exclude=n09,n17,n23,n36'" diff --git a/profiles/nibi/config.yaml b/profiles/nibi/config.yaml index 76cf19e70..265c49541 100644 --- a/profiles/nibi/config.yaml +++ b/profiles/nibi/config.yaml @@ -1,14 +1,12 @@ # Snakemake profile for the Nibi cluster (Digital Research Alliance). # -# SLURM-EXECUTOR mode (PRD #848 D-profile): one SLURM job per rule instance, -# each carrying its rule's own attempt-scaled resources (cpus_per_task, -# mem_mb, runtime). Snakemake feeds the queue as jobs finish, so DR6's ~170k -# total jobs never queue at once, and multi-node scaling is inherent — this -# supersedes the earlier one-allocation/local-scheduler mode. +# The SLURM executor submits one job per rule instance or fused group, using +# the rules' resources. Snakemake feeds the queue as jobs finish, so DR6's +# ~170k jobs do not queue at once. # # Launch via `workflow/bin/sp` (loads apptainer/1.4.5, uses the /project venv). # software-deployment-method wraps every job's shell in `apptainer exec` — the -# user never types apptainer. WHICH image is resolved in the Snakefile through +# user never types apptainer. The image is resolved in the Snakefile through # workflow/scripts/container.py, whose docstring documents the layers and the # resolution order; `sp container status` prints which layer is live. @@ -28,93 +26,22 @@ default-resources: slurm_account: def-mjhudson_cpu software-deployment-method: [apptainer] -# Explicit container environment (finding 2/3): the apptainer SDM otherwise drops -# the proven-recipe env and hardcodes --home , hiding ~/.ssl/cadcproxy.pem. -# --cleanenv strict host-env isolation (APPTAINERENV_*/SINGULARITYENV_* survive) -# OMP_NUM_THREADS=1 caps OpenBLAS fork-explosion (verified: pool 32->1) -# MALLOC_ARENA_MAX=2 bounded allocator (im_sims/nibi lesson) -# --home /home/cdaley wins over the SDM's --home ; restores cadcproxy.pem for vos/vcp -# >>> EDIT THIS: THE ONE NON-PORTABLE LINE IN THIS FILE. <<< -# The apptainer-args string below hardcodes one user's home -# (--home /home/cdaley). A different user must edit it by hand. -# IT CANNOT BE FIXED HERE: YAML cannot splice an env var, and -# snakemake escapes a literal `$` before the string reaches -# any shell (see the $SLURM_TMPDIR post-mortem below, which -# cost a whole campaign). Nor can bin/sp inject it through -# --apptainer-args: that REPLACES the profile value rather -# than appending to it, so the launcher would have to restate -# this whole string -- and workflow/scripts/container.py reads -# THIS LINE to give `sp container exec` the same environment -# jobs get, so a second definition is exactly the divergence -# that mechanism exists to prevent. -# THE PYTHONPATH PIN IS THE ONE EXCEPTION, and it is not -# edited by hand: `sp run` copies this whole file into the -# campaign's code snapshot and rewrites that ONE path to the -# snapshot's src/ (bin/sp, "the launch code snapshot"). Still -# one source of truth for the flags -- the copy is generated, -# never edited -- and jobs stop reading a live checkout. -# --bind /local NODE-LOCAL NVME. $SLURM_TMPDIR on nibi is -# /local/scratch/..0, and the mount is /local -# (/dev/nvme0n1, ext4, 3.5 TB) -- binding /localscratch is -# NOT enough, the real path is under /local (verified, -# probe job 20794182). The fused `tile_shape` group job -# WRITES its 5.6 GB vignette store there and never puts it -# on scratch at all (workflow/rules/tile.smk); every other -# rule simply ignores the bind. Adding it does NOT -# invalidate anything: the `software-env` rerun trigger -# hashes only job.container_img_url (+ conda env, env -# modules) -- -# snakemake/persistence/__init__.py::_software_stack_hash, -# v9.23.1 -- and apptainer-args is not in that hash. -# -# HOW the in-container shell LEARNS its node-local dir: -# IT DOES NOT. It DERIVES the path from the tile wildcard -# (tile.smk::tile_local -> /local/scratch/sp--), and -# that is the only mechanism that survives contact with -# snakemake. Three alternatives were tried; all fail, and -# the third failed IN PRODUCTION, so read this before -# reaching for any of them again: -# * {resources.tmpdir} is NOT $SLURM_TMPDIR. Snakemake -# defers that resource to the job and evaluates it as -# tempfile.gettempdir(); nibi's Slurm does not export -# TMPDIR, so it lands on /tmp -- which on a compute -# node is a 378 GB RAM-backed tmpfs whose pages are -# charged to the job's memory cgroup. Staging 6 GB -# there would eat the job's RAM, not use the NVMe. -# (Measured, job 20794656.) -# * APPTAINERENV_SLURM_TMPDIR would have to be exported -# by something that already knows the JOB's value; -# bin/sp runs on the login node, where it is unset. -# * SPLICING "$SLURM_TMPDIR" INTO apptainer-args AND -# LETTING THE JOB'S SHELL EXPAND IT DOES NOT WORK. -# This was the shipped design for exactly one campaign -# and it killed every tile of run 20798193 in eight -# minutes: snakemake ESCAPES the `$` before the string -# reaches any shell, so the container receives the -# literal characters and never a path. NOTHING can be -# carried into the container this way -- not a tmpdir, -# not anything else. The bind below is static, and the -# VALUE is derived job-side from a wildcard instead. -# Every tile_shape member still REFUSES TO RUN if the -# derived directory is not real (tile.smk), which is the -# only reason that campaign cost minutes and not a night: -# a broken node-local path is a loud failure, never a -# silent slide back onto NFS, and never a 5.6 GB write -# into the RAM-backed /tmp tmpfs. -# PYTHONPATH SETTLED CALL 3 — pins THIS branch's src/, which is -# develop@97e16d50 plus the four commits that genuinely need -# module code: ngmix chunk fields + position-seeded RNG, -# merge_sep_cats chunk paths, the vizier star-cat helpers, -# and the cherry-picked #873 (seeded setools split). -# NOT shapepipe-prod (drifted to a PR branch mid-run, and the -# live p3-batch1 job reads it) and not the sif default -# (frozen pre-#843, and pre-#873). Production later rebuilds -# the sif at the validated commit and DROPS this --env line. -# The path written HERE is the checkout's, and is what -# `sp container exec` uses; the value JOBS see is the -# snapshot's src/, substituted per the paragraph above. So a -# running campaign holds its launch code even after this -# line (or the tree it names) changes. +# Explicit container environment: +# --cleanenv isolates host variables (APPTAINERENV_*/SINGULARITYENV_* survive). +# OMP_NUM_THREADS=1 caps thread pools (measured OpenBLAS pool: 32 -> 1). +# MALLOC_ARENA_MAX=2 bounds allocator arenas. +# --home overrides Snakemake's workdir home so vos/vcp can read cadcproxy.pem. +# A different user must edit the literal --home path below. +# PYTHONPATH selects checkout modules for `sp container exec`; `sp run` rewrites +# it in the launch snapshot. See workflow/bin/sp's launch code snapshot note. +# +# @sc [label:coupling] nibi-static-container-paths +# apptainer-args must use literal paths: YAML does not interpolate variables, +# and Snakemake escapes `$` before the shell sees it. Bind /local, the NVMe +# mount (3.5 TB ext4), so tile.smk::tile_local can derive a campaign/tile path +# beneath /local/scratch. A login-node export cannot supply SLURM_TMPDIR for a +# future job; {resources.tmpdir} resolves to /tmp on nibi, a measured 378 GB +# RAM-backed tmpfs charged to the job's memory cgroup, not to NVMe storage. apptainer-args: "--cleanenv --env OMP_NUM_THREADS=1 --env MALLOC_ARENA_MAX=2 --env PYTHONPATH=/project/def-mjhudson/cdaley/shapepipe-snakemake/src --home /home/cdaley --bind /project --bind /scratch --bind /local" latency-wait: 60 # NFS: wait for outputs to appear after a job @@ -123,35 +50,16 @@ rerun-incomplete: true # re-do jobs left incomplete by an unclean deat show-failed-logs: true printshellcmds: true -# No `keep-incomplete` here. The declared output IS the manifest, so letting -# snakemake delete a failed job's output is exactly the manifest-vs-log -# semantics this workflow wants (completeness.py's docstring argues it), and -# `show-failed-logs` above surfaces the surviving log on the console. +# No keep-incomplete: Snakemake deletes failed manifests and retains logs. +# See completeness.py for manifest/log semantics; show-failed-logs prints logs. -# rerun-triggers: the v9 default MINUS `input`. params/code/mtime fixes still -# propagate — completeness.py writes the manifest only on change, so mtimes move -# only when reality moves. -# -# `input` is dropped because reclamation needs the tile->exposure edge to be -# CONDITIONAL; with the trigger on, that conditional itself reruns every -# finished tile against a reclaimed exposure store. tile.smk's tile_finished -# commentary carries the mechanism and the fixture-t4 job counts. -# -# Nothing this workflow relied on is lost. The two genuinely data-derived input -# sets are covered another way: a changed exposure list arrives through the -# (non-ancient) find_exposures manifest's mtime, and the ngmix chunk count rides -# in params. And clean scheduling is structurally gated to bin/sp (SP_PHASE=compute -# + this profile), so a bare snakemake invocation cannot quietly recombine -# reclamation with the `input` trigger. +# Exclude the input trigger: tile.smk::tile_finished conditionally removes +# reclaimed exposure edges, and that change must not rerun finished tiles. +# Exposure-list changes propagate through find_exposures manifest mtimes; +# ngmix chunk counts are params. See tile.smk for reclaimed-edge handling. rerun-triggers: [mtime, params, code, software-env] -# NO set-threads / set-resources here, deliberately. Profile overrides REPLACE a -# rule's own values (verified snakemake 9.23), which would kill the -# attempt-scaled `mem_mb = lambda wc, attempt: ...` OOM retries and the tuned -# ngmix thread count, and would flatten the group resource composition. The -# RULES own threads and resources; this profile only sets defaults for rules -# that state nothing (default-resources above). -# -# `group:` fusion of the short rules (PRD D-profile) lives in the rule files, -# each label documented in its file's docstring: `tile_prep` (prepare.smk), -# `exp_short` (exposure.smk) and `tile_shape` (tile.smk). +# No set-threads/set-resources: overrides replace rule values (Snakemake 9.23), +# including attempt-scaled memory and ngmix threads. Rules own resource sizing +# and grouping; this profile supplies defaults only. See prepare.smk for +# tile_prep and tile.smk for tile_shape; exposure rules are ungrouped. From f9af15d64565f7205f6d3ed0ad865ebbc9c1ccc0 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:47:06 +0200 Subject: [PATCH 25/43] docs(config): present-tense comments in workflow/config.yaml Co-Authored-By: GPT-6.1 Sol --- workflow/config.yaml | 128 ++++++++----------------------------------- 1 file changed, 24 insertions(+), 104 deletions(-) diff --git a/workflow/config.yaml b/workflow/config.yaml index 99f96daf9..4741dd3ec 100644 --- a/workflow/config.yaml +++ b/workflow/config.yaml @@ -13,20 +13,20 @@ # Main keys, global for all machines -# input_tyle: allowed are data or image_sims +# input_type: allowed are data or image_sims input_type: data # Run name, available as `$run` in the paths below; it also names the # campaign's merged catalogues. Required, and set by the run config. -# run: smk-g6 +# run: my_campaign # Default settings per input_type, can be overridden by a user-defined run config file. # - tile_detection: both run SExtractor on the tile. `unions_catalogue` (data) # also fetches the UNIONS per-tile catalogue and joins the detections to it, # taking its NUMBER (needs the DR6 tile image); `sextractor` keeps # SExtractor's own (required for image_sims) -# - psf_model: allowed are `psfex` (default), `mccd`; `fake` (image_sims only, -# PSF read from psf_dict) +# - psf_model: `psfex` (data default), `fake` (image_sims default, read from +# psf_dict). The Snakefile refuses `mccd`; see refuse_unpersistable_psf. input_types: data: # @sc [decision:detection.tile_detection] @@ -57,7 +57,7 @@ input_types: # - outputs.products_dir: path to final products (default run_dir; one root # needs clean: false and clean_tiles: false) # - outputs.index_db: path to bookkeeping index sqlite file -# - container: TBD +# - container: image fallback; see workflow/scripts/container.py for resolution machines: nibi: @@ -98,98 +98,21 @@ machines: psf_dict: /home/hervas/fhervas/workdir_skills/input/psf_files/Full_psf_dict.pickle container: /n17data/cdaley/containers/shapepipe_im_sims-runtime.sif -# OPTIONAL per-exposure retention: what to carry onto the persistent root ON TOP -# OF the star catalogue's own inputs, before the scratch store goes -# (`exp_persist`, exposure.smk). -# -# WHAT IS ALWAYS KEPT, AND IS NOT A CHOICE HERE: psf_validation, the psfex_interp -# validation catalogue, one per CCD. `star_cat_merge` stacks every one of them -# into /full_starcat_.hdf5, so they are that -# catalogue's PROVENANCE — a merged star catalogue with no per-exposure inputs -# beside it cannot be audited, re-cut, or recomputed after a purge — and they are -# what keeps APPENDING TILES CHEAP, since a tile added next month brings -# exposures whose catalogues must join the existing stack. ~2 MB per exposure: -# ~40 GB and ~40k inodes at DR6 scale, against a ~1 M-inode group quota. That is -# the price of being able to say where the number came from, and it is paid. -# -# SO THIS LIST IS PURELY ADDITIVE, and an empty one is a coherent instruction: -# the tar then holds the star catalogue's inputs and nothing else. -# -# Entries are PRODUCT NAMES — not globs. The catalogue below is the rendering of -# workflow/scripts/persist_exp.py's PRODUCTS table, which is the single source of -# truth for what each name means and what keeping it buys -# (CosmoStat/shapepipe#844); print it any time with +# Optional per-exposure retention, in addition to mandatory psf_validation. +# Entries name products in workflow/scripts/persist_exp.py's PRODUCTS table; +# that file owns product meanings, sizes, glob syntax and additive retention. +# Print the catalogue with: # # workflow/bin/sp container exec python workflow/scripts/persist_exp.py --list-products # -# product glob size/exposure -# -------------- -------------------------- ------------- -# star_selection star_selection-*.fits 24.5 MB -# setools' PRE-SPLIT selection. The only file that answers which -# stars the selection cuts rejected and why; the split samples have -# already lost the rejects. -# star_train star_split_ratio_80-*.fits 19.9 MB -# the 80% TRAINING sample, the stars PSFEx actually fitted. Rows -# duplicate star_selection. -# star_test star_split_ratio_20-*.fits 7.1 MB -# the 20% VALIDATION sample — the positions psf_validation's rows -# correspond to. Rows duplicate star_selection. -# star_stats star_stat-*.txt unmeasured -# setools' per-CCD STAT block: star counts, FWHM mode and cuts. -# The selection's summary without its catalogue. -# psf_model *.psf 2.8 MB -# the PSFEx model itself. Keeping it means the PSF can be -# re-interpolated at ANY position later without rebuilding the -# exposure chain — the single most capability-adding entry here. -# psfex_cat psfex_cat-*.cat unmeasured -# PSFEx's own output catalogue (FITS_LDAC): the per-star FLAGS_PSF -# and CHI2_PSF, i.e. WHICH stars outlier rejection clipped. Not -# recoverable from anything else — the .psf header keeps only the -# LOADED/ACCEPTED counts. -# psf_validation validation_psf-*.fits 2.0 MB -# the psfex_interp validation catalogue, one per CCD: the input to -# the rho/tau statistics, and to the star_cat_merge rule that stacks -# them into the campaign's full_starcat. -# -# A RAW GLOB IS STILL ACCEPTED, as an escape hatch for a file the catalogue does -# not name yet: anything carrying a glob metacharacter or a dot is taken as a -# glob rather than a name (`*.psf` is a glob, `psf_model` is the name for it). -# An unknown NAME is a parse-time error listing the valid ones. -# -# Matches are packed, flat, into ONE uncompressed tar per exposure: -# /exp///psf/.tar, with a manifest listing the -# members (and the product each came from) beside it. One tar rather than loose -# copies because inodes, not bytes, bind on /project. FITS members read straight -# from the tar: fits.open(io.BytesIO(tarfile.open(t).extractfile(m).read())). -# -# WHY COPY RATHER THAN EXEMPT THESE FROM CLEANUP. Reclamation is not the threat. -# run_dir is /scratch and is PURGED on a 60-day window whether or not -# clean_exposure ever ran; products_dir is /project, backed up and not purged. -# The only way a per-exposure product outlives its campaign is to leave the -# filesystem. (Ordering is free: clean_exposure takes the exp_persist manifest -# as an input, so a store is never reclaimed before its keepers are written.) -# -# EDITING THIS LIST IS CHEAP. It rides on exp_persist's `params`, so a change -# reruns the packing (seconds) and NOT exp_psf (four hours per exposure). That -# separation is the whole reason exp_persist is a rule of its own. -# -# THE DEFAULT is psf_model (2.8 MB per exposure on top of psf_validation's 2.0): -# the model that lets the PSF be re-interpolated at any position later without -# rebuilding the exposure chain from VOS, which is the single most -# capability-adding thing an exposure can keep. Add psfex_cat for a production -# run if you want to know which stars PSFEx clipped; the star_* products are for -# selection studies and cost an order of magnitude more. -# -# THIS LIST IS EXPOSURE-SIDE ONLY. Tile-side retention is not configurable: the -# only tile product that persists today is final_cat, written by tile_make_cat -# straight to products_dir. A tile keep list is #844 follow-up. +# The default psf_model preserves the model for interpolation at other positions. +# Add psfex_cat to inspect PSFEx's rejected stars, or star_* products for +# selection studies. Changing this list re-packs live stores without refitting +# PSFs; reclaimed stores cannot supply additional products (see exposure.smk). # -# NOTE ON products_dir DEFAULTING TO run_dir (a fixture or smoke test): the tar -# then lands beside the store on the same filesystem and buys nothing, and -# final_cat and the persist manifests sit inside the trees clean_tile and -# clean_exposure delete. A one-root run is for fixtures and smoke tests only and -# must set clean: false and clean_tiles: false; the Snakefile refuses it -# otherwise. +# @sc [label:hazard] shared-run-products-root-requires-no-clean +# A one-root fixture (products_dir == run_dir) must disable both clean flags: +# otherwise cleanup deletes persistent products. The Snakefile enforces this. persist_exp: - psf_model @@ -205,20 +128,17 @@ clean_tiles: true # Their exposures are rebuilt from scratch if the tile is retried. clean_ignore_tiles: [] -# The ceiling on any rule's mem_mb: nibi's, whose standard compute node is -# 766 GB (192 cores at 4 GB/core), so 750000 leaves room for the OS and slurm's -# own overhead. A candide run never reaches it. A request above the partition -# maximum is a job SLURM never schedules and snakemake never diagnoses: it sits -# PENDING while the campaign looks alive. The two -# campaign-level merges size themselves from the campaign's bytes and will cross -# this at survey scale — the cap turns "never runs" into "runs on the biggest -# node there is", with a parse-time warning saying which rule was capped. +# Memory-request ceiling: 750000 MB leaves OS/SLURM headroom on nibi's +# 766 GB standard nodes. An above-partition request stays PENDING indefinitely. +# Snakefile::capped_mem limits merge requests and warns when sizing exceeds the +# cap; it does not guarantee that the job fits in the capped memory. max_mem_mb: 750000 # ngmix chunks per tile. ngmix_chunks: 8 -# Node-local tile store (vignets, WCS). Unset: the profile's /local/scratch -# bind (nibi NVMe, candide node /tmp). Dense image-sim campaigns on candide -# want a shared directory instead. +# Tile store (vignets, WCS). Unset: the profile's /local/scratch bind +# (nibi NVMe, candide node /tmp). Dense image-sim campaigns on candide can use +# a larger shared directory, trading ngmix read latency for capacity. sp rewrites +# the snapshot's bind source, not the tile path; see workflow/bin/sp. # tile_store_root: /n23data1//shapepipe/tilestore From 32095a0b926308450a422bbb80f5ea57899d95c7 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:47:15 +0200 Subject: [PATCH 26/43] docs(tile): present-tense comments in tile.smk and prepare.smk Co-Authored-By: GPT-6.1 Sol --- workflow/rules/prepare.smk | 21 +- workflow/rules/tile.smk | 854 +++++++++---------------------------- 2 files changed, 204 insertions(+), 671 deletions(-) diff --git a/workflow/rules/prepare.smk b/workflow/rules/prepare.smk index 7e99fe2ae..2273a2fbc 100644 --- a/workflow/rules/prepare.smk +++ b/workflow/rules/prepare.smk @@ -8,19 +8,18 @@ data-derived tile->exposure edge that invocation 2's parse aggregates into the index. Nibi compute nodes have internet, so downloads run in-DAG (no login-node tier). -All three rules carry ``group: "tile_prep"``, so one tile's whole chain is ONE -sbatch instead of three (medians 0:41 / ~0:40 / 0:15 — all far under the -15-minute runtime limit Alliance policy asks us to bundle away, and at DR6 scale three -submissions per tile is a scheduler load out of all proportion to the work). +All three rules carry ``group: "tile_prep"``, so one tile's chain uses one +sbatch submission. Measured stage medians are 0:41 / ~0:40 / 0:15, well below +Alliance's 15-minute bundling threshold; grouping limits scheduler load. Group membership is per connected DAG component and distinct tiles share no edge, so this is exactly one group job per tile, never a cross-tile bundle. Rules stay group-compatible: shell only, no mid-chain localrules, no pipe outputs. Group resource composition (snakemake 9.23, ``GroupResources.basic_layered`` in snakemake/resources.py): jobs are laid out per toposort level; within a level -non-additive resources (mem_mb, cpus) SUM — split into layers when a global -constraint is exceeded, the group's width being the widest layer — while the -additive resource ``runtime`` is maxed within a layer and SUMMED across layers. +non-additive resources (mem_mb, cpus) sum, with layers split when a global +constraint is exceeded and group width set by the widest layer. The additive +resource ``runtime`` takes the maximum within each layer and sums across layers. This chain is strictly linear, one job per level, so the group asks for max(mem_mb) = 8000*attempt, max(threads) = 4 and sum(runtime) = 150 min. Attempt scaling survives grouping: ``GroupJob.attempt``'s setter clears the @@ -29,12 +28,8 @@ cached group resources and re-sets ``attempt`` on every member (jobs.py), and ``retries: 2`` still governs. A retry re-runs the whole group, which is safe because every rule ``rm -rf``s its own run dir at start. -There is no masking node in this phase, or in any other: ShapePipe generates no -masks (PR #847). The instrument flag image ships with the exposure and is split -per CCD by ``exp_split``; the sky-fixed healsparse masks are queried per object -inside the ShapePipe configs (``mask_query`` on exposures, ``make_cat`` on -tiles). Nothing is fetched, staged or rasterized, so there is nothing to -prepare. +This phase has no masking node. See exposure.smk for instrument flags and +config_tile_Mc.ini for tile mask queries; neither requires tile preparation. """ # No NUMBER_LIST for get_images — a download stage has nothing on disk to diff --git a/workflow/rules/tile.smk b/workflow/rules/tile.smk index 3fcf48268..795e14ec8 100644 --- a/workflow/rules/tile.smk +++ b/workflow/rules/tile.smk @@ -4,38 +4,28 @@ tile_merge_headers -> tile_detect -> tile_vignets -> tile_ngmix x N -> tile_merge_cats -> tile_make_cat -The DAG edge to the exposures is always the exposures' MANIFESTS, looked up -through the index (TILE_EXP). The per-tile exposure "forest" — a symlink view of -exactly this tile's exposures' products — exists only so the ShapePipe configs -have one deterministic ``$SP_EXP`` to glob; it is NEVER the edge. Its 2-char -shard level is not cosmetic: ``exp_utils.get_exp_output_files`` hardwires -``///output/run_sp_*`` into its glob, so a flat forest -makes every tile gather stage fail "No split_exp_runner output found". +Exposure manifests, looked up through the index, provide the tile-to-exposure +DAG edges. The per-tile exposure "forest" is a symlink view for ShapePipe's +``$SP_EXP`` globs, not the exposure dependency. See build_forest.py and +``exp_utils.get_exp_output_files`` for the required sharded layout. -All rules are group-compatible (shell only, no mid-chain localrules), and the two -short regions are grouped, per the composition rules in prepare.smk's docstring. -Distinct tiles share no edge, so each is one group job per tile: +The post-chain uses shell rules with no mid-chain localrules. Each connected +component within a group label is one tile, so each group job covers one tile. +See prepare.smk's docstring for Snakemake's group resource composition: * ``group: "tile_gather"`` — tile_exp_forest (2 GB, 20 min) and tile_merge_headers (median 0:38; 8 GB, 4 threads, 120 min). The group asks max mem_mb = 8000*attempt, max threads = 4, sum runtime = 140. tile_detect also - consumes the forest, but a consumer OUTSIDE the group is just an ordinary DAG - edge on the group job — it does not pull tile_detect (16 GB) in. + consumes the forest, but that edge does not pull it into the group. * ``group: TILE_GROUP`` ("tile_shape") — tile_vignets -> 8 x tile_ngmix -> - tile_merge_cats -> tile_make_cat, THE WHOLE SHAPE CHAIN AS ONE JOB. This is - not a latency optimisation; it is what lets the 5.6 GB vignette store live on - node-local NVMe and never touch /scratch (see tile_local() below). - Composition, verified against snakemake 9.23.1 and a real sbatch: + tile_merge_cats -> tile_make_cat in one allocation, so the vignette and WCS + stores can stay node-local (see tile_local() below). + First-attempt composition, verified against snakemake 9.23.1 and sbatch: cpus = max over levels of summed siblings = max(8, 8x1, 8, 8) = 8; - mem_mb = max(32000, 8x5000, 16000, 16000) = 40000, so nibi's - max(cores, mem_GB/4) bills the group 10 core-equivalents rather than the 28 - it billed at 8x14000 (see the mem_mb note on tile_ngmix for the 31-tile - measurement that sized it); tile_vignets' own 32000 is now the second term - and becomes binding if the chunks ever go below 4000; - runtime = sum along the chain of each level's MAX = 20 + 120 + 10 + 15 = 165. - 165 min is under the 180 min ceiling of ``cpubase_bycore_b1``, so the fused - job reaches the widest partition set (plus cpubackfill) — which is why each - member's runtime is measured p99 plus margin and not the old ceiling. + mem_mb = max(32000, 8x5000, 16000, 16000) = 40000; + runtime = sum of each level's maximum = 20 + 120 + 10 + 15 = 165 min. + See tile_ngmix's resource notes for the measurements behind these budgets. + The 165-minute total fits ``cpubase_bycore_b1``'s 180-minute ceiling. tile_detect stays out: the shape chain does not need it co-scheduled, and folding it in would add its runtime to a sum that has no room. @@ -46,105 +36,35 @@ detection, windowed positions, VIGNET neighbour marking and columns. ``tile_detection: unions_catalogue`` (config.yaml, the data default) adds one rule and one step: tile_get_catalogue fetches the UNIONS per-tile catalogue (get_images_runner, config_tile_Gic.ini), and tile_detect joins its SExtractor -rows to it before the multi-epoch post-processing (MATCH_CATALOGUE, set -through SP_MATCH_CATALOGUE). Each row pairs with its mutual nearest catalogue -object within 1 px and takes its NUMBER, from which make_cat builds -``TILE_UNIQUE_ID``, the key shared with the photometry and photo-z -catalogues; unpaired rows leave the catalogue. The catalogue is SExtractor run -by MegaPipe on the same DR6 image with the same configuration, so the join is -exact; on other pixels (a DR5 image) it fails loudly. - -There is no `tile_mask` rule, and there will not be one (PR #847). ShapePipe -generates no masks: tiles have no instrument flag image of their own, so -tile_detect runs SExtractor with FLAG_IMAGE = False against -default_noimaflags.param (config_tile_Sx.ini — what used to be the "sx_nomask" -variant, now the only one). Sky-fixed masks reach the tile as CATALOGUE columns -instead: ``tile_make_cat``'s make_cat queries the configured healsparse maps at -every object's (RA, Dec) and writes one ``MASK_`` column per band, which -is what downstream selections cut on. +rows to it before multi-epoch post-processing (MATCH_CATALOGUE, set through +SP_MATCH_CATALOGUE). See config_tile_Sx.ini and sextractor_runner's catalogue +matching implementation for the join contract and shared object identifiers. + +There is no `tile_mask` rule. Tiles have no instrument flag image of their own, +so tile_detect runs SExtractor with FLAG_IMAGE = False against +default_noimaflags.param; see config_tile_Sx.ini for the detection settings. +Sky-fixed mask columns are configured in config_tile_Mc.ini and written by +make_cat for downstream selection. """ -# --- the node-local tile root (the I/O + scratch fix) ---------------------- -# -# MEASURED PROBLEM. tile_ngmix was I/O-bound, not CPU-bound: median 7h34m -# elapsed against 52 min of CPU (3% efficiency, 0.12 of 4 reserved cores). -# Each chunk read ~20 GB at 0.78 MB/s and all 8 chunks of a tile re-read the -# SAME 5.6 GB vignette store -- ~163 GB of small random sqlite preads per tile -# against /scratch, which on nibi is NFS (VAST, 4.5 PB, 95% full). What hurts -# is per-read LATENCY, not bandwidth: the sequential local-vs-scratch gap is -# only ~3x and is NOT the argument for this change. -# -# THE FIX IS THE FUSE. tile_vignets -> 8 x tile_ngmix -> tile_merge_cats -> -# tile_make_cat run as ONE group job on ONE node, and the vignette store is -# WRITTEN to that node's NVMe and never lands on /scratch at all. So the store -# is not copied, it is simply never remote: zero staging cost, zero NFS random -# reads, and -- the reason this was chosen over per-chunk staging -- the -# per-tile scratch high-water drops by the whole 5.6 GB store (~190 GiB across -# 34 concurrent tiles). Scratch quota, not speed, is what caps batch size. -# -# WHAT STAYS ON SHARED STORAGE, and it is everything that is DAG currency: -# every manifest (the success sentinels), every verdict log, the ngmix chunk -# dirs merge_sep_cats gathers, and final_cat on the PERSISTENT root. Only the -# bulk intra-tile intermediate is node-local. That split is possible because -# ShapePipe's configs set input and output paths independently -- see -# config_tile_PiViVi_.ini (OUTPUT_DIR = $SP_VIGNET_OUT) and -# config_tile_Ng_template.ini and config_tile_Mc.ini ($NGMIX_VIGNET_DIR), and -# config_tile_Ng_template.ini alone for $SP_WCS_DIR. -# -# TWO node-local stores, not one: the ~5-8 GB vignette store and the 11.3 MB -# WCS sqlite, the same fix applied at the two ends of the size distribution -# (tile_local()'s docstring carries the thread-state measurement). -# -# WHY IT NEEDS THE GROUP. The store is node-local, so it exists only on the -# machine that wrote it and only while that job lives. Unfused, tile_vignets and -# tile_ngmix are different jobs on (usually) different nodes, and the store would -# have to be on shared storage -- which is the whole problem. Fused, all members -# run in one allocation on one node, so the store the first member writes is -# simply there for the rest. (Verified live: group members share the directory.) -# -# HOW THE PATH GETS IN HERE. Not through the environment: profiles/nibi passes -# --bind /local and tile_local() below DERIVES the path from the tile wildcard. -# Why nothing can be communicated instead is on that profile line. A campaign -# that needs the store elsewhere (candide's 31 GB /tmp) moves the BIND, via the -# run config's `tile_store_root` (bin/sp), never the path here -- see below. -# -# THE COST WE ACCEPT: a failure anywhere in the tile re-runs the WHOLE tile, -# not one chunk, because the store dies with the job. At ~1 h per fused tile -# that is a cheap trade for the scratch it buys. Noted, not engineered around. -# -# It lives in each rule's `pre_run`, i.e. in THAT RULE's params.pre. The shared -# prologue (unit_pre) and completeness.py are untouched by design: both are -# fingerprinted into every rule, and changing either would invalidate the whole -# campaign, including the 117 finished exposure chains. -# -# EDITING THIS FUNCTION INVALIDATES FINISHED TILES. READ THIS BEFORE YOU DO. -# The returned string lands in `params.pre`, `params` is an active rerun trigger -# in profiles/nibi/config.yaml, and `params.pre` is a non-derived param (its -# lambda takes only `wc`), so snakemake records it and compares it. Any edit -# here therefore reschedules tile_vignets, all eight tile_ngmix chunks and -# tile_make_cat for EVERY tile the campaign already finished. -# -# On a fresh campaign that is free. On a RESUME it is destructive, and the -# mechanism is worth spelling out because it is not obvious: -# * clean_exposure has already reclaimed the exposure stores those tiles read, -# and tile_finished() deliberately drops the manifests of a finished tile's -# exposures, so the rerun is UNSATISFIABLE -- PiViVi runs against dangling -# symlinks and the group fails; -# * a failed group job's postprocess(error=True) fans out over every member in -# every toposort level and removes each member's EXISTING outputs. One of -# those is tile_make_cat's final_cat on the persistent root. So the failure -# deletes the science product of a tile that was finished and correct; -# * with final_cat gone, tile_finished() flips and the tile re-declares its -# whole exposure edge set, rebuilding the reclaimed chains from VOS. That is -# the rerun avalanche D5 exists to prevent, arriving through the params -# trigger instead of through inputs. -# So: land changes to this function BETWEEN campaigns. An edit cannot reach a -# RUNNING one (the launch code snapshot, bin/sp) -- the hazard is the `sp run` -# after it, which is a resume against finished tiles. If that resume is genuinely -# needed, run it once with -# `--rerun-triggers mtime code software-env`, accepting that clean_exposure's -# consumer-set staleness detection (which rides on params) is off for that -# invocation. +# --- node-local tile stores ----------------------------------------------- +# The shape group writes its vignette store on one node and keeps every reader +# in that allocation. This avoids staging and ~163 GB of random NFS reads per +# tile, and saves ~5.6 GB of shared scratch per tile (~190 GiB for 34 tiles). +# Measured on nibi with remote stores: median 7h34m elapsed versus 52 min CPU, +# with ~20 GB read per chunk at 0.78 MB/s. Per-read latency dominates; the +# sequential local/shared bandwidth ratio is only ~3x. +# +# Manifests, verdict logs, chunk directories and final_cat stay on shared +# storage as DAG outputs. The node-local paths are wired through +# config_tile_PiViVi_.ini, config_tile_Ng_template.ini and +# config_tile_Mc.ini. tile_local() also stages the WCS sqlite (see its docstring). +# A group retry rebuilds the stores; they cannot be inherited by another job. +# +# EDITING PROLOGUE OUTPUT CAN DELETE FINISHED CATALOGUES ON RESUME. +# tile_local() contributes to params.pre. See workflow/CONTRACTS, +# unit-pre-changes-at-campaign-boundary, for the rerun hazard and pinned params. +# Change emitted prologues only at a campaign boundary on a fresh root. # Per-campaign prefix of the node-local store name; see tile_local(). LOCAL_TAG = hashlib.sha1(str(RUN_DIR).encode()).hexdigest()[:8] + "-" @@ -152,75 +72,31 @@ LOCAL_TAG = hashlib.sha1(str(RUN_DIR).encode()).hexdigest()[:8] + "-" def tile_local(tile): """The node-local prologue, as bash, for one tile. - A FUNCTION of the tile rather than a constant, because the path has to be - literal by the time apptainer sees it. Nothing can be COMMUNICATED into the - container -- `$SLURM_TMPDIR`, `--env`, `APPTAINERENV_*` and - `{resources.tmpdir}` all fail, one of them in production; the post-mortem is - on the `--bind /local` line of profiles/nibi/config.yaml. - - So the path is derived instead. `/local/scratch` on nibi is - `drwxrwxrwt root:root` -- world-writable with the sticky bit, verified in - the container (probe job 20798618) -- and the tile id is a wildcard - snakemake substitutes at DAG time, so the shell string carries a concrete - path with no `$` left for anything to escape. One group job per tile means - the name cannot collide within a campaign; the sticky bit means nobody else - can remove it. Across campaigns the tile alone would collide: the - image-simulation shear branches and two-arm A/B data runs are concurrent - campaigns over the SAME tile IDs, and nothing keeps their same-tile jobs off - one node (on candide `/local/scratch` is the node's shared `/tmp`). Two - campaigns' fused jobs on one node would share one store -- a second - `tile_vignets` wipes and rewrites it under the first campaign's ngmix, and - the first `tile_make_cat`'s EXIT trap deletes it under the second (seen on - candide n09, between image-sim branches). So every store name is prefixed - with LOCAL_TAG, a hash of the run dir, and is unique per campaign and tile. - - What we give up is Slurm's own cleanup of `$SLURM_TMPDIR`. TILE_VIGNET_FRESH - reclaims a stale directory on the campaign's next attempt at the tile, the - trap in the last group member removes it on the way out, and the sweep below - catches what a hard kill leaves behind -- on a shared node that last one is - manners, not housekeeping. - - NOT `/tmp`: inside the container that is a 378 GB tmpfs, i.e. RAM charged to - the job's cgroup, so a 5.6 GB store there would be paid for twice. - - TWO STORES, AND THE SECOND IS 11 MB. `$SP_WCS_DIR` holds one file, - `log_exp_headers-.sqlite`, and staging it is the rest of the same fix - rather than a refinement of it. Moving the 5.6 GB vignette store off NFS - bought 4.3x and did not solve the problem: thread-state sampling of the - fused job (3,200 samples across 8 chunks, campaign smk-g4 job 20799387) put - every chunk at 56% on CPU and 44% in `rpc_wait_bit_killable`, with 147 of - 176 D-state samples inside NFS RPC and exactly one hot NFS file still open. - ngmix reads that file once per object PER EPOCH for the WCS, so what costs - is the operation count and not the byte count -- a network round trip per - read. The two stores are the same intervention at the two ends of the size - distribution, and the small one is worth ~1.8x on the rule that is 99.3% of - tile-side core-hours. - - A copy, never a symlink: a link resolves straight back to NFS. The prologue - runs in `tile_vignets` and in each of the eight `tile_ngmix` chunks (not in - `tile_merge_cats`, which has no pre_run), so the staging is attempted nine - times per tile, eight of them concurrent siblings in one toposort level. It - is copy-to-a-temp-name plus `mv -f` -- an all-or-nothing publish, so a - reader never sees a partial file -- and it is UNCONDITIONAL, - no `cp -u` and no already-there test. An interrupted `cp` leaves a truncated - destination whose mtime is NEWER than the source, so `-u` would skip it - forever, and nothing downstream would catch it: TILE_VIGNET_FRESH and - TILE_VIGNET_REQUIRED both look only at the vignette store, and a truncated - WCS store reaches ngmix as wrong astrometry rather than as an error. Nine - unconditional copies of 11 MB per tile is not a cost worth reasoning about; - a staleness rule would be. Renaming over a file a sibling chunk already has - open is safe -- the open descriptor keeps the old inode, identical in - content. - - `tile_merge_headers` is upstream of the whole group, so the file exists by - the time any member runs; failing loudly if it does not is correct, because - the alternative is ngmix silently reading a different tree. + Derive a literal path from the tile wildcard and LOCAL_TAG (the run-dir + hash), so concurrent campaigns over the same tile do not share a store. + See profiles/nibi/config.yaml's /local bind note for container path handling + and bin/sp's tile_store_root handling for the host-side bind. + The container's /tmp is RAM-backed; these stores belong on local disk. + + The WCS store is small (11.3 MB), but ngmix reads it once per object per + epoch. With local vignettes and remote WCS, 3,200 thread-state samples + across eight chunks on nibi show 56% CPU and 44% rpc_wait_bit_killable; + 147 of 176 D-state samples are in NFS RPC. Staging WCS avoids those + per-read round trips, with a measured ~1.8x gain for ngmix. + + Copy WCS unconditionally to a temporary name, then rename it atomically. + A symlink would still read NFS; cp -u could preserve a truncated file with + a newer mtime. Concurrent group members can safely publish identical + copies: an open reader keeps its inode. tile_merge_headers is upstream, + so a missing source is an error rather than a reason to read another tree. + + tile_vignets clears its stale vignette run directory before writing, and + tile_make_cat removes the local root on exit. The age-based sweep below + removes this user's matching directories older than one day; SIGKILL can + leave stores behind because it bypasses the exit trap. """ - # `log_exp_headers--.sqlite`, named from the tile in ShapePipe's - # dashed image-number form -- spelled out rather than globbed so a missing - # file is one precise error instead of an empty expansion. unit_num() owns - # that convention and supplies the SEPARATING DASH itself, so there is none - # in the literals below. + # unit_num() supplies the leading dash and dashed tile ID. Use an exact + # WCS filename so a missing source produces a precise error, not an empty glob. tile_num = unit_num(tile) return f''' if [ ! -d /local/scratch ]; then @@ -255,81 +131,47 @@ find /local/scratch -maxdepth 1 -name 'sp-*.*' -user "$(id -u)" -mmin +1440 \ ''' -# Every member of a group job must agree on a STRING resource or snakemake -# raises "Resource slurm_extra is a string but not all jobs in group require -# the same value" (resources.py::_is_string_resource). So the tmp-disk request -# is one constant on all four members, not a property of tile_ngmix alone. -# -# --tmp is a node SELECTION FLOOR, not a reservation: Slurm matches it against -# the node's configured TmpDisk (nibi: 1.67-12 TB, TmpFS=/local) and does not -# decrement it per job. It buys exactly one thing, and it is the thing worth -# having -- the fused job can no longer land on a node with no usable local -# disk, which is the one configuration in which the whole design fails. 16 GB -# against a measured 5.6 GB store leaves room for an unusually rich tile, and -# filters no nibi node today, so it costs no queue time. Resources are not a -# rerun trigger, so adding it invalidates nothing. +# All group members must use the same string resource (Snakemake's +# resources.py::_is_string_resource), so each uses this constant. +# Slurm's --tmp selects nodes by configured TmpDisk; it does not reserve or +# decrement disk space per job. 16 GB leaves room above the measured 5.6 GB +# store and excludes nodes configured without sufficient temporary disk. TILE_SLURM_EXTRA = "--tmp=16000" -# One label for the whole shape chain; the composition arithmetic is in this -# file's docstring. The group's declared runtime is a SUM along the chain and -# that sum gates partitions on nibi, so each member's runtime below is measured -# p99 plus margin, not the old defensive ceiling. +# One label for the whole shape chain; see this file's docstring for resource +# composition. The summed runtime determines eligible partitions on nibi. TILE_GROUP = "tile_shape" -# Cleanup, on the LAST member only. $SLURM_TMPDIR is Slurm's to reclaim, but -# /local/scratch on nibi demonstrably carries stale directories from -# long-finished jobs, so the epilog is not on its own reliable. tile_make_cat is -# the last reader of the store, so its EXIT trap is the earliest moment the -# 5.6 GB can go. It must NOT be set by the earlier members: their shells exit -# while the store is still needed. Residual failure mode accepted: SIGKILL -# (OOM-kill, node death) skips the trap, and if the epilog also misses it the -# store leaks until the node drains -- 5.6 GB against a 3.3 TB disk. +# Set cleanup only on tile_make_cat, the last store reader. Earlier members' +# shells exit while other members still need the store. See tile_local() for +# cleanup after a hard kill. TILE_CLEAN = r""" trap 'rm -rf "$SP_LOCAL"' EXIT """ -# tile_vignets ONLY. unit_pre clears each stage's own run dir on the shared root -# ("the job clears its run dir at start" — ShapePipe's FileHandler raises on an -# existing one); the node-local root needs the same treatment, and only the rule -# that WRITES the store may do it. Putting this in TILE_LOCAL would have -# tile_ngmix delete the store it is about to read. +# Only tile_vignets may clear the vignette run directory before writing. +# Putting this in tile_local() would make readers delete their own input. TILE_VIGNET_FRESH = r""" rm -rf "$SP_VIGNET_OUT/run_sp_tile_PiViVi" """ -# THE SINGLE WRITER OF THE ngmix CHUNK PARTITION, and tile_vignets ONLY. -# -# The ranges are materialised once per tile, here, and every chunk then looks -# its own row up (TILE_NGMIX_RANGE_READ below). tile_vignets is the first member -# of the fused group and strictly precedes all chunks in its DAG, so this is the -# one place in the group where a single process can write something all chunks -# read — no flock, no first-one-wins race. The sexcat it reads is tile_detect's, -# upstream of the whole group and on the SHARED root ($SP_RUN); only the OUTPUT -# is node-local. -# -# Group-internal plumbing, deliberately not a rule output: the file lives and -# dies with the group job exactly as the vignette store does, so a chunk can -# only ever read what its own group job wrote. ngmix_range.py owns the rest. +# tile_vignets writes the partition once, before all chunks; each chunk reads +# its row rather than recomputing boundaries. A measured mid-run splitter edit +# with independent chunk splitting orphaned 520 objects and measured 831 twice; +# merge_sep_cats would accept that inconsistent partition without error. +# The input sexcat is on the shared root; the ranges are group-local plumbing, +# not a rule output. See ngmix_range.py for the partition algorithm. NGMIX_RANGES = "$SP_LOCAL/ngmix_ranges.json" TILE_NGMIX_RANGES = ( f'python {SCRIPTS}/ngmix_range.py --run-dir "$SP_RUN" ' f'--n-chunks {NGMIX_CHUNKS} --write "{NGMIX_RANGES}" || exit 1' ) -# THE FUSE'S ONE SHARP EDGE, made loud rather than mysterious. -# -# The vignette store is not a declared output any more, so snakemake cannot see -# it. tile_vignets' manifest CAN be up to date while the store does not exist on -# this node — concretely, after a fused group job dies part-way: the manifest is -# a success and survives, so a LATER invocation re-plans the group with only the -# surviving members and the store is simply not there. (An in-flight `retries:` -# resubmission is safe: it reruns the same member set, tile_vignets included.) -# -# Recovery is one line, and the message says it: delete the tile's -# tile_vignets.json and resume — that puts tile_vignets back in the group and -# the store comes back with it. The alternative fixes are worse: temp()-ing the -# vignets manifest would break clean_exposure, which keys reclamation -# eligibility on exactly that file. +# Snakemake tracks the manifest, not the node-local store. After a partial +# group failure, a resume can omit tile_vignets because its manifest exists, +# leaving readers without a store. Delete tile_vignets.json and resume to put +# the writer back in the group. An in-flight retry keeps the original member set. +# Keep the manifest non-temp: clean_exposure uses it to establish eligibility. TILE_VIGNET_REQUIRED = r""" if [ ! -d "$NGMIX_VIGNET_DIR/vignetmaker_runner_run_2/output" ]; then echo "tile_shape: the node-local vignette store is missing." >&2 @@ -346,15 +188,11 @@ fi -# THE MODULE LOGS OUTLIVE THE RUN DIRS THAT HOLD THEM. A chunk's ShapePipe run -# dir is temp() (tile_merge_cats is its last reader) and tile_vignets' lives in -# the node-local store, so both kinds of log went with their dirs: the -# run's own logs/ and each module's logs/process-*.log, where ngmix writes its -# per-chunk `epoch cuts:` line and every per-object "ngmix failed" message. -# The rule copies them to $SP_RUN/logs/modules// (the tile dir on the -# shared root) before it exits, whatever its rc, so a failed chunk keeps its -# evidence too. A few KB per chunk; clean_tile reclaims them with the rest of -# logs/. The copy never changes the rule's exit status. +# Preserve run and module logs before temporary or node-local directories go. +# This includes ngmix's epoch-cut counts and per-object failure messages. +# Each rule copies logs to the tile's shared logs/modules// even on +# failure; a copy failure does not change the rule's exit status. clean_tile +# reclaims these logs with the rest of the tile store. def keep_module_logs(run_root, run_name): dest = f'"$SP_RUN/logs/modules/{run_name}"' return ( @@ -368,60 +206,22 @@ def keep_module_logs(run_root, run_name): def tile_exp(wc): return tile_exposures(wc.tile) -# --- the tile->exposure edge, and why it is cut for finished tiles ---------- -# -# THE cascade fix. clean_exposure deletes the exposure's manifests on purpose: -# that is what makes a tile appended later rebuild the chain instead of running -# against an empty store. But those manifests are the tile side's inputs, and an -# exposure is read by ~7-10 tiles. So the moment ONE tile's chain rebuilt an -# exposure, every other tile reading it saw "input files updated by another job" -# and reran — and that rerun rebuilt ITS exposures, which reran THEIR other -# consumers, propagating across the whole exposure-overlap connected component. -# On fixture t4, asking for one damaged tile scheduled all four tiles' chains. -# -# Two mechanisms, and only the second one actually cuts it: -# -# 1. ancient() on every exposure-manifest edge. Correct on its own terms — a -# tile has no business rerunning because an exposure manifest is NEWER — and -# it is what keeps a pure-mtime disturbance (a re-touched manifest, a -# restored backup) from waking finished tiles. But ancient() governs -# TIMESTAMPS only. Snakemake propagates "my input is produced by a job that -# will run in this DAG" separately, and ancient does not suppress it -# (measured: t4 counts were identical with ancient alone). -# -# 2. Cutting the RECLAIMED edges of a FINISHED tile — the mechanism that works. -# A tile whose final_cat is on disk needs nothing further from its -# exposures: it has already extracted everything it will ever read. So for -# such a tile the input list drops the manifests that are GONE, and the -# propagation has nowhere to go. An UNfinished tile keeps its full edge set -# and therefore still drags in — and rebuilds — every exposure it needs, -# which is the accepted price of a late append, unchanged. -# -# Only the missing ones are dropped, never a manifest that still exists: a -# campaign that has cleaned nothing then declares exactly the edges it always -# did, and the cut cannot perturb it. -# -# The marker is final_cat, not the tile's own vignets manifest: on a tile whose -# catalogue was lost, the vignets manifest still exists while the vignette store -# (temp()) does not, so keying on vignets would cut the edge on exactly the tile -# that has to rerun, and run it against a deleted exposure store. -# -# THE CUT REQUIRES THE `input` RERUN-TRIGGER TO BE OFF (profiles/nibi sets the -# trigger list). Dropping an input is itself a change in the set of input files, -# which that trigger reads as a reason to rerun — reinstating the very cascade, -# now as "Set of input files has changed", and running finished tiles against a -# store that is gone. Measured on fixture t4, one damaged tile of four: 82 jobs -# with neither fix, 70 with the cut but the trigger on, 28 with both (= exactly -# the damaged tile's own chain, its two exposures, and the clean jobs). -# -# The cost, stated plainly: `--forcerun` on a tile whose final_cat exists will -# NOT rebuild its reclaimed exposures, because those edges are not in the DAG. -# Delete that tile's final_cat first and the whole chain comes back. -# -# What none of this weakens: clean_exposure's own inputs are neither ancient nor -# cut, so a tile that really did rebuild its vignets still reschedules the cleans -# of the exposures it read, and a grown consumer set still travels through -# params.consumers. +# --- tile-to-exposure edges after reclamation ----------------------------- +# A tile with final_cat needs no reclaimed exposure store. Drop only missing +# exposure manifests for finished tiles, so rebuilding a shared exposure for +# one tile does not propagate reruns through the overlap component. Unfinished +# tiles keep every edge and rebuild the exposures they need. ancient() suppresses +# timestamp changes, but cannot suppress reruns propagated from upstream jobs. +# +# The input rerun trigger must be off (see the profiles' trigger lists): it +# treats the deliberate edge removal as a reason to rerun. On a four-tile +# fixture with one damaged tile, job counts are 82 without these protections, +# 70 with edge removal but the input trigger on, and 28 with both protections. +# +# Use final_cat as the marker, not tile_vignets.json: the manifest can survive +# without the node-local store. To rebuild reclaimed exposures with --forcerun, +# delete the tile's final_cat first so its exposure edges return to the DAG. +# See clean_exposure in exposure.smk for reclamation and consumer-set tracking. def tile_finished(tile): return Path(final_cat(tile)).exists() @@ -439,7 +239,7 @@ def tile_exp_all(wc): return tile_exp_split(wc) + tile_exp_psf(wc) # Build the per-tile symlink forest. Declaring the exposure manifests as input # makes this wait on its exposures; the forest itself is only the $SP_EXP view. -# Its output stays a directory() (it has no ShapePipe run dir and no manifest — +# Its output is a directory() (it has no ShapePipe run dir and no manifest — # it is not a shapepipe_run at all). rule tile_exp_forest: group: "tile_gather" @@ -458,7 +258,7 @@ rule tile_exp_forest: runtime = 20 shell: # --forest {output} lives in the shell string: snakemake formats shell - # ONCE, so an {output} placeholder inside params.cmd would survive + # once, so an {output} placeholder inside params.cmd would survive # literally and every forest job would race one './{output}'. "{params.cmd} --forest {output.forest}" @@ -470,11 +270,8 @@ rule tile_merge_headers: input: forest = rules.tile_exp_forest.output.forest, split = tile_exp_split, - # config_tile_Mh_exp.ini reads run_sp_tile_Fe output, which the PREPARE - # phase produced. Declaring the Fe manifest gives the COMPUTE DAG a - # regeneration path for it instead of a silent dependency on a - # previous invocation (prepare.smk is included in every parse, so the - # rule exists here too). Normally a satisfied no-op. + # config_tile_Mh_exp.ini reads the prepare phase's run_sp_tile_Fe output. + # Its manifest provides a regeneration edge in the compute DAG. fe = f"{TILE_DIR}/manifests/tile_find_exposures.json", output: manifest = f"{TILE_DIR}/manifests/tile_merge_headers.json" @@ -552,23 +349,12 @@ rule tile_detect: pre = lambda wc: unit_pre("tile_detect", wc.tile, env=detect_env(wc.tile)), script_hash = SCRIPT_HASH - # One core: the container's SExtractor is built without threads - # (NTHREADS 4 warns and runs in the same 36 s), default_tile.sex sets - # NTHREADS 1, the join and the post-processing are serial Python, and - # `-b {threads}` only sets the SMP batch over input sets, of which a tile - # is one. - # - # MEASURED on eight DR6 tiles chosen to span conditions (high latitude, - # b = 18 deg, the A2199 cluster, Alioth's halo, two survey-edge tiles at - # 96% zero weight; 1-10 exposures), the step as shapepipe_run runs it on - # candide: wall time 23-160 s (SExtractor + join <= 93 s, post-processing - # <= 97 s), peak RSS 1.84 GiB, set by the join holding the ~430 MB - # catalogue twice. Image simulations run the same step on tiles of the same - # size (10000 x 10000 px); SExtractor alone on one takes 34 s and 0.74 GiB. - # 4000 MB is 2.1x the worst tile, and an OOM retries at 8000. The runtime - # is ~7x the slowest tile, for /scratch I/O on nibi (a 400 MB image and - # weight in, a ~430 MB catalogue out). nibi bills max(cores, mem_GB/4), so - # the job bills 1 core-equivalent. + # One core: the container's SExtractor has no threading support, and the + # join and post-processing are serial. See default_tile.sex for NTHREADS. + # Across eight DR6 tiles on candide (1-10 exposures, including low latitude, + # a cluster, a bright-star halo and survey edges), wall time is 23-160 s + # and peak RSS is 1.84 GiB. 4000 MB gives ~2.1x memory margin; 20 min gives + # ~7x wall-time margin for shared-storage I/O on nibi. Both scale on retry. threads: 1 retries: 1 resources: @@ -577,9 +363,8 @@ rule tile_detect: shell: sp_shell("tile_detect", "config_tile_Sx.ini") -# Configured PSF interpolation to galaxies + vignet postage stamps: the last -# stage that reads exposure products, and the bulk intra-tile intermediate. The store it -# writes is node-local (see TILE_LOCAL above). +# PSF interpolation and galaxy postage stamps: the last exposure-product reader. +# The vignette store is node-local (see tile_local()). rule tile_vignets: group: TILE_GROUP input: @@ -591,12 +376,9 @@ rule tile_vignets: # tile_merge_headers above. fe = f"{TILE_DIR}/manifests/tile_find_exposures.json", output: - # THE MANIFEST IS THE ONLY DECLARED OUTPUT. The vignette store used to - # ride along as a second temp(directory()) so native temp() would - # reclaim it; node-local, it cannot be declared at all — and need not - # be. It was never DAG currency, its readers are all inside this group - # job, and TILE_CLEAN's trap reclaims it. Lost affordance: `--notemp` - # can no longer keep it for debugging. + # The manifest is the only declared output. The store is node-local, + # all readers are in this group, and TILE_CLEAN reclaims it regardless + # of --notemp. manifest = f"{TILE_DIR}/manifests/tile_vignets.json", log: f"{TILE_DIR}/logs/tile_vignets.json" @@ -606,245 +388,91 @@ rule tile_vignets: pre_run=[tile_local(wc.tile), TILE_VIGNET_FRESH, TILE_NGMIX_RANGES]), script_hash = SCRIPT_HASH, - # tile_vignets now PRODUCES the chunk partition (TILE_NGMIX_RANGES), so - # it carries the splitter's fingerprint too. Same hash, same constant — - # the Snakefile's NGMIX_RANGE_HASH argues what it guards, and - # tile_ngmix's copy below carries the mid-campaign-edit warning. + # Fingerprint the partition writer and readers together; see the + # tile_ngmix range_hash note and the prologue hazard above. range_hash = NGMIX_RANGE_HASH - # 8, not 16, for the same reason tile_ngmix is 1: `-b {threads}` is SMP - # batch size over input FILE SETS, and a tile is one set -- this run's own - # log says "Batch size: 16 / Total number of processes: 1". 16 was the - # widest member and therefore set the whole GROUP's cpus_per_task; at 8 the - # group asks exactly what the eight-chunk ngmix wave needs. Billing is - # unchanged either way (nibi is MAX_TRES and the 112 GB memory term is - # 28 core-equivalents, well above both), but the group now packs onto a - # node in 8 cores instead of 16. + # Eight threads match the ngmix wave's width, without making this stage + # request a wider group allocation. ShapePipe's batch parallelism is over + # input file sets; a tile supplies one set (see tile_ngmix's thread note). threads: 8 resources: mem_mb = lambda wc, attempt: 32000 * attempt, - # Measured median 3m29s, max 5:25 (this branch: 2m38s on 198.305). - # 20 min is p99 plus a wide margin, and it is a term in the GROUP's - # runtime sum, so the old defensive 240 is not free any more. + # Measured median 3m29s, max 5:25. The 20-minute budget includes + # margin and contributes to the group's summed runtime. runtime = 20, slurm_extra = TILE_SLURM_EXTRA shell: - # The completeness check is pointed at the NODE-LOCAL run root; see + # The completeness check is pointed at the node-local run root; see # sp_shell's check_args for what the two flags do. sp_shell("tile_vignets", f"config_tile_PiViVi_{PSF_MODEL}.ini", check_args=' --run-dir "$SP_LOCAL" --unit {wildcards.tile}', post=keep_module_logs("$NGMIX_VIGNET_DIR", "run_sp_tile_PiViVi")) -# ngmix shape measurement — N chunks per tile (D4). Each chunk LOOKS UP its own -# CLOSED catalogue-row range in the file tile_vignets materialised at the top of this -# group job (TILE_NGMIX_RANGES); the ranges are knowable only at EXECUTION time, -# from this tile's own sexcat, which is why a params function cannot supply them. -# Closed, not open-ended: `ID_OBJ_MAX = -1` on the last chunk was the 13-hour -# straggler's root cause (ngmix treats id_obj_max <= 0 as unbounded). +# ngmix shape measurement: N chunks per tile, each reading its closed row range +# from TILE_NGMIX_RANGES. The tile's sexcat supplies the bounds at execution time, +# so a params function cannot determine them. ngmix treats ID_OBJ_MAX <= 0 as +# unbounded; a closed upper bound prevents the last chunk from overrunning. # -# Chunks write nothing shared: each has its own run_sp_tile_ngmix_Ngu, and +# Chunks write separate directories: each has its own run_sp_tile_ngmix_Ngu, and # merge_sep_cats — DAG-serialised after all chunks — is the gather. rule tile_ngmix: group: TILE_GROUP input: - # The manifest, and only the manifest. The vignette store is no longer a - # declared input because it is no longer a declared output: it is - # node-local, produced by tile_vignets earlier in THIS SAME group job - # (see TILE_LOCAL). The manifest was always the real edge. + # The manifest is the DAG edge; see tile_vignets for store lifetime. vignets = rules.tile_vignets.output.manifest, sx = rules.tile_detect.output.manifest, output: manifest = f"{TILE_DIR}/manifests/tile_ngmix_{{chunk}}.json", - # STAYS ON SCRATCH, unlike the vignette store. It is DAG currency: - # tile_merge_cats reads it, and merge_sep_cats derives chunks 2..N from - # chunk 1's path. It is also small (~300 KB/chunk), so it costs the - # scratch high-water nothing worth chasing. temp() still reclaims it - # once merge_cats has run. + # Shared DAG output (~300 KB/chunk), reclaimed after tile_merge_cats. + # merge_sep_cats derives chunks 2..N from chunk 1's path. chunkdir = temp(directory(f"{TILE_DIR}/output/run_sp_tile_ngmix_Ng{{chunk}}u")), log: f"{TILE_DIR}/logs/tile_ngmix_{{chunk}}.json" params: pre = lambda wc: unit_pre("tile_ngmix", wc.tile, env={"SP_NGMIX_CHUNK": wc.chunk, "NGMIX_N_CHUNKS": NGMIX_CHUNKS}, - # Two steps, not `eval "$(...)"`: a command substitution inside eval - # discards the script's exit status, so a missing sexcat would fall - # through to shapepipe_run with an unset range and fail as something - # else. Capture, check, then eval — the range script fails as itself. - # tile_local FIRST, because it is what exports $SP_LOCAL — and the - # ranges file the lookup reads lives there, written once by - # tile_vignets (TILE_NGMIX_RANGES). This chunk only looks its row - # up; it never recomputes, and the script refuses to. - # - # Two steps, not `eval "$(...)"`: a command substitution inside eval - # discards the script's exit status, so a missing ranges file would - # fall through to shapepipe_run with an unset range and fail as - # something else. Capture, check, then eval — the lookup fails as - # itself. + # tile_local exports SP_LOCAL before the range lookup. Capture and + # check the lookup before eval: eval "$(...)" would discard its + # failure status and let ShapePipe run with unset bounds. pre_run=[tile_local(wc.tile), TILE_VIGNET_REQUIRED, f'ngmix_range_out=$(python {SCRIPTS}/ngmix_range.py ' f'--read "{NGMIX_RANGES}" --chunk {wc.chunk}) || exit 1', 'eval "$ngmix_range_out"']), script_hash = SCRIPT_HASH, - # `pre_run` already puts the INVOCATION in params, but the invocation is - # invariant to the script's body, and what that body decides is a - # PARTITION (the Snakefile's NGMIX_RANGE_HASH argues the corruption). - # Carried on tile_vignets too, since the split moved there. - # - # WHY THE PARTITION IS MATERIALISED ONCE, kept as history because the - # design only reads as over-careful until you have seen this. The eight - # chunks used to each run ngmix_range.py in their own shell and trust the - # others to have landed on the same boundaries. A fused group holds them - # open for hours (smk-g4: 6,236-7,762 s elapsed per chunk), and each chunk - # read the script only when its own shell started -- against the LIVE - # checkout, as every job did before the launch code snapshot (bin/sp) -- - # so an edit landed mid-flight was read by some and not others. - # Seen once, live: an invocation four seconds after a rewrite returned - # chunk 5 of 186.307 as 17649..22060 where the other seven had been split - # 17129..21229 — 520 objects orphaned, 831 measured twice, and - # merge_sep_cats concatenates whatever it is handed, so the tile would - # have completed green. (Twelve sequential and thirty-six concurrent runs - # against a stable checkout gave the identical correct partition; the - # race was purely the edit.) That is now STRUCTURALLY closed: tile_vignets - # writes the ranges once and the chunks only look their row up, so there - # is nothing left for them to disagree about. The hash below no longer - # buys sibling agreement — it buys REPRODUCIBILITY across a resume. - # - # WHAT A ONE-RULE FINGERPRINT DOES MID-CAMPAIGN: IT DELETES SCIENCE - # PRODUCTS. Observed when range_hash lived on tile_ngmix ALONE, which is - # no longer the case -- tile_vignets carries it too now that it is the - # rule that runs the splitter, so an edit drags tile_vignets back into - # the group, the store and the ranges are rebuilt, and nothing trips. - # Kept because the mechanism is general and the next single-rule param is - # one edit away. A new param entry replans the fused group with "Params have changed since last - # execution", scheduling the eight chunks, tile_merge_cats and - # tile_make_cat but NOT tile_vignets or tile_detect (their manifests - # exist, so missing_output never queues them). That is exactly the state - # TILE_VIGNET_REQUIRED catches: every chunk trips the guard, the group - # fails, and GroupJob.postprocess(error=True) removes every member's - # EXISTING outputs -- tile_make_cat's final_cat on the persistent root - # among them. - # - # OBSERVED end to end, not derived: SLURM job 20818649 against smk-g5, - # 2026-08-30, 32 seconds. The dry run scheduled exactly that member set - # (the group's SLURM label came back - # tile_shape_tile_make_cat_tile_merge_cats_tile_ngmix, no tile_vignets); - # every chunk tripped the guard; the group failed; and - # final_cat-186.307.fits was GONE from /project afterwards, the tile's - # directory on the persistent root empty. It had never been seen before - # because "Group jobs: inactive (local execution)" puts it out of reach - # of any login-node fixture. (Restored from a copy taken first; design - # and transcript in sp-products/smk-g5/EXPERIMENT_postprocess_deletion.md.) - # - # WORTH SITTING WITH: the guard is what makes this loud rather than - # silent, and the loud failure is precisely what triggers the deletion. - # Still the right trade -- but it argues for never reaching this state, - # not for relaxing the guard. - # - # THE RULE THE INCIDENT LEFT BEHIND: a fingerprint that changes on the - # chunks but NOT on tile_vignets is the dangerous shape. tile_local() - # was always safe for exactly this reason -- it sits in three rules' - # params.pre, so an edit there pulls tile_vignets in and the store is - # rebuilt. range_hash is now the same shape by construction. job_head.sh's - # runbook sweep would not have helped either: it skips any tile with a - # final_cat, exactly the set this breaks. - # - # INVERTED BY clean_tiles, so read the default carefully. With - # reclamation ON the tombstoned tile has lost tile_detect.json and the - # structural "Input files updated by another job" propagation puts - # tile_vignets back in the group (measured at clean_tile below), so the - # store is rebuilt and nothing trips. The SHIPPED DEFAULT - # clean_tiles: false is the dangerous configuration -- the opposite of - # how reclamation reads everywhere else in this file. - # - # So: land this hash, and every later edit to ngmix_range.py, at a - # campaign boundary on a fresh root. The same rule and direct command as - # tile_local()'s -- `--rerun-triggers mtime code software-env`. + # Hash the script body, not just its invocation, for resume reproducibility. + # Keep this fingerprint on tile_vignets too: a readers-only params change + # can omit the writer, trip TILE_VIGNET_REQUIRED, and make failed-group + # cleanup delete an existing final_cat (observed in a nibi group job). + # See the prologue hazard above and workflow/CONTRACTS, + # unit-pre-changes-at-campaign-boundary, before changing params. range_hash = NGMIX_RANGE_HASH - # ONE core, not four. `-b {threads}` is shapepipe_run's SMP BATCH SIZE - # (pipeline/args.py) -- joblib Parallel(n_jobs=batch_size) over - # filehd.process_list, i.e. parallelism ACROSS INPUT FILE SETS. An ngmix - # chunk is one catalogue, so process_list has exactly one entry: every real - # log says "Batch size: 4 / Total number of processes: 1". There is no - # internal parallelism either (no multiprocessing/Pool/joblib/threading in - # ngmix_package/ngmix.py), and OMP/BLAS are pinned to 1 by both the prologue - # and the profile. The reserved cores 2-4 never had anything to run. - # - # Worth being precise about what this saves. nibi bills - # TRESBillingWeights=CPU=1000,Mem=250G under PriorityFlags=MAX_TRES, i.e. - # max(cores, mem_GB/4). At 14 GB the memory term alone is 3.5 core- - # equivalents, so 4 -> 1 core moves the reservation from 4.0 to 3.5, a 12% - # saving -- NOT 75%. The real core-hour win is the elapsed-time collapse - # from killing the NFS random reads, not this. + # One core per chunk: ShapePipe's -b controls parallelism over input file + # sets (pipeline/args.py), and a chunk is one catalogue. ngmix has no internal + # worker pool; the prologue and profile pin OMP/BLAS to one thread. threads: 1 retries: 2 benchmark: f"{TILE_DIR}/manifests/tile_ngmix_{{chunk}}.benchmark.tsv" resources: - # 5000, down from 14000, and this is the campaign's largest single cost - # saving — but read what the number means before moving it again. - # - # sacct's MaxRSS here is NOT process memory. nibi runs - # JobAcctGatherType=jobacct_gather/cgroup, so it reports the cgroup's - # memory.current, which under cgroup v2 CHARGES PAGE CACHE to the job. - # Cache is reclaimable — the kernel evicts it before it kills anything — - # so the cgroup high-water is an upper bound on what the job NEEDS, not - # a hard requirement. What a tight reservation can still do is squeeze the cache - # that keeps the node-local store resident, which is part of why the - # fused tile is fast. - # - # MEASURED ACROSS 31 TILES (campaign smk-g4, 2026-08-30): cgroup - # high-water 27.40 GiB max, 22.14 GiB mean, against the 109.4 GiB the - # group was reserving. The eight chunks are SIBLINGS in the group's - # toposort so snakemake SUMS their mem_mb; at 5000 the group asks - # 40000 MiB = 39.1 GiB, a 1.43x margin over the worst tile observed. + # Across 31 nibi tiles, cgroup high-water is 27.40 GiB max, 22.14 GiB + # mean. Eight sibling chunks sum to 40000 MiB = 39.1 GiB, a 1.43x + # margin over the measured maximum. nibi's cgroup-based sacct MaxRSS + # includes reclaimable page cache, not just process memory. + # Prefer the larger per-worker memory estimate in config_tile_Ng_template.ini's + # SAVE_BATCH note over the ~1.25 GiB psutil benchmark: 30-second sampling + # can miss flush peaks. That leaves about 20 GiB for the local store's cache. # - # The ANONYMOUS half of that, which is the part that can actually OOM, - # is the number to argue about, and this repo carries two estimates that - # disagree by 1.8x. Snakemake's psutil benchmark says ~1.25 GiB per - # chunk (~10 GiB for eight); config_tile_Ng_template.ini's own - # SAVE_BATCH note says ~2.3 GB per worker from an A/B test (job - # 17607877), i.e. ~18.4 GiB. Prefer the larger: snakemake samples RSS on - # a 30-second grid (BENCHMARK_INTERVAL), and a SAVE_BATCH = 250 flush - # cycle is exactly the sawtooth such a grid misses. So read the margin - # as ~2.1x over anonymous memory with ~20 GiB left for cache against an - # 8.1 GiB store — comfortable, but not the 4x a psutil-only reading - # would suggest. An OOM is also self-healing: mem_mb scales with - # attempt, so a retry asks 80000. - # - # WHY THIS IS THE BIG ONE. nibi bills max(cores, mem_GB/4), so at - # 112 GB the fused group billed 28 core-equivalents for 8 real cores — - # 3.4x, measured live at 905 billed against 266 allocated. At 40 GB it - # bills 10. That is not a saving so much as a schedule: the account's - # fairshare target is ~250 CE, which buys 9 tiles in flight at 28 and 25 - # at 10, and DR6's wall clock is (tiles / tiles-in-flight) x elapsed. - # - # 4000 is the current lower bound and is NOT recommended yet: it would take the group - # to 32000 MiB, where tile_vignets' own 32000 becomes the binding term - # and the group finally bills its 8 real cores — but that is a 1.14x - # margin over the worst tile measured, and the first thing to give would - # be the page cache holding the store. Take it only with a measurement - # of cache behaviour under pressure, not on the arithmetic alone. + # nibi bills max(cores, mem_GB/4): 40 GB costs 10 core-equivalents for + # eight cores. Reducing chunks to 4000 would make tile_vignets' 32000 + # the group limit, only 1.14x the measured maximum; measure cache + # behaviour under pressure before using that budget. Retries scale memory. mem_mb = lambda wc, attempt: 5000 * attempt, - # 120 on the FIRST attempt, and ATTEMPT-SCALED after it. MEASURED - # two ways, and the second is why the margin is thinner than it looks: - # * alone (job 20795277, tile 198.305 chunk 1): ~76 min for 3547 - # objects = 1.29 s/object, against a 7h34m median on NFS -- the - # same ~50 min of TotalCPU either way, so the collapse is pure I/O. - # * EIGHT-WIDE on one node (job 20799387, tile 186.307, 4412 - # objects/chunk): 1.54 s/object steady-state, i.e. concurrency - # costs ~19%, and the chunk lands at ~113 min. The campaign's - # largest tile (198.306, 4678 objects/chunk) projects to ~120 -- - # exactly this number, with nothing left over. - # - # Inside a group SLURM enforces only the GROUP's wall (165 min), never - # a member's, so 120 is a budgeting term rather than a kill line and - # the worst tile still lands ~127 min inside 165. What is NOT safe is a - # flat retry: a group that TIMEOUTs re-queues against the identical - # wall and fails identically, burning three 165-minute allocations to - # learn nothing. Scaling with `attempt` keeps the happy path in - # cpubase_bycore_b1 (20+120+10+15 = 165 <= 180) and gives a retry real - # headroom (285 min, which is b2) instead of a rerun of the same - # failure. Attempt 1 is unchanged, so this does not perturb the - # benchmark -- it only makes the failure branch mean something. + # Measured on nibi: 1.29 s/object for one local chunk, 1.54 s/object + # for eight concurrent chunks (~19% slower). At 4678 objects/chunk, + # the latter projects to ~120 min. Slurm enforces the group's total, + # not this member budget: attempt 1 gets 165 min, attempt 2 gets 285. + # Scale retries so a timeout receives more wall time rather than + # repeating the same allocation; attempt 1 fits cpubase_bycore_b1. runtime = lambda wc, attempt: 120 * attempt, slurm_extra = TILE_SLURM_EXTRA shell: @@ -888,14 +516,12 @@ rule tile_merge_cats: # The run's science product. make_cat also reads the vignette store's # configured PSF-interpolation output, so it — not ngmix — is the store's last reader. # -# No protected(): the full default rerun-triggers govern, and protected() only -# ever forced people through a `--forcerun` detour. +# No protected(): the profile's rerun triggers govern catalogue regeneration. rule tile_make_cat: group: TILE_GROUP input: - # No store input: it is node-local, written by tile_vignets in this same - # group job. make_cat reads its configured PSF-interpolation output - # through $NGMIX_VIGNET_DIR (config_tile_Mc.ini). + # See tile_vignets for store lifetime, and config_tile_Mc.ini for + # make_cat's PSF-interpolation input through NGMIX_VIGNET_DIR. ms = rules.tile_merge_cats.output.manifest, output: manifest = f"{TILE_DIR}/manifests/tile_make_cat.json", @@ -925,75 +551,17 @@ rule tile_make_cat: "fi\n") -# --- tile reclamation (D5) -------------------------------------------------- -# The tile-side counterpart of clean_exposure, and the differences are the whole -# design. Read that rule's commentary in exposure.smk first; this one only says -# where the tile case departs from it. -# -# WHY IT EXISTS. clean_exposure reclaims exposure stores; nothing reclaimed -# tiles, so a FINISHED tile held 1.19 GiB across 137 inodes on scratch forever -# (measured on 186.307; the per-directory breakdown is in clean_tile.py). At -# DR6's 23,114 tiles that is 26.9 TiB against a 1 TiB quota and 3.17M inodes -# against a 1M one. BOTH BOUNDS BIND, and the byte one binds first: without this -# rule no batch may exceed ~859 tiles. -# -# ELIGIBILITY IS TRIVIAL, AND THAT IS THE POINT: a tile's store has no consumer -# outside that tile, so there is no consumer set, no eligibility test and no -# staleness to detect — only an ordering edge on the tile's own final_cat -# (clean_tile.py opens with the asymmetry; the survivor set is argued there too). -# -# THE INPUT IS final_cat, ON THE PERSISTENT ROOT, AND NOT ancient(). It is -# already the campaign's designated tile-finished marker (see final_cat()'s -# docstring and tile_finished() above), and an ordinary input is what orders the -# clean after the tile — which is what makes reclamation ROLLING: within one -# invocation a tile is reclaimed as soon as its own chain lands, so the scratch -# high-water tracks the tiles in flight instead of the tiles in the batch. That -# is the whole reason the rule is worth having; ancient() would keep the -# dependency but drop the ordering, and the batch would peak at its full size. -# -# SCOPE: TILES_READY only, never every tile in the index — see -# clean_tile_targets() in the Snakefile for what an out-of-scope tombstone drags -# into the DAG. +# --- tile reclamation ----------------------------------------------------- +# Reclaim each ready tile after its persistent final_cat lands, keeping shared +# scratch proportional to tiles in flight. The input is the tile-finished marker +# defined by final_cat() in the Snakefile. See clean_tile_targets() there for +# scope, and clean_tile.py for the measured footprint and survivor set. # -# THE SURVIVING tile_vignets.json DOES NOT TRIP TILE_VIGNET_REQUIRED, AND THE -# REASON IS WHAT GETS DELETED, NOT WHAT GETS KEPT. Read that guard above first. -# A cleaned tile is, on its face, exactly the state it exists to catch: a valid -# tile_vignets manifest over a node-local store that is not there (here because -# it never outlived its job, rather than because a fused group half-ran). -# smk-g4's job_head.sh carries a runbook sweep for that state, and it skips any -# tile with a final_cat — i.e. every tile this rule ever touches — so a cleaned -# tile is invisible to it either way. -# -# Force a cleaned tile back anyway (delete its final_cat) and tile_vignets IS -# rescheduled, so the store is rebuilt and no chunk trips the guard. Measured on -# the fixture: 19 jobs, the whole chain from tile_get_images, tile_vignets inside -# the tile_shape group. Snakemake names the mechanism itself -- -# reason: Input files updated by another job: .../exp_forest, -# .../manifests/tile_find_exposures.json, .../manifests/tile_detect.json -# -- and it is NOT the `mtime` trigger. It is the structural propagation that -# makes every dependent of a rerunning job rerun, the same edge the cascade -# commentary above says ancient() cannot suppress. Worth the distinction: that -# propagation is not in profiles/nibi's rerun-triggers list and so cannot be -# switched off there, where an mtime argument could be. -# -# So the guard stays silent only because tile_detect.json and exp_forest/ GO. -# Counterfactual on the same fixture, upstream manifests restored and mtimes -# controlled so nothing reruns on timestamp -- i.e. the "delete only output/" -# design -- schedules 8 x tile_ngmix + tile_merge_cats + tile_make_cat with -# tile_vignets ABSENT, and every chunk trips the guard. -# ADDING A MANIFEST TO THE SURVIVOR LIST IS THEREFORE NOT FREE: tile_vignets is -# safe because nothing upstream of it survives, and tile_detect.json in -# particular must never join the list. -# -# A LOCALRULE (declared in the Snakefile), same as clean_exposure. It is a DAG -# LEAF, so being local can never make it both a dependency and a dependent of a -# group — no `group:` label here, and none possible. -# -# WHAT THIS SHARPENS ELSEWHERE: the TILE_LOCAL warning above says an edit to -# tile_local() mid-campaign reruns finished tiles unsatisfiably because their -# EXPOSURE stores are reclaimed. With this rule on, the tile's own store is gone -# too, so the rerun has even less to stand on. The recommendation is unchanged — -# land those edits between campaigns — but the margin for being wrong is smaller. +# Rebuilding a cleaned tile must also rebuild its node-local stores. Deleting +# final_cat schedules upstream jobs whose reruns propagate to tile_vignets; +# preserving tile_detect.json or exp_forest would prevent that path and leave +# readers without the writer. See clean_tile.py before changing the survivor set. +# This rule is a local DAG leaf (declared in the Snakefile), outside all groups. rule clean_tile: input: # The tile-finished marker, on the persistent root. A lambda rather than @@ -1017,45 +585,17 @@ rule clean_tile: " --tombstone {output.tombstone}" -# --- the campaign's shear catalogue ----------------------------------------- -# ONE job per campaign, the tile-side twin of exposure.smk's star_cat_merge, and -# the same three design calls hold: the input is the list `rule all` already -# requests (every ready tile's final_cat), the paths never reach the shell -# (MAX_ARG_STRLEN), and a fingerprint on `params` is what makes it rerun when a -# tile is appended. The job derives the same set the fingerprint was taken over -# from the tile list and the index rather than globbing products_dir — on a -# products root shared with an earlier, larger tile list a glob would merge tiles -# no rerun trigger ever saw. -# -# THE OUTPUT SCHEMA IS AN INTERFACE, NOT A CHOICE. sp_validation opens this file -# as its `galaxy_cat_path`: one dataset per tile under a named group, the -# columns of CONFIG_DIR's final_cat.param, an `n_tiles` attribute on the -# root. The group is named for the CAMPAIGN, which is the only unit this -# workflow has above the tile. It is the merger for both input types: an -# image-sims campaign reads config/cfis_image_sims/final_cat.param, whose -# columns are what sp_validation's image-sims extract step reads. So the rule reuses -# scripts/python/create_final_cat.py's column extraction rather than restating -# it, and writes the file itself — merge_final_cat.py argues that split, the one -# legacy literal in the schema, and the two places where the reference -# implementation had to be pinned down to be reproducible. -# -# THE INPUT IS final_cat, NOT the tile_make_cat manifest, for the same reason -# clean_tile's is: final_cat on the persistent root IS the campaign's -# tile-finished marker (see final_cat() in the Snakefile), and it is the file -# this rule actually reads. +# --- campaign shear catalogue -------------------------------------------- +# One job merges the same ready-tile final_cat list requested by rule all. +# Pass the tile list and index, not every path (MAX_ARG_STRLEN), and fingerprint +# that input set so an append triggers reconciliation. Globbing products_dir +# could include tiles outside the campaign. See merge_final_cat.py for the +# reconciliation algorithm and output schema, and workflow/CONTRACTS for +# campaign-name-is-run and final-cat-param-is-exact-allow-list. # -# NOT A LOCALRULE, and here the reason is IO rather than memory: a first build -# reads every tile's catalogue end to end — ~32-46 MB per tile, so ~2 GB for a -# 64-tile campaign and ~800 GB at DR6's 23k tiles. It RECONCILES rather than -# rebuilds or appends: a tile with no dataset is added, a dataset whose tile -# left the campaign is deleted, a dataset whose source catalogue changed is -# re-read, and one that agrees with its source is left alone. So an append -# reads the appended tiles and nothing else, while the file still cannot drift -# from its inputs the way an append-only tool does (merge_final_cat.py argues -# what is and is not a function of the input set here). Memory is one tile's -# catalogue at a time plus the hdf5 write buffer, which is why mem_mb is modest -# where star_cat_merge's is not — and why runtime, which is sized on the whole -# campaign, is the pessimistic first-build case. +# Run on a compute node: the first build reads ~32-46 MB per tile (~800 GB for +# DR6's 23k tiles). Memory holds one tile plus the HDF5 buffer; runtime budgets +# the full first build. Reconciliation reads only new or changed source catalogues. rule final_cat_merge: input: lambda wc: [final_cat(t) for t in TILES_READY] @@ -1072,15 +612,13 @@ rule final_cat_merge: script_hash = MERGE_FINAL_HASH threads: 1 resources: - # Sized on the LARGEST tile, not the total: the merge holds one - # catalogue at a time, and the measurement is flat in the tile count - # (the Snakefile's sizing block carries both points). + # See the Snakefile's sizing block for the largest-tile memory model. mem_mb = lambda wc, attempt: capped_mem(attempt * ( FINAL_MEM_BASE_MB + FINAL_MEM_FACTOR * final_cat_max_bytes() // 1_000_000), "final_cat_merge"), - # Runtime, unlike memory, is the TOTAL: every tile is read end to end. - # ~1 min per 10 tiles on the measurement, triply generous, over a floor. + # First-build budget: ~1 min per 10 tiles measured, with 3x margin + # above a fixed floor. Reconciliation can read fewer tiles. runtime = lambda wc, attempt: attempt * (30 + len(TILES_READY) // 3) shell: "set -euo pipefail\n" From 67b2f3f416fdb2901bdeb6fa94b6d2a27e998de1 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:47:22 +0200 Subject: [PATCH 27/43] docs(workflow): present-tense comments in workflow README and contracts Co-Authored-By: GPT-6.1 Sol --- workflow/CONTRACTS | 6 +- workflow/README.md | 184 +++++++++++++-------------------------------- 2 files changed, 55 insertions(+), 135 deletions(-) diff --git a/workflow/CONTRACTS b/workflow/CONTRACTS index fe2e16c36..4a5b51873 100644 --- a/workflow/CONTRACTS +++ b/workflow/CONTRACTS @@ -22,7 +22,7 @@ tests/workflow/test_dag.py::test_products_use_products_dir_and_run_name. Each input type's `config/*/final_cat.param` is the merged catalogue's exact column list, in order: `final_cat_merge` writes those columns and no others, and the reader raises on a listed column a tile lacks. So a name belongs there -only if `make_cat` writes it on EVERY tile. A per-epoch family (`EXP_ID_n`, +only if `make_cat` writes it on every tile. A per-epoch family (`EXP_ID_n`, `CCD_n`, `HSM_*_PSF_n`) qualifies only with a fixed slot count, since `make_cat` otherwise sizes it from the sexcat's maximum geometric `N_EPOCH`. Enforced by tests/unit/test_final_cat_merge_invariants.py (the merge) and @@ -44,8 +44,8 @@ Regenerate tests/workflow/params_pin.json only at that boundary with @sc [label:custody] clean-exposure-waits-on-persist-iff-psf `clean_exposure` waits on the exposure's persisted PSF products exactly when -`PERSISTS_PSF` (`psf_model != "fake"`, which is `psfex`: `mccd` is refused at -parse time by `refuse_unpersistable_psf`): on the `exp_persist` manifest for a +`PERSISTS_PSF` is true (`psf_model != "fake"`; `mccd` is refused at parse time +by `refuse_unpersistable_psf`): on the `exp_persist` manifest for a live store, and on the tar (a leaf), or nothing, for a reclaimed one. Dropping the edge under psfex lets reclamation delete the scratch store before its PSF products reach `products_dir`; keeping it under `fake` makes every clean wait on diff --git a/workflow/README.md b/workflow/README.md index a415e1b55..0b6baab59 100644 --- a/workflow/README.md +++ b/workflow/README.md @@ -1,10 +1,8 @@ # ShapePipe Snakemake orchestration -Snakemake workflow that orchestrates real-data ShapePipe runs. It replaces the -`curl_canfar_local.sh → run_job_sp_canfar_v2.0.bash → job_sp_canfar_v2.0.bash` -bash layers and the per-site sbatch reimplementations. **Module code is -untouched**: rules call `shapepipe_run -c ` on the existing config -chains. Design and rationale: +Snakemake workflow that orchestrates ShapePipe data and image-simulation runs. +Rules call `shapepipe_run -c ` on the module configuration chains. +Design and rationale: [CosmoStat/shapepipe#848](https://github.com/CosmoStat/shapepipe/issues/848) (the design document). @@ -26,8 +24,7 @@ uv pip install 'snakemake>=9,<10' 'snakemake-executor-plugin-slurm>=2.7,<3' # the campaign's name; workflow/config.yaml's input_types: and machines: tables supply the rest. # `psf_model` is `psfex` for data (`fake` for image sims). `mccd` is refused -# until `persist_exp.py` and `merge_star_cat.py` read MCCD products. PSFEx is -# exercised by smk-g4 through smk-g6. +# until `persist_exp.py` and `merge_star_cat.py` read MCCD products. # Both `tile_detection` values run SExtractor on the tile image. Under # `unions_catalogue` (the input_types default for data) the UNIONS per-tile # catalogue at `inputs.catalogues` is fetched and the detections are joined @@ -44,11 +41,9 @@ workflow/bin/sp cancel # scancel this workflow's jobs workflow/bin/sp container status # which image the jobs will run ``` -Installed and pinned versions on nibi (`/project/def-mjhudson/cdaley/snakemake-env`, -queried 2026-07-30): `snakemake==9.23.1`, `snakemake-executor-plugin-slurm==2.7.1`. -Pin range: `snakemake>=9,<10`, `snakemake-executor-plugin-slurm>=2.7,<3`. The -v8→v9 breaks matter here: `--use-singularity` became `--sdm`, executors became -plugins, and full `rerun-triggers` became the default. +Supported ranges: `snakemake>=9,<10`, `snakemake-executor-plugin-slurm>=2.7,<3`. +The profiles use Snakemake 9's `--sdm` software deployment and executor plugins; +see `profiles/` for the rerun triggers. Anything other than `run`, `report`, `container`, `cancel` passes straight through to snakemake with the workflow's profile and state dir — the direct command path for @@ -141,9 +136,7 @@ only exists if you ask for one: The Snakefile's `container:` is that resolution, in one order shared by the CLI and the workflow: **sandbox → cached SIF → the `container:` path in -`config.yaml`**. With an empty cache — the normal case — that lands on the -shared `/project` `.sif` the workflow has always used, so this changes nothing -until you opt in. +`config.yaml`**. With an empty cache, resolution falls back to the configured shared image. ```bash sp container status # layers present, active one, revision vs HEAD @@ -174,9 +167,9 @@ therefore two snakemake invocations over one Snakefile: 1. **PREPARE** — `snakemake prepare_all_tiles`: per-tile static DAG (`Git_vos → Uz → Fe`), `keep-going` so tile failures are independent. A - nonzero exit here is not fatal to the run — tiles that lost their - exposure list are dropped at the compute parse — but it is a warning: - `SP_MISSING_THRESHOLD` (default 0.0) is the real gate. + nonzero exit still allows COMPUTE to run, but makes `sp run` fail. + Tiles without exposure lists are dropped at the compute parse, subject to + `SP_MISSING_THRESHOLD` (default 0.0). 2. **COMPUTE** — `snakemake all`: this invocation's *parse* builds the tile↔exposure index (`build_index.py`, imported at parse time, not a DAG node) and runs the full tile/exposure compute chain. The index @@ -208,11 +201,11 @@ the other verbs run out of the existing snapshot, `sp container` excepted (it is about the image you are working with now, not about a campaign). The mechanism and its rationale live in one place: `bin/sp`. -## Execution mode: one SLURM job per rule +## Execution mode: SLURM rules and groups -The profile (`profiles/nibi/config.yaml`) sets `executor: slurm`. Every rule -instance becomes its own SLURM job carrying that rule's own attempt-scaled -resources (`cpus_per_task = threads`, `mem_mb`, `runtime`) and runs inside +The profile (`profiles/nibi/config.yaml`) sets `executor: slurm`. Ungrouped rule instances become individual SLURM jobs; grouped rules share a job. +Rules define attempt-scaled resources (`cpus_per_task = threads`, `mem_mb`, +`runtime`) and run inside `apptainer exec` via the profile's software-deployment method — the workflow never calls apptainer directly. Snakemake feeds the queue as jobs finish, so the full campaign's job count never needs to queue at once, and multi-node @@ -226,10 +219,8 @@ attempt-scaled `mem_mb = lambda wc, attempt: ...` OOM retries and the tuned ngmix thread count. Rules own their own resources; the profile only supplies defaults for rules that state nothing. -`group:` labels that fuse short rules (uncompress, merges) into their chunky -neighbours (queue-latency amortization, per the PRD) are **not yet wired**: -they require labels in `workflow/rules/*.smk`, out of scope for this -profile-only pass. +The rule files define the groups and their resource constraints. +See `workflow/rules/prepare.smk`, `exposure.smk`, and `tile.smk` for their composition. ## Layout @@ -245,7 +236,7 @@ workflow/ scripts/ build_index.py prepare-phase run_index.sqlite builder (plain script) build_forest.py per-tile exposure symlink forest (group-compatible shell) - completeness.py the ported count table (shared by sp_rule + run_report) + completeness.py count table shared by sp_rule + run_report run_report.py standalone report (NOT a DAG node; run_report hooks call it) container.py image layers + the resolution order behind `sp container` (stdlib-only) persist_exp.py ONE exposure's keepable PSF products -> one tar on products_dir (the exp_persist rule) @@ -258,7 +249,7 @@ workflow/ profiles/nibi/config.yaml SLURM executor; apptainer SDM; per-user jobs cap; keep-going ``` -## How it works +## Products and lifecycle - **The atom is one rule == one `shapepipe_run` on one unit.** Its single declared output is that unit's manifest @@ -279,57 +270,28 @@ profiles/nibi/config.yaml SLURM executor; apptainer SDM; per-user jobs cap; kee - **Completeness is an exact-count check, not a taxonomy.** After a run, `sp_rule.py` counts products per mandatory runner against `completeness.py`'s expected count and exits nonzero below it. A shortfall - below `expect` fails unless `warn` is set. No 3-class taxonomy, no - error-signature whitelist. `--keep-going` isolates a failure to its own + below `expect` fails unless `warn` is set. Failure signatures do not exempt shortfalls. `--keep-going` isolates a failure to its own DAG cone. - **Stores are sharded.** Every tile/exposure runs its own `shapepipe_run` in `tiles/<2-char prefix>//` or `exp///`. Configs are committed under `workflow/config/cfis/` and version with the rules that set the env vars they interpolate — there is no `config_src` knob, and no per-unit config symlink; `$SP_CONFIG` points straight at the committed directory. -- **There is no masking stage, on either side.** ShapePipe generates no masks - (PR #847). The one mask that reaches pixels is the instrument flag image - delivered with each exposure, which `exp_split` splits per CCD beside image - and weight and SExtractor reads as `IMAFLAGS_ISO`. Everything else — star - halos, manual masks, per-band coverage, MaxiMask — is supplied as sky-fixed - healsparse maps and QUERIED once per object: the `mask_query` module writes a - `MASK_EXT` column onto each CCD's detection catalogue (inside `exp_psf`), and - `make_cat` writes one `MASK_` column per band onto the final catalogue - (inside `tile_make_cat`). Neither is cut on in the pipeline: instrument flags - mark corrupted measurements and are the only masks that reject anything here, - while the healsparse masks are location flags and every decision about them is - downstream. On exposures the query ships off — `MASK_PATHS` is commented out, - making `mask_query` a no-op pass-through — so turning it on is a config edit, - not a chain edit. Map paths are config, not - code, so regenerated products cost a config edit. Nothing is fetched from a - catalogue server, staged, or rasterized, which is why the old - `star_catalogue` / `exp_star_cat` / `exp_mask` rules and their cache root are - gone. -- **Two HealSparse maps come out of the exposures.** At the mask ladder's - resolution (nside 131072), `nexp_.hsp` counts per sky pixel the - exposures whose CCD with a valid PSF model covers it, and - `nflagged_.hsp` counts the flagged CCD pixels (bad columns, saturated - pixels, bleed trails, cosmics) of those exposures that fall in it, ~74 CCD - pixels filling one sky pixel. The flag image is the one mask that otherwise - never leaves the pixel domain, so a footprint built from CCD corners could not - subtract it (#878). `exp_maps` writes one fragment per exposure beside its PSF - tar, from the split's `DATASEC` (the overscan border is neither coverage nor - defect) and the CCDs `exp_persist` packed a PSF for; `exposure_maps` sums the - campaign's fragments. Nothing in the workflow reads either map. `nexp` is - what randoms need to match an `N_EPOCH` cut; `nflagged` is a diagnostic of - where the detector is bad, not a cut (it does not count exposures lost to - the defect veto). -- **External masks are wired, on the tile side only (data runs).** `inputs.masks` - is a third input root beside tiles and exposures, set per machine in the - `machines:` table, exported as `$SP_INPUT_MASKS` and - pointing at the UNIONS DR6 ugriz bit ladder: one boolean healsparse map per - bit, nside 131072, `True` = masked. `config_tile_Mc.ini` names all 11 of them - in `MASK_EXT_PATHS` under a `_` label, so `make_cat` - writes `MASK_1_Faint_star_halos` … `MASK_2048_z2` and `final_cat.param` - carries the matching 11 names; the config holds the label table. - `MASK_2048_z2` is True where there is *no* Pan-STARRS z data, so an OR over - every column masks everything. ShapePipe cuts on none of them; the mask - choice is sp_validation's. +- **ShapePipe generates no masks.** `exp_split` splits the delivered instrument + flag image per CCD; SExtractor reads it as `IMAFLAGS_ISO`. + External sky masks are queried per object, not rasterized. + Exposure queries are disabled by the commented-out `MASK_PATHS` in + `config_exp_psfex.ini`; see `src/shapepipe/modules/mask_query_package/` + for query semantics and `star_selection.setools` for star-mask selection. +- **Exposure maps describe coverage and detector defects.** `exposure_maps` + merges the `exp_maps` fragments into `nexp_.hsp` (valid-PSF exposure + coverage) and `nflagged_.hsp` (flagged detector-pixel counts). + Neither map is a pipeline cut. See `workflow/scripts/exp_maps.py` and + `merge_exposure_maps.py` for resolution, footprint, and counting semantics. +- **Data tiles carry external mask flags, without cuts.** `inputs.masks` is + exported as `$SP_INPUT_MASKS`. The label table and map semantics live in + `config/cfis/config_tile_Mc.ini`; `final_cat.param` lists the output columns. + Mask selection belongs to downstream analysis. - **The index is parse-time data, never a rule input.** Appending tiles changes which jobs exist without invalidating completed work. - **Exposure products are not `temp()`.** Exposures overlap tiles, so @@ -354,65 +316,25 @@ profiles/nibi/config.yaml SLURM executor; apptainer SDM; per-user jobs cap; kee trigger reads that cut as a reason to rerun the very tiles it protects. Know the consequence — `--forcerun` on a tile whose `final_cat` exists will not rebuild its reclaimed exposures. Delete the `final_cat` first. -- **PSF products leave scratch before the purge does.** `exp_persist` packs - the products named by `persist_exp:` in `config.yaml` from the exposure's - scratch store into ONE uncompressed tar, - `/exp///psf/.tar` (inodes, not bytes, bind - on /project), and writes ONE manifest beside it recording the patterns, the - members and their sizes. The - threat it answers is the /scratch purge, not `clean_exposure` — the store goes - in 60 days whether or not the workflow reclaimed it — so it runs even with - `clean: false`, requested directly by `rule all`. `clean_exposure` takes its - manifest as an input, so reclamation can never overtake the copy. It is a - rule of its own rather than a `cp` on the end of `exp_psf` because the keep - list rides on `params`: adding a pattern reruns seconds of packing, not four - hours of PSF fitting per exposure. A pattern that matches nothing is a - recorded warning (setools rejects sparse CCDs); matching nothing at all is a - failure. A `localrule`, by the same arithmetic as `clean_exposure`. -- **The star catalogue's inputs are always kept; `persist_exp:` is what you - keep on top.** `exp_persist` packs `psf_validation` — the psfex_interp - validation catalogue, one per CCD — for every exposure whatever the config - says, because `star_cat_merge` stacks exactly those into the campaign's - `full_starcat`. They are that catalogue's provenance, and they are what keeps - appending a tile next month cheap rather than a rebuild from VOS. About 2 MB - per exposure: ~40 GB and ~40k inodes at DR6 scale, against a ~1 M-inode group - quota. `persist_exp:` is purely additive, and an empty list is legal — the tar - then holds the merge's inputs and nothing else. -- **The keep list names products, not globs.** Entries are names from a - catalogue in `workflow/scripts/persist_exp.py`, which is the single source of - truth for what each one means and what keeping it buys - ([#844](https://github.com/CosmoStat/shapepipe/issues/844)); `config.yaml`'s - block is that catalogue rendered, and `persist_exp.py --list-products` prints - it. Sizes are per exposure, 40 CCDs, measured on smk-m2. - - | product | glob | per exposure | what it buys | - |---|---|---|---| - | `psf_model` | `*.psf` | 2.8 MB | re-interpolate the PSF anywhere later, no rebuild | - | `psfex_cat` | `psfex_cat-*.cat` | unmeasured | which stars PSFEx's outlier rejection clipped | - | `star_selection` | `star_selection-*.fits` | 24.5 MB | which stars the selection cuts rejected, and why | - | `star_train` | `star_split_ratio_80-*.fits` | 19.9 MB | the 80% sample PSFEx fitted | - | `star_test` | `star_split_ratio_20-*.fits` | 7.1 MB | the 20% sample `psf_validation` corresponds to | - | `star_stats` | `star_stat-*.txt` | unmeasured | setools' per-CCD counts, density and FWHM cuts | - - The default is `psf_model`. `psf_validation` is in the catalogue too but needs - no naming; naming it anyway is harmless. **Retention is additive**: an - existing tar is a floor, so shrinking the list adds nothing and removes - nothing. Dropping a product is a deliberate act on `products_dir`, not a - config edit — otherwise editing a config would delete products from the - backed-up filesystem whose scratch originals are long gone. A raw glob is still accepted as an - escape hatch — anything with a glob metacharacter or a dot is read as one — - and an unknown *name* is a parse-time error listing the valid ones. The list - is exposure-side only; tile-side retention is #844 follow-up. +- **PSF products are persisted independently of scratch cleanup.** `exp_persist` + writes `/exp///psf/.tar` and a manifest, + even with `clean: false`. See `workflow/scripts/persist_exp.py` for retention + guarantees, measured storage costs, and packing details; see + `workflow/CONTRACTS` for cleanup ordering. +- **Configure optional retention with `persist_exp:`.** Product names and their + meanings live in `workflow/scripts/persist_exp.py`; `persist_exp.py --list-products` + prints them. The default is `psf_model`, and `psf_validation` is mandatory + regardless of the list. Retention is additive: shrinking the list does not + remove saved products. Remove products explicitly from `products_dir`. - **The campaign ends in two merged catalogues, and the workflow makes both.** Everything above is per unit; the two products downstream analysis actually - opens are per *campaign*, and until these rules existed each was a manual pass - after the run. + opens are per *campaign*. `star_cat_merge` collects every exposure's every CCD's `psf_validation` into `/full_starcat_.hdf5`, one dataset per exposure at `exposures/` — the rho/tau statistics input. It reads the members straight out of the per-exposure tars (`tarfile`; unpacking ~800k files to merge them would defeat the tar's whole purpose), keeps their native dtypes, - and stores `CCD_NB` as an int. sp_validation still opens the old flat FITS + and stores `CCD_NB` as an int. sp_validation opens the flat FITS name, `full_starcat-0000000.fits`; its readers move to this file under [sp_validation#340](https://github.com/CosmoStat/sp_validation/issues/340), the same migration that retires the `patches/` key on the tile side. The rule @@ -420,19 +342,17 @@ profiles/nibi/config.yaml SLURM executor; apptainer SDM; per-user jobs cap; kee **Two writers, one schema.** The module runner still emits the flat FITS table through `MergeStarCatPSFEX`, and this rule emits the hdf5; they are separate implementations on purpose, because only one of them reads tars, - keeps native dtypes and reconciles. Their 22 COLUMN NAMES must not drift - apart, and nothing else would notice if they did — a column added to one - writer would just be missing from the other's product. `tests/unit/` - `test_star_cat_columns.py` is what holds them together. + keeps native dtypes and reconciles. Their column names must agree; + `tests/unit/test_star_cat_columns.py` enforces that coupling. `final_cat_merge` collects every ready tile's `final_cat-.hdf5` into `/final_cat_.hdf5`: one dataset per tile under a group - named for the campaign, the `final_cat.param` columns, an `n_tiles` attribute. + named `patches/`, the `final_cat.param` columns, an `n_tiles` attribute. That schema is what sp_validation's reader opens, so it is fixed; the column extraction reuses `scripts/python/create_final_cat.py` while the file is written here, because that script's own discovery walks a directory layout this workflow does not have. The run config's `run:` names both files and the group. - BOTH RECONCILE, through one shared module (`hdf5_reconcile.py`) so the + Both reconcile through one shared module (`hdf5_reconcile.py`) so the campaign's two products cannot disagree about what an output owes its inputs. Each adds the units that have no dataset, drops datasets whose unit left the campaign, re-reads one whose source changed (every dataset records its @@ -457,7 +377,7 @@ profiles/nibi/config.yaml SLURM executor; apptainer SDM; per-user jobs cap; kee absorbed manifests out of `cleaned.json`, so a reclaimed exposure keeps its per-runner counts and blocks no tile. The logs go with the manifests — a log claiming `complete` for a store that is gone would contradict the unbuilt - chain the DAG must now see, and its content duplicates the manifest anyway. + chain the DAG must see, and its content duplicates the manifest anyway. The `exp_psf` benchmark tsv lives beside both dirs, not inside either, so reclamation does not eat the memory-sizing data. - **Failure is a report, not a gate.** `run_report.py` disk-scans the trees From 70f2070203c23419b9f4340f104f3dc458c7f75b Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:47:22 +0200 Subject: [PATCH 28/43] docs(environment): present-tense comments in container and dependency documentation Co-Authored-By: GPT-6.1 Sol --- .github/workflows/deploy-image.yml | 5 ++--- Dockerfile | 6 +++--- docs/source/container.md | 31 +++++++++++------------------- docs/source/dependencies.md | 14 +++++--------- pyproject.toml | 21 ++++++++------------ 5 files changed, 29 insertions(+), 48 deletions(-) diff --git a/.github/workflows/deploy-image.yml b/.github/workflows/deploy-image.yml index 6e8eb1206..6ecd4791e 100644 --- a/.github/workflows/deploy-image.yml +++ b/.github/workflows/deploy-image.yml @@ -120,9 +120,8 @@ jobs: docker run --rm -e HYPOTHESIS_PROFILE=ci "$IMAGE" \ pytest -rX -m "not slow" --no-cov tests/science - # The actual test suite, run inside the shipped image — replacing the - # retired conda-based suite. pytest exercises the same wheels, binaries, - # and Python (3.12) that production runs on, not a parallel environment. + # Run the full suite inside the shipped image: the same wheels, + # binaries, and Python (3.12) that production uses. # pyproject's addopts add `--cov=shapepipe`; COVERAGE_FILE is set to /tmp # in the image so it works on read-only filesystems too. The Hypothesis # profile is explicit here so CI always uses the deterministic, capped diff --git a/Dockerfile b/Dockerfile index 400ee591f..d5682b4c1 100644 --- a/Dockerfile +++ b/Dockerfile @@ -2,8 +2,8 @@ # # One image: CI tests it, canfar batch jobs run it, downstream stacks (e.g. # sp_validation) build FROM it. Everyday CLI tools and all extras (test, -# lint, doc, jupyter, fitsio, …) are baked in, so there is nothing a job -# runs that CI did not already exercise. +# lint, doc, jupyter, fitsio, …) are baked in. See +# .github/workflows/deploy-image.yml for the checks run before publication. # ---------------------------------------------------------------------- # base — system deps @@ -61,7 +61,7 @@ RUN apt-get update -y --quiet && \ # host's `mpirun` launches one container rank per slot, and the OpenMPI inside # the image wires the ranks together through PMIx. That handshake requires the # container's PMIx to be compatible with the host launcher's. Debian bookworm's -# package is OpenMPI 4.1.4 with PMIx 2.x; modern clusters (e.g. candide) now run +# package is OpenMPI 4.1.4 with PMIx 2.x; modern clusters (e.g. candide) run # OpenMPI 5.0.x with PMIx 5.x, and a PMIx 2 client cannot talk to a PMIx 5 # server — so every rank silently degrades to a standalone "rank 0 of 1" and the # job runs N independent copies instead of one N-rank job. Building OpenMPI diff --git a/docs/source/container.md b/docs/source/container.md index 704465b47..e3e791f11 100644 --- a/docs/source/container.md +++ b/docs/source/container.md @@ -7,7 +7,7 @@ CANFAR), see [Running on a cluster](clusters.md). ## One image -Every tag (`:develop`, a feature branch, `:v1.1.0`, …) is the same build: +Every published branch tag (`:develop`, a feature branch, …) uses the same build: everyday CLI tools (`vim`, `tmux`, `htop`, `rg`, `fd`, `jq`, `bat`, `less`, `git-lfs`, …) and **all** Python extras (test, lint, doc, jupyter, fitsio, release) on top of core deps. CI runs the test suite inside it before @@ -110,7 +110,7 @@ This is the **single source of truth** for *what kinds of environments shapepipe is compatible with*. Edit when: - adding a new dependency (`uv add foo` will edit it for you) -- bumping a minimum after crossing a major version line +- bumping a minimum when code requires a newer API - adding/restructuring extras - changing project metadata (scripts, version, etc.) @@ -129,9 +129,9 @@ uv add 'foo>=1.2' # adds to pyproject AND uv.lock ``` Commit `uv.lock` to the repo — that's how reproducibility transfers -between machines. The Dockerfile uses `uv sync --frozen`, which fails if -`pyproject.toml` and `uv.lock` have drifted. That's the discipline -mechanism: a stale lockfile cannot ship. +between machines. The Dockerfile uses `uv sync --frozen` to install from the +lockfile without resolving dependencies or checking whether it is current. +Regenerate the lock whenever requirements change. ### `Dockerfile` — the build recipe @@ -161,18 +161,9 @@ The asymmetry is deliberate: Python deps go through pyproject + lockfile versioning). Don't `apt install` something that has a Python wheel; don't `pip install` something Debian packages directly (e.g. `source-extractor`). -## Why this shape - -- **No conda.** Earlier images double-installed packages via conda *and* - pip, ballooning the image and creating environment-resolution bugs. - Dropped in favour of plain Python + uv. -- **`uv sync --frozen`** at build time means the image is bit-exactly - reproducible from a tagged commit, and impossible to ship with a stale - lockfile. -- **Astromatic binaries from Debian** (`psfex`, `source-extractor`) - instead of source builds — Debian carries the - GCC-compatibility patches that the previous Dockerfile had to apply - inline with `sed`. -- **One image** so a canfar batch job, a downstream `FROM` clause, and CI's - test run are never three different environments — `vim`, `pytest`, and - everything else ship on every tag. +## Build choices + +The image uses plain Python and uv for Python dependencies, and Debian's +`psfex` and `source-extractor` packages for the Astromatic binaries. +See `Dockerfile` for the source-built OpenMPI version and host-compatibility +requirements. diff --git a/docs/source/dependencies.md b/docs/source/dependencies.md index 8c9afd630..776dca1c3 100644 --- a/docs/source/dependencies.md +++ b/docs/source/dependencies.md @@ -16,7 +16,7 @@ Scientific stack: |---------|------------| | [Astropy](https://www.astropy.org/) | {cite:p}`astropy:2013,astropy:2018` | | [GalSim](https://github.com/GalSim-developers/GalSim) | {cite:p}`rowe:15` | -| [ngmix](https://github.com/aguinot/ngmix) | {cite:p}`sheldon:15` | +| [ngmix](https://github.com/esheldon/ngmix) | {cite:p}`sheldon:15` | | [MCCD](https://github.com/CosmoStat/mccd) | {cite:p}`liaudat:21` | | [ModOpt](https://cea-cosmic.github.io/ModOpt/) | {cite:p}`farrens:20` | | [python-pysap](https://github.com/CEA-COSMIC/pysap) | | @@ -26,7 +26,6 @@ Scientific stack: | [Matplotlib](https://matplotlib.org/) | {cite:p}`hunter:07` | | [Joblib](https://joblib.readthedocs.io/en/latest/) | {cite:p}`joblib:20` | | [mpi4py](https://mpi4py.readthedocs.io/en/stable/) | {cite:p}`dalcin:05,dalcin:08,dalcin:11` | -| [reproject](https://reproject.readthedocs.io/) | | | [h5py](https://www.h5py.org/) | | Data access & infrastructure (CANFAR / UNIONS): @@ -36,21 +35,18 @@ Data access & infrastructure (CANFAR / UNIONS): | [vos](https://github.com/opencadc/vostools) | CADC / CANFAR VOSpace access | | [skaha](https://github.com/shinybrar/skaha) | CANFAR Science Platform sessions | | canfar | CANFAR container-job submission | -| [astroquery](https://astroquery.readthedocs.io/) | external catalogue queries | | [cs_util](https://github.com/CosmoStat/cs_util) | shared CosmoStat utilities | | [sqlitedict](https://github.com/RaRe-Technologies/sqlitedict) | on-disk pipeline state | ```{note} -`ngmix` is pinned to the -[`aguinot/ngmix@stable_version`](https://github.com/aguinot/ngmix/tree/stable_version) -fork until the fixes land upstream — do not modernise that line (see the note in -`pyproject.toml`). +Git-sourced packages and their selected refs are declared in +`pyproject.toml` under `[tool.uv.sources]`; `uv.lock` pins the resolved commits. ``` ## System Dependencies -The container also provides the non-Python tools ShapePipe calls, all from Debian -packages (no source builds), plus the MPI stack: +The container also provides the non-Python tools ShapePipe calls, from Debian +packages, plus a source-built MPI stack (see `Dockerfile`): | Package | References | |---------|------------| diff --git a/pyproject.toml b/pyproject.toml index 53f0dae61..8457a38fd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -11,14 +11,14 @@ authors = [ license = "MIT" license-files = ["LICENSE"] requires-python = ">=3.12" -# Dependencies use FLOOR RANGES; exact versions live in `uv.lock`, the +# Dependencies use floor ranges; exact versions live in `uv.lock`, the # reproducible manifest generated by uv (never hand-edited). Renovate updates # the lock (rangeStrategy=update-lockfile, see renovate.json), so uv's resolver # owns version selection — cross-package constraints (e.g. numba capping numpy) # are resolved jointly instead of failing update PRs. Bump a floor only when # the code actually requires the newer API. dependencies = [ - "astropy>=7.0", # major 6 → 7 + "astropy>=7.0", "canfar", "cs_util>=0.2.1", "galsim>=2.8", @@ -31,8 +31,8 @@ dependencies = [ "modopt>=1.6", "mpi4py>=4.1.2", "numba>=0.59", # numpy 2 support - "numpy>=2.4.6", # major 1 → 2 - "pandas>=3.0.3", # major 2 → 3 + "numpy>=2.4.6", + "pandas>=3.0.3", "python-dateutil", "python-pysap>=0.3", "PyQt5", @@ -96,10 +96,8 @@ environments = ["sys_platform == 'linux'"] [tool.uv.sources] ngmix = { git = "https://github.com/esheldon/ngmix", tag = "v2.4.1" } -# Track cs_util's develop branch (a git ref, not a pinned release) so new -# cs_util.size / cs_util.shape helpers are available without shipping a release -# and rebuilding the image each time. shapepipe's column grammar sources size -# (T = 2*sigma^2) and ellipticity conversions from cs_util. +# The column grammar uses cs_util's size (T = 2*sigma^2) and ellipticity +# conversions. Resolve develop at lock generation; uv.lock pins the commit. cs_util = { git = "https://github.com/CosmoStat/cs_util", branch = "develop" } [build-system] @@ -120,11 +118,8 @@ script-files = [ where = ["src"] [tool.ruff] -# Ruff is the single lint + format tool, replacing black, isort, and the -# pytest-pycodestyle / pytest-pydocstyle plugins (the latter broke on the -# pytest 9.1 hookspec change). Rules below cover pycodestyle (E/W), Pyflakes -# (F), import sorting (I), and numpydoc-style docstrings (D) — matching the -# project style: PEP 8 + numpydoc. +# Ruff handles linting and formatting: pycodestyle (E/W), Pyflakes (F), +# import sorting (I), and numpydoc-style docstrings (D). line-length = 79 target-version = "py312" src = ["src", "tests"] From 2d7eae93f7440dbb543a379c19cf3f8fec28aba2 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:47:23 +0200 Subject: [PATCH 29/43] docs(guides): present-tense comments in execution guides and repository overview Co-Authored-By: GPT-6.1 Sol --- CLAUDE.md | 11 +++++------ README.rst | 12 ++++++------ docs/source/clusters.md | 13 +++++-------- docs/source/installation.md | 7 +++---- docs/source/testing.md | 31 +++++++------------------------ docs/source/workflow.md | 5 ++--- 6 files changed, 28 insertions(+), 51 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index d8011f7f3..ee33bf5e2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -5,15 +5,14 @@ runs the full chain from raw survey images to calibrated shear catalogues — object detection, PSF modelling, and shape measurement — and was used to produce the first UNIONS cosmic-shear release. -The project is now entering a substantial rework of the shape-measurement -pipeline, with the near-term goal of a tight loop between the pipeline and image -simulations for validation and calibration. Development is by a small team; +Development focuses on a tight loop between the shape-measurement pipeline and +image simulations for validation and calibration. Development is by a small team; reproducibility and clarity matter more than breadth. ## Environment — the container is the source of truth ShapePipe is not a stand-alone library: it needs system tools (Source Extractor, -PSFEx, WeightWatcher), MPI, and a specific scientific-Python stack. The supported +PSFEx), MPI, and a specific scientific-Python stack. The supported way to get all of that is the container. - **Dependencies** are declared in `pyproject.toml` as **floor ranges**; exact @@ -105,8 +104,8 @@ Full detail: `docs/source/installation.md` and `docs/source/container.md`. exercises exactly what ships — run them the same way, in the container. - **CI** (`.github/workflows/deploy-image.yml`): every PR and push builds the image and runs the test suite + the example pipeline + binary smokes; pushes to - `develop` / `main` / `master` additionally publish the images. API docs deploy - from `master` (`cd.yml`). + any branch additionally publish the images. See `cd.yml` for versioned API-doc + builds and deployment. - **Style**: PEP 8; numpydoc docstrings on public modules, classes, and methods. Match the surrounding code. diff --git a/README.rst b/README.rst index fc1413031..86c4aa57f 100644 --- a/README.rst +++ b/README.rst @@ -3,8 +3,8 @@ ShapePipe |CI| |CD| |python312| |release| -.. |CI| image:: https://github.com/CosmoStat/shapepipe/workflows/CI/badge.svg - :target: https://github.com/CosmoStat/shapepipe/actions?query=workflow%3ACI +.. |CI| image:: https://github.com/CosmoStat/shapepipe/actions/workflows/deploy-image.yml/badge.svg + :target: https://github.com/CosmoStat/shapepipe/actions/workflows/deploy-image.yml .. |CD| image:: https://github.com/CosmoStat/shapepipe/actions/workflows/pages/pages-build-deployment/badge.svg :target: https://github.com/CosmoStat/shapepipe/actions/workflows/pages/pages-build-deployment @@ -24,8 +24,8 @@ Quickstart ---------- ShapePipe ships as a container image, so you can run the bundled example -pipeline — a single CFIS tile through the full chain — without installing -anything: +pipeline — a chain of demonstration modules — without installing the +scientific stack on the host: .. code-block:: bash @@ -37,8 +37,8 @@ anything: The image is built, tested, and published on every push to the `GitHub Container Registry `_: -``:develop`` tracks the integration branch, release tags (e.g. ``:v1.1.0``) a -stable cut. +``:develop`` tracks the integration branch; branch tags support testing before +merge. See ``.github/workflows/deploy-image.yml`` for publication triggers. Documentation ------------- diff --git a/docs/source/clusters.md b/docs/source/clusters.md index 0dd189b74..da7b6082b 100644 --- a/docs/source/clusters.md +++ b/docs/source/clusters.md @@ -29,7 +29,7 @@ protocol as the host launcher (see the candide section below). ## candide (SLURM) -candide uses **SLURM** (`sbatch`; the old `qsub`/PBS commands are gone). The repo +candide uses **SLURM** (`sbatch`). The repo ships ready job scripts in `example/pbs/` — `candide_smp.sh` (single node, parallelised with joblib) and `candide_mpi.sh` (multi-node, hybrid MPI). To run the bundled single-tile example end to end: @@ -88,14 +88,11 @@ post-processing — is documented in the [CANFAR production walkthrough](pipeline_canfar.md). ```{note} -The CANFAR production submission scripts (`scripts/sh/job_sp_canfar*.bash`) still -run under the pre-container environment and are slated for the same -container-first cleanup the candide scripts received. Treat the walkthrough as -the current-but-evolving production procedure. +For the CANFAR production submission scripts (`scripts/sh/job_sp_canfar*.bash`), +follow their environment requirements and the production walkthrough. ``` ## ccin2p3 -ccin2p3 is not yet containerised. The `example/pbs/cc_{smp,mpi}.sh` scripts target -the pre-container environment; a container-first path mirroring candide is future -work. +The `example/pbs/cc_{smp,mpi}.sh` scripts target a host-installed environment. +See their headers for machine-specific requirements. diff --git a/docs/source/installation.md b/docs/source/installation.md index 169a623db..8355df7c2 100644 --- a/docs/source/installation.md +++ b/docs/source/installation.md @@ -1,9 +1,8 @@ # Installation ```{attention} -ShapePipe was not designed to be a stand-alone Python library. Instead users -are expected to install the full ShapePipe environment on the system(s) where -data should be processed. +Running the pipeline requires both the Python package and system executables. +Use the container for the complete supported environment. ``` ## Container Installation (Recommended) @@ -35,7 +34,7 @@ docker pull ghcr.io/cosmostat/shapepipe:develop ``` ```{attention} -We do not currently build images for Apple Silicon/amr64; however the amd64 images should work on these systems, albeit with reduced performance. +We do not currently build images for Apple Silicon/arm64; however the amd64 images should work on these systems, albeit with reduced performance. ``` The image bundles the astromatic binaries (`source-extractor`, `psfex`), diff --git a/docs/source/testing.md b/docs/source/testing.md index 6808c07b2..f397abf13 100644 --- a/docs/source/testing.md +++ b/docs/source/testing.md @@ -7,30 +7,13 @@ the bundled example pipeline. ## The automated test suite The test suite runs with [pytest](https://docs.pytest.org/) under a single -discovery root, `tests/`, with one tier per subdirectory: - -- `tests/module/` — **unit, property-based, and integration** tests for - the analytic surface: pure helpers and geometry (coordinate round-trips, - postage-stamp shapes, the MegaCam CCD flip), tested both by example and with - [Hypothesis](https://hypothesis.readthedocs.io/) property tests where a - function has a clear invariant. Integration tests that need the astromatic - binaries (Source Extractor, PSFEx) build synthetic FITS on the fly rather - than requiring survey data. -- `tests/unit/` — **structural** tests, one parametrized case per file: every - shell script parses (`bash -n`), every example config parses, every - `shapepipe.*` submodule imports, every `[project.scripts]` entry handles - `-h`, and every `*_runner.py` carries its `@module_runner` metadata. Cheap, - broad, and good at catching the dumb-but-real regressions (a syntax error, a - broken import, a renamed entry point). -- `tests/science/` — **fast scientific guardrails** (controlled simulations - with a known answer) and `tests/cluster/` — **candide guardrails** that read - real on-disk catalogs; the latter are marked `candide` and auto-skip off the - cluster. See `tests/README.md` for the full layout and markers. - -**CI is the single gate.** On every pull request and every push to `develop`, -`.github/workflows/deploy-image.yml` builds the image and runs the suite -*inside it* — so the tests exercise exactly the environment that ships. A green -check means the container actually works. +discovery root, `tests/`. See `tests/README.md` for the tiers, markers, and +machine-specific requirements. + +`.github/workflows/deploy-image.yml` builds the image and runs tests inside it +on branch pushes and pull requests targeting the integration or release branches. +See that workflow for the suite, science checks, workflow-DAG checks, and binary +and example smoke runs. ### Running it yourself diff --git a/docs/source/workflow.md b/docs/source/workflow.md index 624a9819d..816364a29 100644 --- a/docs/source/workflow.md +++ b/docs/source/workflow.md @@ -2,8 +2,7 @@ The end-to-end CFIS production run is driven by a [Snakemake](https://snakemake.readthedocs.io) workflow that lives in the -`workflow/` directory of the repository. It replaces the older bit-coded bash -job chain (`job_sp`) that this documentation previously described. +`workflow/` directory of the repository. The workflow owns: @@ -19,6 +18,6 @@ repository. ```{note} The PSF-validation and post-processing configurations that the workflow does -not yet cover are kept separately in `example/cfis/`; see the README there and +not cover are kept separately in `example/cfis/`; see the README there and [Post-processing](post_processing.md). ``` From 6c919f533918822ec8c40dbebac013f867b794f6 Mon Sep 17 00:00:00 2001 From: Cail Daley Date: Sat, 10 Oct 2026 18:47:23 +0200 Subject: [PATCH 30/43] docs(configs): present-tense comments in CFIS configuration comments Co-Authored-By: GPT-6.1 Sol --- example/cfis/README.md | 2 +- workflow/config/cfis/config_MCCD.ini | 2 +- workflow/config/cfis/config_exp_mccd.ini | 3 +- workflow/config/cfis/config_tile_Mc.ini | 4 +-- .../config/cfis/config_tile_PiViVi_mccd.ini | 5 ++- .../config/cfis/config_tile_PiViVi_psfex.ini | 10 +++--- workflow/config/cfis/final_cat.param | 3 +- workflow/config/cfis/star_selection.setools | 34 +++++-------------- 8 files changed, 20 insertions(+), 43 deletions(-) diff --git a/example/cfis/README.md b/example/cfis/README.md index 700f31e02..5a9eae75d 100644 --- a/example/cfis/README.md +++ b/example/cfis/README.md @@ -2,7 +2,7 @@ These configuration files cover PSF validation (`Ms`, `Pl`, `MsPl`, `valjoint`) and assorted post-processing / statistics steps that are **not -(yet) covered by the Snakemake workflow**. They are kept here for reference +covered by the Snakemake workflow**. They are kept here for reference and manual use with `shapepipe_run -c `. The per-module pipeline configurations driven by the Snakemake workflow live diff --git a/workflow/config/cfis/config_MCCD.ini b/workflow/config/cfis/config_MCCD.ini index a195d5241..64c33b504 100644 --- a/workflow/config/cfis/config_MCCD.ini +++ b/workflow/config/cfis/config_MCCD.ini @@ -71,7 +71,7 @@ GLOBAL_POL_INTERP = False # stars will be rejected. ie 9 is a conservative rejection. # USE_SNR_WEIGHTS : Boolean to determine if the SNR weighting strategy will # be used. -# For now, it needs the SNR estimations from SExtractor. +# Requires the SNR estimates from SExtractor. # PREPROCESSED_OUTPUT_DIR : (Required) Must be a valid directory to write the # preprocessed input files. # OUTPUT_DIR : (Required) Must be a valid directory to write the output files. diff --git a/workflow/config/cfis/config_exp_mccd.ini b/workflow/config/cfis/config_exp_mccd.ini index 3e1edfcdc..cdfdd9de9 100644 --- a/workflow/config/cfis/config_exp_mccd.ini +++ b/workflow/config/cfis/config_exp_mccd.ini @@ -234,8 +234,7 @@ NUMBERING_SCHEME = -0000000 [MCCD_PLOTS_RUNNER] -# Now MCCD has created a focal-plane PSF model, including all CCDS per images, -# thus single-exposure files +# The focal-plane MCCD model combines all CCDs into one file per exposure. NUMBERING_SCHEME = -0000000 # PSF model type read by mccd_plots_runner; the other option is "psfex" diff --git a/workflow/config/cfis/config_tile_Mc.ini b/workflow/config/cfis/config_tile_Mc.ini index 695654523..18429216c 100644 --- a/workflow/config/cfis/config_tile_Mc.ini +++ b/workflow/config/cfis/config_tile_Mc.ini @@ -83,7 +83,7 @@ SHAPE_MEASUREMENT_TYPE = ngmix # Save per-epoch PSF shapes and epoch identity (HSM_*_PSF_n, EXP_ID_n, # CCD_n). The campaign merge needs one column schema across all tiles, so # the slot count is fixed rather than the sexcat's geometric max(N_EPOCH)+1. It must -# exceed the survey's maximum epochs per object with margin (smk-g7: max 6 +# exceed the survey's maximum epochs per object with margin (maximum 6 # over 8 inspected tiles); an object with more epochs than slots is an error. SAVE_PSF_DATA = True N_EPOCH_SLOTS = 12 @@ -91,7 +91,7 @@ N_EPOCH_SLOTS = 12 # The UNIONS sky-fixed masks, queried once per object and written as one # MASK_