Repository navigation
Testing and documentation improvements: unit tests, docstrings, CI docs gate, entry-point fixes - #161
Conversation
… build in CI - Rewrite AGENTS.md as a briefer reference; add the legend-dataflow-scripts package split, rule->script invocation styles, PRODENV/$_ config expansion, runnable commands, and CI data-scale constraints - Add CONTRIBUTING.md and link it from the README - Add inline comments to dataflow-config.yaml (values unchanged) - Add a build-docs CI job running sphinx-build -W via make -C docs - Fix five RST formatting warnings that failed the strict docs build Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- add missing par-geds-pht-lq entry point (invoked by pht_pars_geds.smk but never registered, so the rule failed with command not found) - remove create-chankeylist, which pointed at a module deleted in 7d4e578 - rename mistyped mutliprocess_mode config key; common.smk reads the correct spelling, so the old key was silently ignored Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d patterns - module docstrings for all 23 undocumented scripts/ modules, so the apidoc-generated API reference is no longer empty stubs - method docstrings for FileKey classes and CalGrouping; correct the per_grouper/run_splitter docstrings which all claimed to group by run - unit tests for methods/paths.py (tier/pars resolvers, link_external_paths) and methods/patterns.py (tier/pars/log patterns, out-of-cycle handling) - user manual: document multiprocess_mode and max_processes, and correct the $_ placeholder description (it expands to the config directory, not $PRODENV) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Tests for build_filelist (run expansion, analysis-run/ignore-key selection, key-part multi-selectors, per-tier search patterns, run concatenation, and end-to-end filelist building on touched files), merge-channels (yaml, pickle and shelve modes) and flow utils. Writing them uncovered two bugs in build_filelist.py, both fixed: - get_analysis_runs checked Path(ignore_keys_file).suffix where it meant analysis_runs_file, crashing on Path(None) whenever a runlist was given without an ignore file - build_filelist assumed dict-shaped ignore_keys and crashed on the flat lists produced from .keylist files (or when no ignore file is given) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…mode - merge-in-channels: yaml, pickle and lh5 modes, verifying processing-step identifiers become struct fields in the channel-keyed output - build_chanlist: channel selection by system and processable status, missing status-map errors, and per-channel par/plot filename generation - merge-channels: lh5 mode with real lgdo objects, including db file-reference rewriting via --in-db/--out-db Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ind_gen_runs The two snakemake script: files imported the injected snakemake object at module level, making them impossible to import (and test) outside a workflow run. The snakemake import is now guarded and the glue runs only when the object exists; the logic lives in plain functions taking setup/threads explicitly instead of reading the snakemake global. find_gen_runs parsed run info for concat tiers by splitting the full path on hyphens, which mis-parses whenever the production path itself contains one (e.g. prod-blind); it now splits the filename only. Tests cover log-file checking and cleanup, run discovery (with a hyphenated parent path as regression), FileDB config generation with and without PRODENV, filelist writing, and the snakemake glue itself by injecting a fake object into snakemake.script and executing the scripts with runpy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #161 +/- ##
===========================================
- Coverage 65.00% 53.85% -11.15%
===========================================
Files 8 34 +26
Lines 720 2863 +2143
===========================================
+ Hits 468 1542 +1074
- Misses 252 1321 +1069 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- FileKey.get_filekey_from_pattern raises ValueError naming the filename and pattern instead of returning None (every caller dereferenced the result, turning stray files into cryptic NoneType AttributeErrors); parse_keypart raises a named error on unparseable keyparts. - get_path_from_filekey: fix the dict-kwarg resolution, which raised StopIteration on an empty intersection (and would have raised "dictionary changed size during iteration" in its fallback); both copies now share _resolve_dict_kwargs. - paths: route all accessors through _get_path so an incomplete dataflow-config raises "paths.<key> is not set" instead of a bare KeyError. - patterns: raise ValueError (naming the tier) instead of bare Exception. - pars_loading: check validity.yaml existence with tier context before reading. - create_pars_keylist: validate both catalogs have an 'all' entry in apply_run_override. - cal_grouping: named errors for unknown partitions and missing 'default'; validate rNNN..rNNN run ranges at config load; warn when a partition resolves to zero par files before returning the placeholder log-file/timestamp fallbacks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Crash bugs:
- par-geds-pht-{aoe,lq,ecal-part} read args.files instead of
args.input_files (AttributeError on every invocation); aoe also called
run_aoe_calibration with a missing plot_dicts argument and a spurious
log= kwarg.
- raw_blind: import TextDB from dbetto (legendmeta no longer exports it),
fix the blind-curve channel-name lookup, use the real 'bsln' system
name (was 'blsns'), and refuse to write an unblinded output when no
blindable channel exists.
- merge scripts: NameError paths for --out-db without --in-db and empty
inputs; unknown output extensions now raise instead of writing nothing;
lh5 merge derives the output group via fkey.identifier with named
errors for missing identifiers/fields and mixed-channel inputs.
- eval/lq/ecal_part: plot_dicts undefined when run_lq is false; popped
'common' plot dict reused after pop; misc branch-scope fixes (fast.py
plot save, psp/average input matching).
Logic:
- build_filelist.expand_runs no longer skips entries when several
run ranges appear in one list; skm.py no longer mutates keep_fields
while iterating; tcm.py resolves per-fcid configs explicitly instead
of silently splatting the whole mapping; qc_phy reads each run's
files once (the baseline mask was misaligned).
- complete_run.check_log_files detects traceback blocks and reports the
exception line as an error, so crashed jobs no longer show up as
"no errors" in run summaries (build_log's excepthook output carries no
ERROR token).
Error messages: filelist/selection errors name available options and
expected formats; evt.py prechecks channel-map systems and
hardware_tcm_1; blindcheck names the channel and found peaks;
spms trigger-threshold messages fix broken f-strings.
Shared helpers (expand_filelist, check_pulser_mask, require_config_keys,
get_rule_config, get_channel_config) now come from
legenddataflowscripts.utils (the former scripts/par/utils.py duplicates
were removed); requires legend-dataflow-scripts > 0.3.3.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- test_console_scripts.py: import every [project.scripts] entry point and run --help (21 scripts) -- catches import breakage and entry-point regressions. - new error-path tests across test_filekey, test_paths, test_patterns, test_cal_grouping, test_pars_loading, test_create_pars, test_complete_run (traceback detection), test_merge_in_channel (identifier/mixed-channel/missing-field) and test_merge_channels. - helper unit tests moved to legend-dataflow-scripts alongside the helpers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Systematic review of Important Depends on legend-exp/legend-dataflow-scripts#39. The scripts now import shared helpers ( Crash bugs
Wrong results
Crashed jobs are now visible in run summaries
methods/ error handling
Tests (62 → 100)New console-script smoke test (imports all 21 Verification
Per 🤖 Generated with Claude Code |
There was a problem hiding this comment.
Pull request overview
This PR strengthens the workflow “plumbing” layer by adding broad unit-test coverage, improving documentation/docstrings, fixing multiple latent runtime bugs in scripts and helpers, and adding a CI documentation build gate to keep Sphinx warnings from regressing.
Changes:
- Fixes and hardens tier/flow/par scripts (config handling, filelist expansion, error messages/validation, safer Snakemake “glue” imports).
- Expands unit tests substantially across
methods/andscripts/flow/, plus smoke tests for console-script entry points. - Adds/updates contributor and user documentation, plus a new CI job to build docs with warnings treated as errors.
Reviewed changes
Copilot reviewed 55 out of 55 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| workflow/src/legenddataflow/scripts/tier/tcm.py | Support per-FCID config mappings and improve error reporting. |
| workflow/src/legenddataflow/scripts/tier/skm.py | Make keep_fields expansion more robust with clearer errors. |
| workflow/src/legenddataflow/scripts/tier/raw_orca.py | Switch to shared logging/config helpers and simplify logging setup. |
| workflow/src/legenddataflow/scripts/tier/raw_fcio.py | Add module docstring. |
| workflow/src/legenddataflow/scripts/tier/raw_blind.py | Fix imports and add validation/guardrails for channel/blinding inputs. |
| workflow/src/legenddataflow/scripts/tier/evt.py | Refactor channel resolution, improve argument defaults, validate TCM input, adjust jitter delay scan. |
| workflow/src/legenddataflow/scripts/par/spms/dsp/trigger_threshold.py | Improve warning/error messages for missing/insufficient waveform data. |
| workflow/src/legenddataflow/scripts/par/geds/tcm/pulser.py | Use shared filelist expansion and tighten required CLI args. |
| workflow/src/legenddataflow/scripts/par/geds/raw/blindcheck.py | Use shared filelist expansion and improve failure message detail. |
| workflow/src/legenddataflow/scripts/par/geds/raw/blindcal.py | Use shared filelist expansion. |
| workflow/src/legenddataflow/scripts/par/geds/psp/dplms.py | Add explicit error when no FFT inputs are found. |
| workflow/src/legenddataflow/scripts/par/geds/psp/average.py | Centralize config lookup, harden parsing/averaging, and improve plot/object merge logic. |
| workflow/src/legenddataflow/scripts/par/geds/pht/util.py | Add module docstring. |
| workflow/src/legenddataflow/scripts/par/geds/pht/qc.py | Centralize config lookup and channel config selection; add module docstring. |
| workflow/src/legenddataflow/scripts/par/geds/pht/qc_phy.py | Centralize config lookup, fix file reading scope, add input-empty guard, and correct discharge timestamp handling. |
| workflow/src/legenddataflow/scripts/par/geds/pht/lq.py | Refactor to shared config helpers, validate required keys, and harden pulser-mask handling; add module docstring. |
| workflow/src/legenddataflow/scripts/par/geds/pht/fast.py | Refactor config loading/validation, harden pulser-mask handling, and guard optional outputs; add module docstring. |
| workflow/src/legenddataflow/scripts/par/geds/pht/ecal_part.py | Refactor config loading/validation, improve error message clarity, and fix “common” plot merge behavior. |
| workflow/src/legenddataflow/scripts/par/geds/pht/aoe.py | Refactor config loading/validation and improve pulser-mask handling; add module docstring. |
| workflow/src/legenddataflow/scripts/flow/write_filelist.py | Make Snakemake glue import-safe for unit tests and expose a testable helper function. |
| workflow/src/legenddataflow/scripts/flow/utils.py | Add module docstring for shared flow helpers. |
| workflow/src/legenddataflow/scripts/flow/merge_in_channel.py | Add validation, improve LH5 merge semantics, and provide clearer errors. |
| workflow/src/legenddataflow/scripts/flow/merge_channels.py | Add validation and clearer errors; harden DB rewrite behavior. |
| workflow/src/legenddataflow/scripts/flow/complete_run.py | Make Snakemake glue import-safe for unit tests; refactor log scanning and FileDB config/build steps. |
| workflow/src/legenddataflow/scripts/flow/build_filelist.py | Fix ignore/runlist parsing bugs, improve run expansion, and harden ignore-keys handling. |
| workflow/src/legenddataflow/scripts/flow/build_chanlist.py | Add module docstring. |
| workflow/src/legenddataflow/methods/patterns.py | Raise ValueError with clearer messages for invalid tier inputs. |
| workflow/src/legenddataflow/methods/paths.py | Centralize path lookup with clearer paths.<key> error reporting. |
| workflow/src/legenddataflow/methods/pars_loading.py | Add explicit validity-file existence checks with actionable errors. |
| workflow/src/legenddataflow/methods/FileKey.py | Add docstrings and improve error handling for key parsing/matching; refactor dict-kwarg resolution. |
| workflow/src/legenddataflow/methods/create_pars_keylist.py | Add validation for required all entries in override catalogs. |
| workflow/src/legenddataflow/methods/cal_grouping.py | Add logging, validation, clearer errors, and docstrings for grouping/expansion logic. |
| tests/test_write_filelist.py | Add unit tests for helper and Snakemake glue execution. |
| tests/test_patterns.py | Add unit tests for tier/pars patterns and out-of-cycle behavior. |
| tests/test_paths.py | Add unit tests for path resolvers and symlink behavior. |
| tests/test_pars_loading.py | Expand unit coverage and add missing-validity error coverage. |
| tests/test_merge_in_channel.py | Add tests for YAML/pickle/LH5 merging and failure modes. |
| tests/test_merge_channels.py | Add tests for YAML/pickle/shelve/LH5 merging and DB path rewriting. |
| tests/test_flow_utils.py | Add unit tests for replace_path behavior. |
| tests/test_filekey.py | Add unit tests for parse/match errors and dict-kwarg expansion behavior. |
| tests/test_create_pars.py | Add coverage for missing all in run-override validity handling. |
| tests/test_console_scripts.py | Add smoke test to import and --help all configured console scripts. |
| tests/test_complete_run.py | Add tests for log scanning (including tracebacks), concat run parsing, and Snakemake glue. |
| tests/test_cal_grouping.py | Add tests for new cal-grouping validation and warning behavior. |
| tests/test_build_filelist.py | Add tests covering runlists/keylists, ignore keys, concat behavior, and wildcard selection. |
| tests/test_build_chanlist.py | Add tests for channel selection and output filename generation. |
| README.md | Link to new contributing guide. |
| pyproject.toml | Fix console script registration (remove broken entry point, add missing one). |
| docs/source/user_manual.rst | Document multiprocessing settings and correct $_ placeholder semantics. |
| docs/source/pipeline.rst | Fix formatting/indentation issues and improve readability. |
| dataflow-config.yaml | Fix config key typo and add clarifying inline comments (values unchanged). |
| CONTRIBUTING.md | Add contributor setup/testing/docs guidance. |
| AGENTS.md | Add/refresh developer/agent reference for repo structure and conventions. |
| .github/workflows/main.yml | Add docs build job to gate Sphinx warnings-as-errors in CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
this looks good but shouldn't we also prepare the full integration tests we discussed (test production cycle with minimal L200 data to test all features) before deploying all this? |
|
This is mainly testing and error updates so should be fine to merge, but I agree we should try to get this set up |
…f local _expand_range
…vation after pylegendmeta switch
…tegory= Input validation (helpers from legenddataflowscripts.utils): - check_input_files for late-consumed inputs: evt hit/dsp/tcm/par/xtc/ann files (previously failed inside build_evt), raw_blind --blind-curve, raw_orca --inl-table (was string-embedded and never checked), pht --pulser-files (consumed after load_data), dplms peak/database/inplots, spms raw-file/dsp-db, merge script inputs. - filelist members are now existence-checked before the expensive loads: split_files_by_run (aoe/ecal_part/lq/psp-dplms), fast.py inline loop, qc cal/fft lists (empty fft stays legitimate), qc_phy per-run lists (empty-run continue preserved). - prepare_output_paths right after build_log everywhere; fixes the no-mkdir gaps (raw_blind/skm/average/dplms/blindcal/blindcheck outputs) and save_dict_to_files now creates parent dirs for every pht caller. - --alias-table (raw_blind/raw_fcio/raw_orca) parsed via parse_json_arg right after build_log instead of a bare json.loads after the entire tier build; parsed mapping passed through. - trigger-thr-multi: named error for mismatched --sipm-names / --raw-table-names lengths (zip strict=False silently truncated). - fast.py: drop --tcm-filelist (declared but unused; no rule passes it). dbetto migration: - all remaining TextDB.on(system=)/valid_for(system=) call sites moved to category= (deprecated in dbetto 1.4), mostly via get_rule_config adoption in tcm/skm/evt/raw_blind/raw_fcio/raw_orca/pulser/blindcal/ blindcheck/trigger_threshold — which also gives named config-dir and rule-lookup errors; direct kwarg edits in ecal_part (metadata read) and flow/build_chanlist. - pyproject: dbetto==1.3.7 -> dbetto>=1.4. - LegendMetadata.channelmap() still forwards system= internally (pylegendmeta 1.4.4) -- not fixable at this layer. Tests: split_files_by_run member checking, save_dict_to_files parent creation; test_build_chanlist fake updated to the category= signature. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
New commit Input validation
dbetto
|
Two conflicts, both keep-both: - pyproject: main's pygama==2.7.0a1 and legend-dataflow-scripts==0.4.0a1 bumps alongside this branch's dbetto>=1.4 (needed for the TextDB.on category= migration). 0.4.0a1 ships every helper this branch imports (check_input_files, prepare_output_paths, parse_json_arg, get_rule_config, require_config_keys, expand_filelist, check_pulser_mask, get_channel_config), so the pin bump this branch was waiting on is now satisfied upstream. - dataflow-config: main's auto_update_metadata key kept, documented in the comment style this branch adds to the rest of the file. Verified: full suite (110 tests) passes against the pinned legend-dataflow-scripts 0.4.0a1 release rather than a local checkout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Merged Two conflicts, both keep-both:
CI: unit tests (3.11/3.12 × ubuntu/macOS), all four Snakemake DAG jobs, docs, readthedocs, coverage and pre-commit.ci all pass. The DAG jobs are the meaningful check for this branch, since the sweep moved every tier/par script onto Also stacked below: #168 was failing CI on @gipert on the integration tests — agreed they're the real gap. I've scoped a plan (skimmed per-channel calibration fixtures hosted in 🤖 Generated with Claude Code |
* perf(evt): let the evt/pet chunk size be configured, and raise it build_evt reads the TCM in chunks and pays a fixed per-chunk cost for every operation and every channel. That cost does not shrink with the chunk size, so the chunk *count* is what drives the runtime. A cal file holds ~1.08M events against ~5k for a phy file, so at the pygama default of 10^4 a cal file was split into 108 chunks and paid the overhead 108 times, while phy paid it once. Nothing in the dataflow ever set the value. Add --buffer-len to build-tier-evt, resolved as CLI > the tier_evt rule config > the pygama default, and pass it to both build_evt calls -- the main one and the muon pass, which was also running at the default. The rules take it from a new evt_buffer_len key in dataflow-config.yaml. Set it to 10^6, which makes a cal file a single chunk. Together with the caching work in pygama this takes a production p16 cal pet build from 44m00s to 1m34s, with peak RSS essentially unchanged (1.43 -> 1.63 GB): because build_evt is called without an output file it already held every chunk in memory, so fewer, larger chunks is not a memory regression. Raising this is only safe now that first_at/last_at no longer depend on where the chunk boundaries fall (pygama: assign first_at/last_at results positionally). Verified end to end against the production pet files for both cal and phy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * perf(evt): match muon timestamps by offset instead of an exact-equality sweep `_find_matching_values_with_delay` swept 10000 delay values and tested exact float equality with `np.isin` on each pass. On a calibration cycle that is 10000 full scans of a ~1.07M-element array: measured at 55.3 s per cycle, or ~45 min across the 49 cycles of a run. The sweep only worked by coincidence. At unix timestamps of ~1.76e9 s the float64 spacing *is* the configured jitter (2.384185791015625e-07), so every timestamp difference is an integer multiple of it and the swept delays landed exactly on that grid -- the 10000 values collapse to just 239 distinct representable numbers, so 9761 of the passes were duplicates. Compare the offset to the preceding muon trigger directly instead: one `searchsorted` into the sorted muon timestamps, then a window test. Same result, no dependence on the timestamp/jitter coincidence, 28.9 ms instead of 55.3 s (~1900x). Verified identical to the previous implementation on a real cal cycle (1,074,676 ged x 8 muon timestamps, 2 matches both ways) and across 200 randomised trials. The one-sided convention (a germanium trigger is flagged only at or after a muon) and the effective window are preserved exactly, including the surprise that the window is `int(1e9*jitter)*jitter`, i.e. ~`1e9 * jitter**2`, rather than the jitter itself -- documented in the docstring and pinned by a test, but deliberately not changed here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(evt): validate the configured buffer_len instead of trusting it `--buffer-len` and the `buffer_len` rule-config option were coerced with a bare `int()` and passed straight to `build_evt`, so 0, a negative number or a non-numeric string reached the read loop and failed there, far from the setting that caused it. Resolve and validate in one place, and report which of the two sources the bad value came from. An explicit `buffer_len: null` now means "unset" and falls back to the default, matching how `null` is already used for `muon_config` in these rule configs. Addresses a review comment on this PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * style: pre-commit fixes --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Summary
Documentation, testing, and bug-fix pass over the workflow-plumbing layer. Unit suite grows from 13 to 62 tests, and writing them surfaced six latent bugs, all fixed here.
Bugs fixed
par-geds-pht-lqconsole script was never registered in[project.scripts], althoughpht_pars_geds.smkinvokes it — the rule failed with command not found.create-chankeylistentry point pointed at a deleted module, producing a broken console script on install; removed (nothing references it).mutliprocess_modeconfig key was misspelled —common.smkreads the correct spelling, so the setting was silently ignored. Renamed in the config (value equals the default, so behavior is unchanged).get_analysis_runscheckedPath(ignore_keys_file).suffixwhere it meantanalysis_runs_file, crashing onPath(None)whenever a runlist was passed without an ignore file.build_filelistassumed dict-shapedignore_keysand crashed on the flat lists produced from.keylistfiles.find_gen_runssplit the full path on-for concat tiers, mis-parsing run specs whenever the production path contains a hyphen (e.g.prod-blind); it now splits the filename only.Tests (13 → 62)
methods/paths.pyandpatterns.py: resolvers, error cases,link_external_pathssymlinking, out-of-cycle/tmpfallbackscripts/flow/:build_filelist(run expansion, runlist/keylist selection, multi-selectors, concat handling, end-to-end on touched files),merge-channelsandmerge-in-channels(yaml/pickle/shelve/lh5 with real lgdo objects, db path rewriting),build_chanlist,write_filelist,complete_runcomplete_run.py/write_filelist.pywere restructured so the Snakemake-injected object import is guarded and the logic is importable; the glue itself is tested by injecting a fake object intosnakemake.scriptand executing viarunpyDocumentation
AGENTS.mdcondensed (268 → 97 lines) to non-discoverable content: two-package split with legend-dataflow-scripts, rule→script invocation styles,PRODENV/$_expansion, commands, CI data-scale constraintsCONTRIBUTING.md, linked from READMEdataflow-config.yaml(values unchanged)scripts/modules and method docstrings forFileKey/CalGrouping, so the apidoc-generated API reference is no longer empty stubsmultiprocess_mode/max_processes, correct the$_placeholder description (expands to the config directory, not$PRODENV)CI
build-docsjob gating the strictsphinx-build -Wbuild (previously RTD-only); fixed the five pre-existing RST warnings that were already failing itTest plan
python -m pytest— 62 tests passpre-commit run --all-filesclean on all touched filesmake -C docsbuilds with warnings-as-errors🤖 Generated with Claude Code