ngmix: each chunk holds only its own rows' tile stamps - #940
Open
cailmdaley wants to merge 1 commit into
Open
cailmdaley wants to merge 1 commit into
cailmdaley wants to merge 1 commit into
Conversation
Tile_cat memory-maps the tile catalogue (and the segmentation vignet catalogue) and copies the stamps of the chunk's rows only, into a ChunkStamps indexed by tile-catalogue row; rows outside the chunk raise IndexError. The per-object columns (obj_id, ra, dec, flux) stay full length. Ngmix.process passes its ID_OBJ_MIN/ID_OBJ_MAX bounds to Tile_cat. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017JQtu8oaZQEAqzbxZbPpWs
This branch has not been deployed
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
Snakemake runs N ngmix chunks per tile (16 in recent campaigns), each fitting the catalogue rows
chunk_rows(n_obj, ID_OBJ_MIN, ID_OBJ_MAX). Ondevelop,Tile_cat.get_datareads the whole tile SExtractor catalogue into memory (FITSCatalogue's defaultmemmap=False) andnp.copys the fullVIGNETcolumn, and the segmentation vignets the same way whenSEG_VIGNET_PATHis set. So every chunk keeps all ~36K 51×51 float32 stamps (357 MiB) for the whole run, although it only uses about 1/16 of them. While it loads, it also holds the full table read and the copy together.Change
ChunkStamps(column, rows)holds one stamp column for a range of rows. Like the full column, it is indexed by tile-catalogue row:stamps[i_tile]is rowi_tile's stamp. A row outside the chunk raisesIndexErrorand names the chunk's range, so code that reaches outside its chunk fails loudly rather than reading the wrong stamp.Tile_cat(cat_path, seg_cat_path=None, row_min=-1, row_max=-1)memory-maps the catalogue (memmap=True) and copies only the chunk's stamp rows (np.array(column[start:stop])), so the full column is never materialised. It setsself.rows = chunk_rows(...).obj_id,ra,decandfluxstay full length, socheck_wcs_centroid_offset, which iterates over everyobj_id, is unchanged. The seg catalogue is read the same way, and the row-alignment checks are unchanged.Ngmix.processpasses itsID_OBJ_MIN/ID_OBJ_MAXtoTile_catand still loops over its ownchunk_rows(...). The defaults (-1, unbounded) load the whole catalogue as before.Every consumer (
tile_cat.vign[i_tile],tile_cat.seg[i_tile]inprepare_postage_stampsand the uberseg checks, andflux/ra/dec/obj_id[i_tile]) indexes with the loop's global row, so none of them changes.Verification
tests/module/test_ngmix_tile_cat_chunk.pyuses an LDAC-layout tile catalogue with a row-aligned seg catalogue. ItsNUMBERvalues are gapped and shuffled, and its stamps carry-1e30markers and a NaN. The test covers six chunk shapes: unbounded, first, middle, last, open-ended, and the splitter's empty range. For each it checks that:memmap=False) read of the file in bytes and dtype, forVIGNETand the segVIGNET;len(rows)stamps and own their memory;IndexError;Separate tests check that the held stamps survive the file being overwritten afterwards (copies, not views of the map), that
ChunkStampsbounds work with numpy integer rows, and that a misaligned seg catalogue still raises.Real data, byte-identical. The input is tile 202.301 (36,022 objects) from the
nibi-checks/897-sexrun, whose exposure stores survive (clean: false). Its vignet and PSF stores were rebuilt withconfig_tile_PiViVi_psfex.iniand develop's code into a separate bench directory. ngmix ran three chunks of 150 rows (rows 1–150, 18001–18150 and 35873–36022) once withdevelop(615e73f) and once with this branch, in theshapepipe-develop-240b37e4container, as Nibi job 23087595. Each pair ofngmix-202-301.fitsoutputs is byte-identical (md514764eec…,718ba35f…,eca591e5…). The chunks fitted 107, 149 and 142 of their 150 objects (the rest have no valid epoch), so the comparison covers fitted rows and skipped rows.Load memory, one realistic chunk. The test loads chunk 8 of the 16-chunk partition that
ngmix_range.pycomputes for this tile (rows 16017–18148, 2,132 objects), and reads/proc/self/statusbefore and afterTile_cat(load_probe.py, allocation 23109257 steps 0 and 1; output saved inngmixrows-bench/probe-23109257.txt):RssAnon, kept for the whole run)VmHWM− baseline)MaxRSSof the load-only stepOn the branch, the load peak is mostly mapped file pages.
peak_probe.py(step 2, same file) holds the map open and showsRssFilerising by 366 MiB, which is about the whole 369 MiB file: readingNUMBER/XWIN_WORLD/YWIN_WORLD/FLUX_AUTOtouches every row, and readahead maps the rest. The chunk's stamp copy is the anonymous part,RssAnon+21 MiB. The mapped pages are clean page cache: the kernel can reclaim them, and the chunks on a node share them because they all map the same file. The mappings go whenget_data()returns and its localdata/seg_datareferences are dropped;close()alone does not unmap while those views are alive. After that,RssFileis back at its baseline. Ondevelop, the corresponding +724 MiB is anonymous memory in each chunk. Load time is dominated by I/O and varies with the page cache: in this run it was 1.8 s for develop and 0.07 s for the branch.Full ngmix run of that chunk. Steps 7 and 8 of allocation 23095876 ran develop and the branch concurrently. Both fitted 2,121 of 2,132 objects, and their outputs are byte-identical (md5
6edc824d…). Wall time was 7,032 s for the branch and 7,084 s for develop. Most of that is store I/O from NFS/scratch: production stages the stores node-local, and the bench does not. sacctMaxRSSis 2.83 GiB for the branch and 2.86 GiB for develop, so it does not resolve the difference. Nibi usesjobacct_gather/cgroup, and itsMaxRSSis the step cgroup's memory, which includes the page cache of the ~3 GB of sqlite stores each chunk reads (inferred).Tile_catlives for the whole ofprocess(), so the +360 MiB versus +21 MiB of anonymous memory measured above is what each chunk carries throughout the fit. On this tile, with 16 chunks, that is about 5.3 GiB less anonymous memory per tile.Full suite in the develop SIF (allocation 23095876,
pytest tests -m "not slow"): 851 passed, 4 skipped, 2 deselected.Composition with #925
#925 moves the seg stamps into the tile catalogue as a
SEG_VIGNETcolumn and holdsVIGNET/SEG_VIGNETas views of the in-memory table. That avoids the copy, but still keeps the whole table in every chunk. The two PRs conflict inTile_catand at theTile_cat(...)call inprocess. The resolution is mechanical: dropseg_cat_path, keeprow_min/row_max, and readwith
Tile_cat(self._tile_cat_path, self._id_obj_min, self._id_obj_max). #925's comment aboveself.flux("views into it, not copies, so it is held once") auto-merges, and has to go. Two of #925's tests assert the whole-column view design and need adapting:test_tile_cat_reads_seg_vignetcompareswith_seg.segagainst the full array, and should compare row by row instead;test_tile_cat_holds_the_stamp_columns_onceasserts thatvignandsegshare one buffer, andtest_ngmix_tile_cat_chunk.pyreplaces it. With that resolution, plus this PR's chunk test switched to aSEG_VIGNETcolumn, the module tests matchingngmix or tile_catpass on the merge (134 passed, allocation 23095876).#925 is now retargeted onto #933, and the resolution recipe (the diff saved as
ngmixrows-bench/compose925-resolution.diff) applies cleanly there; that last point comes from an independent review, not a rerun on my part.Merge notes
git merge-treeagainst #933 (fix/dr6-windowed-positions) and #886 (feat/wire-external-masks) is clean. Against #925, the conflict is inngmix.py(above). 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.Claude Opus 5.5 on behalf of Cail
🤖 Generated with Claude Code