clean up workflow image sims - #894
Draft
martinkilbinger wants to merge 17 commits into
Draft
martinkilbinger wants to merge 17 commits into
martinkilbinger wants to merge 17 commits into
Conversation
Image-sim m-bias ran through the legacy bash runner and example/cfis_image_sims, a config chain frozen at an older pipeline state (no background vignets, no bkg-rms ngmix, no neighbour/mom-fail flags; its default.* symlinks dangle since e32c4fb). An m measured that way does not calibrate the pipeline that makes the real catalogue. This makes the simulations an input of the same workflow: - input_type: data | image_sims selects $SP_CONFIG. config/cfis_image_sims is an overlay: real files only for the ingestion INIs whose input naming differs (tile Git/Uz/Fe, exposure Gie/Sp -- the sim get_images writes the real-data output names, so every later stage reads identical patterns), everything else symlinked into config/cfis. - psf_model: fake -- the simulations' true PSF. The exposure stage runs only SExtractor (background maps for the vignets); tile_vignets runs fake_interp_runner, which writes galaxy_psf from `psf_dict` under the name the shared configs read (${SP_PSF}_interp_runner). Star-bearing sims can run psfex/mccd unchanged. - unit_pre writes dashed tile numbers for sims (get_images substitutes them verbatim) and exports PSF_DICT when set. A data run's prologue is byte-identical (checked by diffing dry-run shell commands against the base), so no finished data unit is rerun by the params trigger. - bin/sp: SP_PROFILE selects profiles/<name>; SP_RUN_CONFIG replaces (not layers on) workflow/config.yaml and is snapshotted with the code; the venv is optional when snakemake is on PATH. container.py follows both. - profiles/candide: SLURM profile for candide (node-local /tmp bound as /local/scratch for the tile store). Note: completeness.py gains the `fake` tables, and its content hash is a param on every rule, so this lands at a campaign boundary like any completeness edit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KNJYtm9z6VeoChxtDLYn4W
…low layout create_final_cat.py -I globbed run_sp_tile_Mc_*, the legacy bash runner's dated run dir; the workflow writes an undated run_sp_tile_Mc. Accept both. The overlay's final_cat.param was a symlink to the real-data list, which names columns make_cat does not write (IMAFLAGS_ISO: the tile SExtractor has no flag image; NGMIX_MOM_FAIL: written as NGMIX_MCAL_TYPES_FAIL) and omits NUMBER and NGMIX_MCAL_TYPES_FAIL, which sp_validation's image-sim extract reads. It is now a real file: the legacy example/cfis_image_sims list plus NGMIX_NEIGHBOUR_FLAG, checked column by column against a smoke-run final_cat. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RSjZrhgsGCLAJCLb5Zjmc3
…de excludes The node-local tile store was /local/scratch/sp-<tile>, unique within a campaign but not across concurrent ones. The image-simulation shear branches are concurrent campaigns over the same tile IDs, and on candide /local/scratch is the node's shared /tmp: two branches' tile_shape jobs on n09 shared one store, one tile_vignets wiped it under the other's ngmix, and both failed. The failure can also be silent -- ngmix reading the other branch's vignets. image_sims now prefixes the store name with a hash of the run dir; data campaigns keep the bare name, so their shell commands (a rerun trigger) are unchanged. The tile_shape group's own slurm_extra (the --tmp floor) replaced the profile default and dropped candide's node excludes; its jobs ran on n09 and n17. profiles/candide restates both for the four members via set-resources (slurm_extra only; the rules keep their threads and attempt-scaled mem_mb, checked in a dry run). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RSjZrhgsGCLAJCLb5Zjmc3
config_exp_mccd.ini was never ported: it read split_exp and mask_runner outputs through INPUT_MODULE (mask_runner is gone since #847, and with it the pipeline_flag files), had no mask_query, chained setools and MCCD through last:, and left RUN_DATETIME at its default, so the run dir was dated and nothing downstream could find it. psf_model: mccd could not run. It is now the PSFEx chain up to the star selection -- same split-CCD inputs, flag image, mask_query and setools, so both models fit the same stars -- followed by mccd_preprocessing (all CCDs of the exposure into one training and one test catalogue) and mccd_fit_val (fitted_model-<exp>.npy, which the tiles' mccd_interp reads, plus validation_psf-<exp>.fits). merge_starcat and mccd_plots are dropped: they are campaign-level diagnostics, not per-exposure products. The completeness table's MCCD branch had preprocessing at 80 (per CCD; it is 2 per exposure), listed merge_starcat and the plots, and was all warning-only. It now carries the counts this chain produces, mandatory as for PSFEx: an exposure without a model has no PSF on any tile it overlaps. Checked on SKiLLS star sim 1z2z (exposure 2086792): 120/40/80/2 through preprocessing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VSU9pPZjpBjzWorAL88aKQ
Every MCCD fit in the ShapePipe image fails, for real data as for sims:
* MCCD builds its starlet filters by running Sparse2D's mr_transform through
pysap, and the image ships pysap without the Sparse2D binaries
("Sparse2d command 'mr_transform' not found"). mccd's FILTER_PATH escape
does not help: 1.2.4 passes the path string on as the filter array.
* mccd 1.2.4 calls np.vstack on a generator, which numpy 2 rejects, in the
hybrid model's graph constraint. Fixed upstream (CosmoStat/mccd b87d280),
not released.
The filters MCCD asks for are the B3-spline a-trous transform of a centred
Dirac, which shapepipe_auxiliary_mccd now computes in numpy and installs as
mccd_utils.get_mr_filters when mr_transform is not on PATH. Compared with
Sparse2D's own mr_transform -t 2 (51x51 and 41x41 at 3 scales, 33x33 at 4):
identical to the last bit. mccd.utils gets a numpy whose vstack takes a
generator, under numpy >= 2 only. Both are pinned in test_mccd_compat.py.
The durable fixes are in the image: mccd from upstream (or a release with
b87d280), and either Sparse2D or keeping the numpy filters.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VSU9pPZjpBjzWorAL88aKQ
MCCD fits one focal-plane model per exposure after the per-CCD stages: 85 min single-threaded for ~2500 stars and 8.3 GB (SKiLLS star sim 1z2z_1, exposure 2086792), and 48 min on 8 BLAS threads -- 1.75x for 8x the cores, which the thread caps forbid anyway. With threads: 8 the fit leaves 7 reserved cores idle for its whole length. The mccd chain now takes 2: the per-CCD stages run 2 wide (minutes), the fit is unchanged, and an exposure reserves about a quarter of the core-hours. PSFEx and fake keep 8. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VSU9pPZjpBjzWorAL88aKQ
Multi-epoch MCCD interpolation checked `"N_EPOCH" not in cat.get_data()`,
a membership test against the FITS record array, which current numpy
rejects ("Cannot compare structured or void to non-void arrays"): every tile
failed in tile_vignets with psf_model: mccd. psfex_interp already tests
`.dtype.names`; so does this now. With it, tile 233.293 of SKiLLS star sim
1z2z_1 interpolates the seven exposures' MCCD models (a few sparse CCDs
lose an epoch, as intended).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VSU9pPZjpBjzWorAL88aKQ
…lags With the ported exposure chain, the mccd fixes and the mccd_interp fix, psf_model: mccd ran SKiLLS star sim 1z2z_1 tile 233.293 end to end: seven exposures' focal-plane models (120/40/80/2/2 each), mccd_interp's galaxy_psf store, vignets, ngmix and a final_cat of 21857 objects. tile_vignets' mccd counts are now mandatory as for psfex, and the README no longer calls mccd "wired but unvalidated". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VSU9pPZjpBjzWorAL88aKQ
… per campaign bin/sp rewrites the source of the `/local/scratch` bind in the launch snapshot's profile (appending one when the profile has none) and creates the directory. tile_local() is untouched: its output is a params rerun trigger, a bind is not, so the key can be set on a resume. candide's 31 GB node /tmp cannot hold dense image-sim tile stores; image-sim campaigns there point this at NFS. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Qv5MwRiVKa8wnH665dWBia
write_if_changed() used Path.write_text(), which truncates before writing. run_report.py reads every logs/*.json right after compute finishes (the onsuccess hook) and can catch one mid-write as an empty file -- observed during the grid_3 image-sims acceptance test (exp_get_images.json for one exposure came back "Expecting value: line 1 column 1"). Write to a same-directory temp file and os.replace() it into place instead, which is atomic and closes the race.
The committed default carried one person's real nibi paths (tile_list, inputs, container, outputs) baked in. That means `sp run` was not actually the same command on every machine: anyone who forgot to set SP_RUN_CONFIG on a different cluster would either hit a confusing filesystem error somewhere downstream, or silently point at paths that happen to also resolve on their machine but belong to someone else's campaign. config.yaml now carries a REPLACE_ME sentinel for every machine- or campaign-specific path, with a top-of-file explanation that a real run always supplies its own via SP_RUN_CONFIG. The Snakefile checks the keys that have no safe fallback (tile_list, inputs.tiles/exposures, outputs.run_dir/index_db) right after the config file is read and raises one clear, identical error on any cluster if one is still the placeholder, rather than failing confusingly later or not at all. `container:` is left out of that check: a local sandbox or cached SIF is resolved before it, so a placeholder there is often harmless. Also cleans up several stale comments left over from earlier campaign states (smk-g4/g5 narrative, `clean`/`clean_tiles` comments describing "OFF" for a value that had already been changed to `true`) and two "knob" -> "setting" wording fixes.
…in the ini RETRIEVE=symlink was hard-coded in config_tile_Git.ini and config_exp_Gie.ini (both config/cfis and config/cfis_image_sims), which assumes a pre-staged local mirror. That's nibi's layout; candide's real-data ingestion fetches tiles/exposures from VOSpace (RETRIEVE=vos) and has no local mirror. Add `retrieve: symlink|vos` to the run config, validated at parse time, exported as SP_RETRIEVE by unit_pre() and interpolated into the four ini files. input_type=image_sims always forces symlink regardless of the run config's value, since simulation output is generated locally and never lives in VOSpace -- so a `data` run config's `retrieve: vos` reused as a sims campaign's base can't leak into the sims ingestion stages. Verified with a dry run against both input_type=data (retrieve: vos) and the existing image_sims acceptance-test config (still forces symlink) that SP_RETRIEVE renders correctly in each case.
…s a guard REPLACE_ME sentinels made the committed config unrunnable as-is on any cluster, including nibi -- its own actual mainline user. Restore the real, working nibi values (tile_list, inputs, container, outputs) so `sp run` with no other setup does something real and correct there, the same way it always used to. The REPLACE_ME check in the Snakefile stays: it now guards against a future edit that strips these defaults without supplying real ones, rather than gatekeeping every run. Running the committed default from candide now fails honestly on nibi's real, unreachable /project path (FileNotFoundError) rather than on the placeholder check -- correct, since candide cannot see nibi's filesystem and must supply its own paths via SP_RUN_CONFIG regardless.
machine: (the run config) and SP_PROFILE (the env var bin/sp reads to pick profiles/<name>/, default nibi) are two independent switches for the same fact -- which cluster this is -- set in two different places at two different times, with nothing stopping them from disagreeing: export SP_PROFILE=candide but leave a copied run config's `machine: nibi` unedited, and jobs run under candide's SLURM settings against nibi's data paths. Raise a clear WorkflowError at parse time if they don't match, skipped only when `machine:` is genuinely absent (a config predating this key). No code default for `machine:` -- guessing one on its behalf would defeat the point of the check. Also fixes a comment above the REPLACE_ME check left stale by the previous commit: it still called config.yaml "a universal template with no real paths in it" after nibi's real defaults were restored.
…defaults `machine: nibi` selects config.yaml's own real defaults; the same key now also indexes a `machines:` table carrying tile_list, retrieve, inputs, outputs and container for both nibi and candide, so switching `machine:` (kept consistent with SP_PROFILE by the existing check) is enough to point a bare `sp run` at the right cluster's paths. On candide, where the real survey (VOSpace, RETRIEVE=vos) and the SKiLLS simulations (local disk, RETRIEVE=symlink, a different container) need different values, a machine entry nests them under `data:`/ `image_sims:` instead of stating them flat. `$base_dir` in a string is substituted with that machine's own `base_dir:`, so nibi's block states its one common root once instead of four times. A value may be the sentinel `TBD` -- known to be needed, not yet known what it is (candide's real-data ingestion: no local mirror exists there, the real vos: paths and output roots aren't confirmed yet) -- checked at parse time with the same one-clear-error mechanism REPLACE_ME used to provide. Container resolution needed a specific bridge: container.py reads `container:` by regex off the run config FILE, independent of the Snakefile's `config` dict, so a machine-derived container (which only exists after this Python-side merge) would otherwise be invisible to it. Reused SP_CONTAINER (already the documented way to point resolve_image() at a specific image) instead of teaching container.py about `machines:` too -- set only when neither an explicit top-level `container:` nor a user-set SP_CONTAINER already exists, preserving the same override precedence as every other machine default. Also fixes SP_RUN_CONFIG to LAYER on top of config.yaml rather than replacing it wholesale (workflow.configfile(), called conditionally -- it's the plain method the `configfile:` directive itself compiles to, so it works inside an `if`; merges recursively per snakemake.utils.update_config). Necessary for `machines:` to reach the common case at all: every image-sims run and any one-off cluster override already goes through SP_RUN_CONFIG, and under the old wholesale-replace behaviour none of them would ever have seen the table. Safe against the original replace-wholesale concern (an external config silently inheriting an unrelated real campaign's paths) because `machines:` entries are well-scoped defaults gated on `machine:`/`input_type:` and independently checked against SP_PROFILE, not one campaign's actual values reused as another's fallback. Verified: the committed nibi default still fails honestly (on nibi's real, candide-unreachable path) rather than on a placeholder; a machine:/SP_PROFILE mismatch still raises before that; a candide+data run config (all TBD) raises the new clear placeholder error; and the grid_3 image-sims acceptance-test config, with only `machine: candide` added and its explicit retrieve:/container: removed, dry-runs identically to before except for the retrieve: addition already committed separately -- confirming the machines: table now supplies what that config used to have to state itself, without disturbing the already-completed run's resume state.
The machines: table was only understood by the Snakefile. bin/sp read outputs.run_dir/index_db and tile_store_root straight from the config file, so the committed config (outputs now under machines.nibi) broke `sp run` on nibi with a KeyError, and container.py only saw a top-level container: line. scripts/run_config.py now does the whole resolution (config.yaml, SP_RUN_CONFIG merged on top, then machines[machine] [input_type] for anything unset) and all three use it. - container.resolve_image() takes the resolved container explicitly; drops the SP_CONTAINER workaround, which had let a machine default outrank a user's cached SIF, against the documented order. - nibi's machine entry is nested under data: like candide's, so a nibi image_sims run cannot inherit real-data paths. - Unset required keys are reported like TBD, in one error. - README gains a "Run configuration" section; config.yaml, Snakefile and the ini comments are cut down. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013NzWDtdQbsK1VhvYbDnmTo
config.yaml gains `run: smk-g6`, and nibi's paths use $run instead of the literal campaign name. $base_dir and $run now expand in values set in the run config itself too, not only in machine defaults, so a sims run config can name its output directories by `run:`. A required path left holding an unexpanded $variable is reported like TBD. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013NzWDtdQbsK1VhvYbDnmTo
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Image simulations in the Snakemake workflow, per-machine run config (#891 step 3)
Step 3 of #891: SKiLLS image simulations run through the same workflow as the
real data, instead of a separate orchestrator. Also covers the candide profile
from step 1.
Summary
Input type and PSF model
input_type: data | image_simsandpsf_model: psfex | mccd | fake,validated at parse time.
fakerequiresimage_sims; sims with stars canstill use
psfex/mccd.config/cfis_image_sims/is an overlay ofconfig/cfis/: symlinks foreverything shared, real files only for the five ingestion configs whose
input naming differs,
final_cat.param, and the two fake-PSF configs.psf_model: fake:exp_psfruns SExtractor only;tile_vignetsbuilds thePSF with
fake_interp_runner(from Image simulations workflow #766) frompsf_dict. Completenesstables cover all three PSF models.
Run configuration (
workflow/config.yaml,workflow/scripts/run_config.py)SP_RUN_CONFIGis now merged on top ofconfig.yamlinstead of replacingit, so a run config only states what differs.
machines:table gives, per machine andinput_type, thetile_list,retrieve,inputs,outputsandcontainerused when the run configleaves them unset.
$base_dirand$runexpand in paths;TBDstops therun at parse time.
machine:must matchSP_PROFILE, so a run cannot use one cluster'sSLURM profile with another cluster's paths.
run_config.py) is shared by the Snakefile,bin/spandcontainer.py.retrieve: symlink | vosis a run-config key, exported asSP_RETRIEVE(previously
RETRIEVE = symlinkwas fixed in the ini files). Sims always usesymlink.Other
profiles/candide/.completeness.pywrites manifests and logs atomically;run_report.pycould previously read a log while it was being written.
For review (@cailmdaley)
machines.nibi.data, withrun: smk-g6. Theresolved values are identical to before (checked by resolving them on
candide), but
sp runitself has not been run on nibi with this branch.SP_RUN_CONFIGnow layers instead of replacing. Please check this fitshow you drive nibi campaigns.
TBD(VOSpace paths, output roots).completeness.pyexpects theget_imagesfile counts of the symlinksetup; the old bash table had different counts for vos.
Acceptance test
One
grid_3tile (262.288,1z2z_grid_3) throughsp runon candide,input_type: image_sims,psf_model: fake:status=success, 5/5 exposuresand all 9 tile stages ok,
final_cat-262.288.fitswith 1621 objects, ~14 minwall-clock.
Testing
grid_3tile on candide.machine/SP_PROFILEmismatch,TBDvalues,unset required keys, unexpanded
$run.tests/unit/test_workflow_run_names.pypasses.sp runon nibi with the committed config.psf_model: mccd.The history includes some back-and-forth on the config design; squash-merging
is probably cleanest.
🤖 Generated with Claude Code
https://claude.ai/code/session_013NzWDtdQbsK1VhvYbDnmTo
Acceptance test
One
grid_3tile (262.288,1z2z_grid_3) throughsp runon candide,input_type: image_sims,psf_model: fake:status=success, 5/5 exposuresand all 9 tile stages ok,
final_cat-262.288.fitswith 1621 objects, ~14 minwall-clock.
Testing
grid_3tile on candide.machine/SP_PROFILEmismatch,TBDvalues,unset required keys, unexpanded
$run.tests/unit/test_workflow_run_names.pypasses.sp runon nibi with the committed config.psf_model: mccd.The history includes some back-and-forth on the config design; squash-merging
is probably cleanest.
🤖 Generated with Claude Code