make_cat: write the final catalogue once per save stage, node-local - #939
Merged
Merged
Conversation
FITSCatalogue.add_col rebuilt the table HDU from every existing column plus the new one and rewrote the whole file each call. make_cat appended ~243 columns this way, so tile_make_cat spent 10-20 min per tile in astropy writeto on NFS scratch. FITSCatalogue.add_cols appends a dict of columns with one rebuild and one write; add_col is add_cols with one item, and the per-column FITS type, repeat count and TDIM logic lives in _make_fits_col. make_cat uses it for TILE_ID/TILE_UNIQUE_ID, for each SaveCatalogue.process stage (ngmix, PSF slots) and for the MASK_* columns: four writes per tile. tests/module/test_file_io_add_cols.py checks that add_cols writes the same file, byte for byte, as successive add_col calls, for plain and SExtractor-layout (table between other HDUs) catalogues and for int16, int32, int64, float32, bool, string, vector and 2-D columns. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
make_cat rewrites its whole catalogue at each save stage. On NFS scratch one write of a 33K x 271 tile catalogue costs ~19 s against ~0.4 s on node-local disk, so the build belongs node-local even at four writes. make_cat_runner takes an optional WORK_DIR: the catalogue is built there and moved to the run's output directory once complete. A work file left by an earlier attempt is removed first, since save_as_fits appends to an existing file. Without WORK_DIR the catalogue is built in the output directory as before. config_tile_Mc.ini sets WORK_DIR = $SP_LOCAL/make_cat, the per-tile node-local store tile_local() already exports for tile_make_cat and that its EXIT trap removes. tile.smk is unchanged: the published path, the completeness check and the final_cat copy all read the run's output directory as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… path The runner WORK_DIR test runs ngmix, per-epoch PSF slots and one external mask band, staged and in place, and checks the published bytes match and the stage columns carry the expected values. A new add_cols test covers new_cat/new_cat_inst with hdu_no and ext_name on a SExtractor-layout catalogue: the source is untouched, surrounding HDUs carry over, and the output equals successive add_col calls. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
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.
Problem
tile_make_catspends 10–20 min per tile rewriting its output FITS file.FITSCatalogue.add_colrebuilds the table HDU from every existing column plus the new one,writeto(overwrite=True)s the whole file, then closes and reopens it. make_cat adds ~243 of the final ~271 columns this way:TILE_ID/TILE_UNIQUE_ID, the ngmix block and the per-epoch PSF slot block (SaveCatalogue.process), and theMASK_*columns. So the catalogue gets written roughly 243 times.Measured on nibi:
tile_make_catworker: ~94% of samples in astropywritetounderSaveCatalogue.process'sadd_colloop.add_colcosts 2.05 s per column on NFS/scratchand 0.06 s node-local.Change
FITSCatalogue.add_cols(columns)appends a dict of columns with one HDU rebuild and one write.add_colnow just callsadd_colswith one item. The per-column FITS type, repeat count,TDIMand string-width logic moved into_make_fits_colwithout changes, so both paths build the same columns.TILE_ID+TILE_UNIQUE_ID, once perSaveCatalogue.processcall (ngmix, PSF slots), and once for allMASK_*columns. That's four writes per tile instead of ~243.make_cat_runnertakes an optionalWORK_DIR. The catalogue is built there, then moved to the run's output directory once it's complete. Any work file left by an earlier attempt is removed first, becausesave_as_fitsappends to an existing file.config_tile_Mc.inisetsWORK_DIR = $SP_LOCAL/make_cat, the per-tile node-local store thattile_local()already exports fortile_make_cat. That rule's EXIT trap already removes the store.tile.smkis unchanged: the published path, the completeness check and thefinal_catcopy all still read$SP_RUN/output/run_sp_tile_Mc/.... No params changed, so finished tiles are not rerun.Verification
tests/module/test_file_io_add_cols.py: oneadd_colscall produces a file byte-identical to successiveadd_colcalls (each of which writes and reopens the file). Also checks headers, column names/formats/TDIMand data HDU by HDU. Covers a plain catalogue and the SExtractor layout (table between other HDUs), with int16/int32/int64/float32/bool/string/vector/2-D columns. Also checks that a non-array column raises before anything is written, and thatnew_cat/new_cat_instwithhdu_no/ext_nameleaves the source untouched, carries the surrounding HDUs over, and matches theadd_colchain.tests/module/test_make_cat.py::test_make_cat_runner_work_dir_publishes_same_catalogueruns all three save stages (ngmix, per-epoch PSF slots, oneMASK_EXT_PATHSband). The catalogue published throughWORK_DIRis byte-identical to one built in place,WORK_DIRis left empty, and a stale work file does not leak in. If the stale-file removal is disabled, this test fails.Targeted module tests (
test_file_io_add_cols,test_make_cat,test_make_cat_mask_ext,test_mask_query) and the config unit tests pass (72 module tests, 108 unit tests) in theshapepipe-develop-240b37e4container.Real data (Nibi job 23055211): the g11 tile 181.308 catalogue (34,946 rows × 271 columns) was rebuilt from its first 28 columns, adding the other 243 once through develop's per-column
add_coland once throughadd_cols.add_col× 243add_cols/scratchBoth outputs are byte-identical (md5
9a43e3be…, 69,382,080 B), with equal headers and column names. The script'sall data equal: Falseline comes fromnp.array_equalwithoutequal_nanon NaN-bearing columns; the identical md5 settles equality. With the node-local working file (WORK_DIR), make_cat's catalogue writes drop from ~14 min on NFS to under a second, plus one copy.Full suite (Nibi job 23055353,
pytest tests -m "not slow"in the develop SIF): 831 passed, 4 skipped, 2 deselected.Out of scope
make_post_process(tile_detect) adds a single column (N_EPOCH), so batching doesn't apply there. Its cost is one rewrite of the LDAC sexcat, on top of the per-epochsave_as_fitsHDU appends before it._get_fits_col_typemapsnp.bool_toD. Wire the UNIONS external healsparse masks into the workflow (MASK_<flag value>_<name> columns) #886 changes it toL.add_colsgoes through the same function, so whichever mapping lands applies to both paths.TDIMfor columns with two or more trailing dimensions is written in numpy order rather than FITS order, so a(n, 2, 3)column reads back as(n, 3, 2). This behaviour already exists, andadd_colskeeps it so output stays identical. No current final_cat column is multi-dimensional; the g11 catalogue carries noTDIMcards.Merge notes
git merge-treeagainst #886 (feat/wire-external-masks), #925 (feat/uberseg-seg-vignet) and #933 (fix/dr6-windowed-positions) is clean. Against #887 (feat/instrument-defect-map) the only conflict is inworkflow/README.md, which #887 already has withdevelopitself; this branch doesn't touch that file. Ondevelop,cfis_image_sims/config_tile_Mc.iniis a symlink to the cfis one, so it picks upWORK_DIRtoo. #886 replaces that symlink with a regular file, so after it merges, image-sims builds in place until theWORK_DIRline is added to its copy too (a one-line follow-up; output is identical either way).Claude Opus 5.5 on behalf of Cail
🤖 Generated with Claude Code