Code review

[gsleap]

1.10.2 26-Aug-2026

• Clean up of ruff errors.

1.10.1 26-Aug-2026

• Fixed gain-outlier pages failing with [Errno 12] Cannot allocate memory on a 256-tile picket fence (seen via cal_utils). plot_outlier_gains() created its ProcessPoolExecutor with no max_workers, so it spawned os.cpu_count() workers, and a stitched page peaks at ~322MB measured (10800x3600px for the cal_utils default of 16 tiles per page: a 156MB raw RGBA buffer, roughly doubled because savefig(bbox_inches="tight") renders once to measure the bounding box and again to write the file). On a many-core calvin node that was tens of GB of live render buffers. The pool is now bounded by _max_render_workers(), which takes the smallest of the page count, the CPU count, and how many page-sized allocations fit in a fraction of the memory actually available. Memory detection checks the cgroup limit before the node's MemAvailable, since these run as Slurm jobs and the node can have hundreds of GB free while the job is confined to a fraction of it; if neither can be read it falls back to 4 workers rather than assuming plenty. Page geometry now comes from a shared _page_grid() helper used by both the renderer and the estimator, so the memory estimate cannot drift from the figure actually created. plot_outlier_gains() also takes an optional max_workers override. Note the per-worker bundle copy was measured at only ~5% of the per-page peak (16.5MB against 322MB), so slicing the bundle per page was deliberately not done -- it would have meant threading a tile offset through the renderer for a negligible saving.
• Picket-fence post-hyperdrive performance. Benchmarked against real fixtures (tests/data/1391522232, 24 pickets x 32 chanblocks, vs tests/data/1094488624, 1 file, both 768 total chanblocks): the group-level stages are not picket-sensitive at all -- 127.6s vs 126.1s, of which ~98% is phase fitting, which already runs on the whole group's concatenated chanblocks. The 3-5x runtime gap was entirely in the per-file loops in process_solutions().
• plot_outlier_gains() was the bulk of that gap: it was called once per solution file, so a 24-file observation created 24 ProcessPoolExecutors and rendered 120 pages against 4 for a contiguous observation, for the same number of data points (4 of 24 files measured at 223s, i.e. ~22 minutes extrapolated, vs 56s contiguous). It now renders one paginated set for the whole observation from a single pool: 5 pages, 22.4MB, 186s single-core measured. Outlier detection is untouched and remains strictly per file -- a polynomial fit across a picket-fence frequency gap would be meaningless -- so only presentation changed.
• Gain-outlier plots now stitch every coarse channel onto one compressed ("broken") x-axis instead of one plot per picket, which is what a human reviewer asked for: 24 separate plots per observation was unmanageable. Each picket keeps its true uniform spacing, gaps are compressed to a fixed token width (STITCH_GAP_CHANBLOCKS) with a dashed break line at every boundary, and ticks are labelled with the real coarse channel number. Plotting against true frequency was rejected: 1391522232 spans 78.7-241.2 MHz with only 18.9% of that span covered by data, so over 80% of the axis would have been empty. A real broken axis (one subplot per segment) was rejected too -- ~1500 axes per page would have cost more than the per-file plotting it replaced. Stitched pages use wider subplots on a narrower grid (STITCHED_SUBPLOT_WIDTH_IN/STITCHED_TILE_COLS), since 768 chanblocks at the single-file width gave only ~38px per picket; single-file observations keep the previous layout unchanged. Note two behaviour changes: output filenames no longer carry a _ch<N> component (generate_plot_index_file matches on the gain_outliers_tiles substring, so the index still categorises them correctly), and the per-tile "% Good" summary is now counted across the whole observation rather than per picket.
• Per-channel axvspan shading in the gain-outlier plots replaced with a single masked fill_between(..., step="mid") per axis. Equivalent output, but with every file's chanblocks now on one axis the old per-channel loop could have added tens of thousands of Rectangle patches to a single page.
• generate_hyperdrive_plots() is now driven by generate_hyperdrive_plots_for_files(), which runs the per-file invocations concurrently on a thread pool (each is an external hyperdrive process, so the GIL is released while waiting on the child). A picket fence previously did 48 serial process launches -- 24 for the "before" pass and 24 for "after" -- against 2 for a contiguous observation. Failures are collected and returned rather than raised, so one bad file neither aborts the rest nor fails an otherwise good calibration.
• That parallelism required fixing the "before" rename in generate_hyperdrive_plots(), which globbed the whole output directory for *_solutions_amps.png. It was only correct serially (an already-renamed file stops matching), cost a full directory scan per file (O(N^2) for a picket fence), and would have had concurrent calls renaming each other's output. It is now scoped to its own input file's stem -- still a glob rather than hardcoded _amps/_phases suffixes, so a hyperdrive version emitting a different plot type still gets its "before" copy protected, and it now warns if a run that reported success produced no matching plots.
• HyperfitsSolution.results now caches the RESULTS HDU instead of reopening the file on every access, and HyperfitsSolutionGroup.results no longer reads each file twice per access (once in the length-validation loop, once in the concatenate). weights goes through that several times per pipeline run, so a 24-file observation was opening solution files 192 times per run against 8 for a contiguous one, over a shared filesystem. A missing RESULTS HDU is cached as missing, so it keeps raising KeyError for the uniform-weights fallback without re-reading. Safe because write_jones() only rewrites the SOLUTIONS HDU -- there is a regression test pinning that.

1.10.0 26-Aug-2026

Breaking changes

• needs coordinated deployment: mwax_subfile_distributor's web service endpoints which change state (/shutdown, /pause_archiving, /resume_archiving, /dump_voltages, /release_cal_obs) are now POST only; they previously accepted GET as well, meaning anything that speculatively fetched a URL could stop the correlator or dump the voltage buffer. /status still answers GET. In-repo callers have been updated (calvin_processor now POSTs to /release_cal_obs), but external callers must be updated too: the M&C system calls /pause_archiving, /resume_archiving and /dump_voltages, and FREDDA triggers /dump_voltages. Deploy the new mwax_subfile_distributor only once those callers use POST, or those calls will start returning 405 Method Not Allowed. utils.call_webservice() gained a method parameter to support this.

Bug fixes

Fixed a number of bugs found during code review:

• calvin_processor: {obs_id}_residual.png -- the phase-residual polynomial fit was including zero-weight (flagged) channels. np.logical_and is a binary ufunc, so the third positional argument was being taken as out= rather than as a third condition, silently discarding the weights > 0 filter.
• calvin_processor: HyperfitsSolutionGroup.weights returned an array sized from only the FIRST solution file's chanblocks when a file had no RESULTS HDU, so for a picket-fence observation spanning several files it was silently shorter than the concatenated chanblock axis callers index it against. Added a multi-file regression test; the existing test only covered the single-file case, which is why this was missed.
• calvin_processor: a failed metafits download logged (and recorded in the database) only the first fragment of its error message, discarding the underlying exception -- the continuation lines were separate statements rather than an implicit string concatenation.
• run_command_ext() bound NUMA node 0 but run_command_popen() did not (>= 0 vs > 0), even though 0 is a valid node and -1 is the "do not bind" convention. Both now share one _apply_numa_binding() helper. Also corrected run_command_popen's numa_node annotation, which said int while the body handled None.
• cal_utils: the "all solution files must be for the same obsid" check compared the first filename against itself, so it never fired and mixed-observation inputs were accepted silently. Also fixed obs_id being assigned a str despite its int annotation, and removed an unreachable branch that referenced a non-existent args.solution_path.
• cal_utils: --gain-max-cutoff help text claimed a negative value disables the check. It does not -- only --no-gain-max-cutoff does, and a negative ceiling would flag every channel of every tile.
• cal_utils: corrected three wrong statements in the module docstring (phase outliers described as flagged rather than report-only; bad entries described as replaced by frequency interpolation when they are NaN'd; a Usage block citing flags that do not exist).
• vdif_cat: a .vdif file not matching the expected name pattern raised AttributeError instead of being skipped, as the log message referenced a non-existent args.obs_id.
• validate_filename(): the "filename has no extension" check tested len(os.path.splitext(...)) == 2, which is always true, so such filenames were reported with a misleading "Unknown file extension" message instead.
• read_subfile_values(): a PSRDADA keyword appearing twice in a header counted twice towards the "found them all" early exit, so a later requested key could be missed.
• add_digital_gains_column() returned a fits.BinTableHDU that escaped its with fits.open(...) block; astropy loads HDU data lazily, so using it after the file was closed was unsafe. It now returns a bool indicating whether the column was added.
• Fixed a worker thread's fatal error being reported as a clean shutdown- mwax_subfile_distributor: when signalling the beamformer via redis failed (after push_message_to_redis's own 3 retries), the handler called sys.exit(3). That runs on a worker thread, where sys.exit() only raises SystemExit in that thread -- the thread died, the exit code was discarded, and the main loop then logged "Completed Successfully" while main() exited 0. A redis outage therefore silently stopped subfile distribution on that host and looked like a clean stop to systemd, so it would not be restarted and would not alert. There were three such thread-level sys.exit() calls (exit codes 2, 2 and 3).
• Worker code now calls a new request_fatal_shutdown(exit_code, reason) on the processor, which records the intended exit code, logs it, and asks the main loop to stop. The main loop reports the failure rather than claiming success, and main() exits with that code. The first caller wins, so the exit code reflects the root cause rather than a knock-on failure. Redis being unreachable remains deliberately fatal for mwax_subfile_distributor: shutting down is what raises the alert.
• mwacache_archiver: had the identical "worker died -> log Completed Successfully -> exit 0" path in its main loop and main(). Same mechanism applied. Both daemons also now exit 1 (not 0) when initialise() or start() raises.
• Added regression tests.
• Test suite: removed the ResourceWarning: unclosed file warnings from test005 (ConfigParser.read() takes a path, so no handle needs opening) and test014 (Path.read_text()). The suite is now warning-free apart from one PendingDeprecationWarning raised inside seaborn.
• mwax_subfile_distributor / mwacache_archiver: the queue workers' failure backoff was linear, not exponential, despite every docstring and the README describing it as exponential. It was initial * factor * consecutive_failures (2, 4, 6, 8, ...) rather than initial * factor**(n-1) (1, 2, 4, 8, 16, ...), so a persistently failing item was retried far more aggressively than intended. Both QueueWorker and PriorityQueueWorker now share a single calculate_backoff_seconds() helper rather than each computing it inline, so they cannot drift apart again. Added regression tests.
• mwax_subfile_distributor / mwacache_archiver: stop() sets a threading.Event to interrupt an in-flight backoff wait, but nothing ever cleared it. After the first stop(), every subsequent event.wait(backoff) returned immediately, silently disabling backoff for the rest of the process's life. start() now clears it. Added a regression test.
• MWAXDBHandler.execute_single_dml_row() carried its own @retry decorator identical to the one on execute_dml, which it does nothing but call. The two nested retries multiplied: up to 9 attempts instead of 3, and a worst case of roughly 5 minutes instead of 1. The redundant outer decorator has been removed so the retry policy is defined in one place.
• utils.call_webservice(): max_retries ran one more attempt than its name and docstring implied (while attempt <= max_retries starting from 0). It now means exactly that many attempts at the whole URL list. Note this is a small behaviour change: the default max_retries=3 now makes 3 attempts rather than 4, and release_cal_obs (which passes 1) makes 1 rather than 2 -- that call already sits inside a 60-minute outer retry loop, so it is not weakened in practice. Failed attempts are now also logged individually.
• utils.run_giant_squid(): retry backoff is collapsed to ~0 under pytest. With the defaults the loop slept 10+20+40+80+160 = 310 real seconds before giving up, so a single unit test took over five minutes whenever the giant-squid binary was absent. The retry logic is still exercised; only the wall-clock wait is skipped.
• Added fitsid ordering (descending) to the calvin plot upload code and batches so that new fits get uploaded quickly if there is a big backlog.
• calvin_controller plot upload: plot_upload_interval_secs is a sleep between passes, not a fixed-rate timer, so with the production value of 600s a backlog drained at only one batch (100 fit dirs) per 10 minutes while the thread sat idle in between. upload_published_fit_dirs() now returns whether the path still has fit dirs waiting, and a pass that left a backlog behind sleeps PLOT_UPLOAD_BACKLOG_DELAY_SECS (5s) instead of the full interval. The base directory is still re-scanned every pass, so a newly published fit is still uploaded ahead of the backlog. A total failure still falls back to the full interval and per-path backoff rather than hot-looping.
• calvin_controller plot upload: the shutdown event was only checked once per pass, so stop() during an in-flight pass had to wait out every rclone transfer across all seven upload paths. It is now checked between paths and before each fit dir, and any dir not reached is simply left published for the next run.

Code review changes:

• Removed stale, unused code and dependencies.
• Reformatted all of src/ and tests/ with ruff format at the configured 120 character line length. Many files had been formatted at 88 characters previously, so line wrapping was inconsistent across the codebase. No code changes: verified by comparing the AST of every file before and after. Also added a [tool.ruff.format] section to pyproject.toml recording the intended style, so this does not drift again.
• Added a ruff format pre-commit hook (.pre-commit-config.yaml) so formatting stays consistent automatically rather than relying on remembering to run it. Enable it in a clone with uv run pre-commit install. The hook version is pinned deliberately, since ruff format output can change between ruff releases. ruff check is not part of the hook yet: the codebase still has ~90 outstanding lint findings, which would block commits to any file containing one.
• calvin_processor / mwacache_archiver: the two near-identical copies of generate_hyperdrive_plots() have been merged into one (in mwax_calvin_plots.py). The removed copy accepted max_amp but not before, and ignored run_command_ext's return code, so a failed hyperdrive run was reported as a success -- which meant update_calvin_plots_and_index's "exit early on failure" check could never fire. The surviving implementation takes both parameters and checks the return code.
• mwax_subfile_distributor: ChecksumAndDBProcessor and OutgoingProcessor now requeue a failed file to the back of the queue instead of retrying it in place. Order does not matter for either, and retrying in place meant one permanently-bad file blocked the head of the queue indefinitely, stalling everything behind it.
• mwax_subfile_distributor: use shutil.move() rather than os.rename() when routing files between the incoming/outgoing/dont_archive directories, so a configuration which places those paths on different filesystems does not raise EXDEV. This matches what the rest of the codebase already does.
• mwacache_archiver: the per-host recursive config option was read but never used -- incoming paths were always watched non-recursively regardless of the setting. It is now passed through to the watcher.
• calvin_controller / calvin_processor: create_sbatch_script()'s request_ids parameter was annotated list[str] while submit_sbatch()'s was list[int], and the same call site converted for one and not the other. Request IDs are an integer database column, so they are now list[int] throughout.
• calvin_processor: insert_calibration_fits_row() no longer rolls back the transaction itself. The caller owns the transaction (it runs inside a with conn.transaction(): block and raises on failure), so both were trying to unwind the same transaction.
• calvin_processor: flag_amplitude_outliers() and run_flagging_pipeline()'s mad_residual_threshold default is now 10.0, matching the shipped config and cal_utils --mad-threshold. It was 5.0, so calling either directly without a config value behaved differently to the deployed pipeline. The corresponding caveat in CALVIN.md has been removed.
• calvin_processor: removed a redundant np.round() wrapped around a floor division, which was a no-op.
• docs: make_illustrations.py now uses iterative_poly_clip_batch, the implementation the production pipeline actually calls, rather than the per-tile iterative_poly_clip. The two differ in their zero-MAD handling, so the illustration could have shown behaviour the pipeline does not have. The generated PNGs are unchanged.

Documentation and comment corrections found during code review. No executable code changed (verified by comparing every file's AST with docstrings stripped and other strings blanked), other than renaming two functions in docs/img/make_illustrations.py

• HyperfitsSolutionGroup.combined_tile_flags said refant did not use it and was "left as-is for now" -- refant had since been switched over, so the two docstrings directly contradicted each other.
• fit_phase_line still documented its sigma-clip as rejecting "outliers beyond 2*resid_std" in three places (the niter arg, the quality-metric comment, and the _MIN_CLIP_THRESHOLD_RAD comment). It has used a robust median + MAD clip since the phase-fit speedup work.
• mwax_calvin_solutions' module docstring listed the flagging pipeline in the wrong order and omitted the gain-magnitude cutoff stage.
• flag_mostly_bad_tiles omitted GAIN_MAX_CUTOFF from its list of per-channel reasons.
• populate_index_json_entry said only .png/.tsv/.txt were supported; it also accepts the two solutions.fits flavours.
• HyperfitsSolutionGroup.__init__ documented metafits as a list, and get_metafits_chan_info referred to "all metafits files". There is one.
• inject_beamformer_headers said it appends to the subfile header; it overwrites the last line.
• remove_file claimed up to 5 retries; the decorator allows 3.
• run_hyperdrive described its input as one uvfits file per coarse channel; it is one per contiguous coarse-channel band.
• copy_file_rsync's docstring had source and destination the wrong way round -- its only caller pulls a remote file to a local directory.
• utils.py's module docstring and the README both documented an MWAXSubfileDistirbutorMode enum which does not exist anywhere in the codebase.
• estimate_birli_output_bytes' comment gave the baseline count as tiles * tiles + 1.
• iterative_poly_clip_batch claimed to be "mathematically identical" to the per-tile version. It now documents the one real difference (zero-MAD tolerance) and why make_illustrations.py uses the batch version.
• get_unattempted_calibration_requests used a Parameters: block documenting a hostname argument it does not take.
• _checksum_and_insert_db's tri-state return is now spelled out (None means success), and the Raises: section no longer documents an exception it catches rather than raises.
• VisStatsProcessor.handler now states that a failed mwax_stats run is deliberately tolerated.
• README Module Reference: corrected entries for version.py, mwax_db.py, mwa_archiver.py (documented a function deleted earlier in this branch and omitted the one actually in use), mwax_wqw_pawsey_outgoing.py, mwax_calvin_utils.py and mwax_calvin_solutions.py; added missing entries for mwax_hyperdrive_solutions.py and mwax_calvin_plots.py.
• README: the six command line utilities (cal_utils, update_calvin_plots_and_index, generate_index_json, fits_inspect, print_metafits_info, vdif_cat) were entirely undocumented. All 10 console scripts are now listed.
• CALVIN.md: renamed the phase-fit illustrations from step3a/step3b to step6a/step6b. Phase-outlier detection became Step 6 when the pipeline was reordered, but the filenames (and a cross-reference in mwax_calvin_plots.py) still said Step 3.
• Added docstrings to the 13 public classes and functions that had none -- including all eight mwax_wqw_* processor classes and ArchiveLocation.
• mwax_priority_queue_data's module docstring example called q.put(1, 'path/file'), which is not how PriorityQueue.put() works.
• Fixed typos: existiing, retured, reciever, Atempting, sliently, visibilitlies, dowload, requyest, submmited, deatils, a dangling "database" in a log message, and a missing space in another.
• Replaced the # FIX 1:/# FIX 2:/# FIX 3: changelog-style annotations in run_hyperdrive with comments stating the invariant each one maintains.

Formatting

• Wrapped all 60 remaining lines over the configured 120 character limit, clearing every E501 finding (ruff check is now down from 86 findings to 26). These are the lines ruff format cannot fix for you, since it will not split a string literal. Purely mechanical: verified by comparing every file's AST with all string constants' whitespace collapsed, so no word, punctuation mark or format placeholder was altered and no code changed. ruff check is still not a CI/pre-commit gate: the remaining 26 findings are B905 (17, zip() without strict=), B017, B904, B007 and B018, which need real decisions rather than reflowing.

Tooling

• Added a GitHub Actions CI workflow (.github/workflows/ci.yml), running on pushes to main, on every pull request, and on demand. Two jobs: a fast lint job (ruff format --check, ty check) and a test job running the suite against Python 3.12 and 3.13. Until now nothing was enforced on push -- the pre-commit hook only protects developers who have run pre-commit install. Notes:
  • ruff check runs but is deliberately not a gate yet: there are ~86 outstanding findings (mostly E501 on long string literals the formatter cannot split), so blocking on it would make CI red from the first run. Its findings appear in the job log and as a counts table in the run summary. The workflow comment records exactly what to change once ruff check . is clean.
  • The integration marked tests are not run: they need network access and the real giant-squid binary. They remain deselected by the addopts in pyproject.toml.
  • The test job has a 30 minute timeout; the suite currently takes ~11 minutes, dominated by test020_calvin_solutions.py.