Conversation
Cirro submits kmer_min_depth, local_min_OVE, and local_min_pvalue as native numeric types when overridden through the bulk analysis form, but nextflow_schema.json declared them strictly as type "string". validateParameters() rejected the run before any process executed (reproduced with `nextflow run . --kmer_min_depth 3 --local_min_OVE 10 --local_min_pvalue 0.001`). Widen the accepted types and fix two defaults that had drifted to unquoted JSON numbers. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…evel The pipeline now warns "workflow_level is set for deprecation, please use run_<level> flags" on every Cirro run. Both bulk_analysis and convert_adaptive preprocess.py were still building the old comma-joined workflow_level string from convert_lvl/sample_lvl/patient_lvl/compare_lvl. Point process-input.json directly at run_sample/run_patient/run_compare/ run_convert (the params workflows/tcrtoolkit_bulk.nf actually reads) and drop the now-unnecessary workflow_level construction from both scripts. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Nextflow's CLI param parser auto-casts numeric-looking overrides to Integer/Double but leaves "true"/"false" as plain Strings. Groovy truthiness treats any non-empty String as true, so `if (params.run_sample)` etc. always ran that stage regardless of an explicit false override - verified locally that --run_sample false --run_compare false still launched SAMPLE (VDJDB_GET and friends). This silently broke the convert-only pathway that .cirro/convert_adaptive depends on (run_sample: false, run_compare: false), since Cirro renders these as CLI flags the same way it does numeric params. Route each flag through toString().toBoolean() so both real Booleans and Cirro's string overrides parse correctly. Verified with a real (non-stub, Docker) convert-only run: only SAMPLESHEET_CHECK and CONVERT_ADAPTIVE execute now. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…ecutors
RENDER_NOTEBOOK built its `quarto render -P sample_table:...` arg from
`${file(params.samplesheet)}` as raw script-string interpolation rather
than a declared process input, so Nextflow never staged the samplesheet
into the task's work dir - it just embedded the head node's local
absolute path. That happened to still resolve when running fully local
(head and worker share a filesystem), but on Cirro/AWS Batch the worker
container has no such path: dataset 261a4d9a failed with
`FileNotFoundError: /opt/work/.../samplesheet.csv` once RENDER_NOTEBOOK
(template_qc) ran, after every other stage (sample/patient incl.
GIANA/GLIPH2) had completed successfully on fix/bulk-cirro-config.
Declare `path samplesheet` as a real process input (stageAs'd to a fixed
name so it can't collide with a same-named file in the caller's
report_files list, as the "nested project_dir layout" nf-test caught)
and reference the staged file directly. Verified via nf-test - the
rendered .command.sh now references the staged bare filename, not an
absolute host path.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Dataset c2fa7c64 failed at RENDER_NOTEBOOK (template_qc and
template_details_patient) with:
ValueError: Value of 'hover_data_0' is not the name of a column in
'data_frame'. Expected one of [...] but received: alias
alias_col (default "alias") is documented as an optional samplesheet
column, and several call sites already guard with `if 'alias' in
...columns`, but each notebook's own `meta = pd.read_csv(sample_table)`
never actually backfills it - other spots (hover_data=['alias'],
column-selection merges) reference it unconditionally and blow up the
instant a real samplesheet omits it, as this one did.
Add the same defaulting each notebook already uses for
timepoint_order_col ("if col not in meta.columns: inject it") for
alias_col too, right after each notebook's own meta load: fall back to
the sample name. Applied everywhere a notebook independently loads
sample_table and references alias_col (template_qc, template_giana,
template_gliph, template_discovery_brief, template_overlap,
template_pheno_sc/bulk, template_sample) - not just the two that
happened to fail in this run, since it's the same structural gap in
every one of them.
Verified: the fallback logic re-run against the real container's
pandas, and every edited notebook's Python code blocks still compile.
Have not re-run the full multi-hour pipeline end-to-end locally to
confirm no further downstream issue - recommend retrying on Cirro next.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
SAMPLESHEET_CHECK's samplesheet_stats.csv (e.g. dataset 2059e0b4) was unreadable: bin/samplesheet.py builds it from ss.describe() (whose row index holds the stat names - count/unique/top/freq for object columns, or count/mean/std/min/25%/50%/75%/max for numeric ones) but wrote it with index=False, dropping those labels entirely. The CSV ends up with the right values in the right columns but no way to tell which row is which stat. Write it with index=True instead. Verified against the real container: the fixed output now has the count/unique/top/freq labels in the first column, matching what .describe() actually produced. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Dataset a0231533 got through every stage (sample, patient, GIANA, GLIPH2, template_qc render) and OOM-killed on RENDER_NOTEBOOK (template_details_patient): Essential container in task exited - OutOfMemoryError: Container killed due to memory usage RENDER_NOTEBOOK is labeled process_single, which only budgets 2.GB. That's enough for template_qc (which just succeeded in the same run) but not template_details_patient, which embeds GIANA sunburst and GLIPH2 network plots into one HTML file via quarto's embed-resources: true. The pipeline's usual attempt-scaled-memory retry (errorStrategy checks task.exitStatus in (130..145)+[104], which covers a SIGKILL/137 OOM) never engaged either - the task list shows only one attempt with exitCode 1, not 137, so Batch didn't surface the OOM as a retryable code here. Bump it to process_medium (16.GB) via a withName override in conf/modules.config, matching the existing TCRDIST3_MATRIX pattern in the same file. Verified with `nextflow config -flat` that the label override resolves as intended. Co-Authored-By: Claude Sonnet 5 <[email protected]>
The full rationale for each of these already lives in its own commit message (samplesheet.py stats fix, run_<level> boolean coercion, RENDER_NOTEBOOK samplesheet staging/collision, alias_col fallback, RENDER_NOTEBOOK memory bump) - no need to duplicate it in the code too. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Dataset a367a549 (fix/bulk-cirro-config with RENDER_NOTEBOOK already at process_medium/16GB) OOM-killed on template_details_sample, template_details_patient, template_discovery_brief, and template_details_compare - template_details_sample's kernel died mid- render (cell 20/23). These reports embed far more per-sample data than template_qc (which passed at 16GB): TCRdist3 HDF5 distance matrices, OLGA pgen tables, VDJDB match tables, convergence tables, all via embed-resources: true. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Dataset fa684158 confirmed it synced to the latest commit (9d98276, already includes the process_high/64GB bump) and still OOM-killed on template_details_patient - but this time all 17 notebook cells completed successfully first; the OOM happened during pandoc's embed-resources HTML-embedding step, not Python execution. Points at the interactive Plotly figures (GIANA/GLIPH2 network + sunburst plots) being expensive for pandoc to self-contain, not the underlying computation. Add process_high_memory alongside process_high, matching the existing TCRDIST3_MATRIX pattern in this file, to get 256GB while keeping process_high's cpu/time settings. Co-Authored-By: Claude Sonnet 5 <[email protected]>
template_overlap.qmd and template_discovery_brief.qmd (both hit by the same copy-pasted code) ran fisher_exact via comp_df.apply(axis=1) once per surviving clonotype per (subject, origin, timepoint-pair) group - likely the real cause of template_details_compare's 84min runtime and template_discovery_brief's 26min runtime before both OOM'd at 256GB, not an actual memory-sizing problem. total_pre/total_post are fixed margins within each group (assigned as a single scalar to the whole comp_df), so the one-sided Fisher's exact p-value reduces to a hypergeometric survival/CDF call, vectorizable across every row in the group at once instead of one Python call per clonotype. Verified the exact function as written in both files against scipy.stats.fisher_exact across random cases, zero counts, ties, extreme margin imbalance, and a 50k-row stress test - all identical to floating-point epsilon. ~60x faster in a synthetic 20k-row benchmark. Co-Authored-By: Claude Sonnet 5 <[email protected]>
The last few commits bumped RENDER_NOTEBOOK's memory via
`withName: RENDER_NOTEBOOK { label = [...] }` in conf/modules.config,
copying TCRDIST3_MATRIX's pattern - but base.config (with all the
withLabel:process_* blocks) is includeConfig'd before modules.config.
Nextflow resolves config in one top-to-bottom pass: RENDER_NOTEBOOK's
inline `label 'process_single'` already matched
`withLabel:process_single { memory = 2.GB }` by the time base.config
loaded, and reassigning the label later in modules.config never gave
the (earlier-declared) process_high/process_high_memory withLabel
blocks a second chance to fire. Confirmed with an isolated repro using
the actual withLabel blocks: task.memory stayed 2 GB despite the
override. This is why the workflow report kept showing every
RENDER_NOTEBOOK task at 1 cpu / 2 GB regardless of the last two
"bump to process_high(_memory)" commits.
modules/local/compare/gliph2.nf already has the correct pattern for a
process that needs multiple resource-tier labels: declare them all
inline in the process definition, not via a later config override,
since inline labels are known before any config file parses and every
matching withLabel block applies. Move RENDER_NOTEBOOK's labels there
instead. Verified via a faithful isolated repro (byte-identical
withLabel blocks): inline multi-label resolves to 256GB; the
config-override version does not.
Note: TCRDIST3_MATRIX's dense-matrix branch
(`matrix_sparsity != 'sparse'`) uses the same config-override pattern
this commit moves away from, and is likely subject to the identical
bug (silently capped at process_high's 64GB instead of
process_high_memory's 256GB) - pre-existing, not introduced here, not
fixed in this commit.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Dataset af639006 got through every other report (qc, details_patient,
details_sample, details_compare - the last two confirming the
vectorized Fisher's exact fix works) and failed on
template_discovery_brief:
Warning: File s3://.../data/convert/su005_BCC_post_TCRB_airr.tsv
not found. Skipping. (x4)
ValueError: No objects to concatenate
The VDJdb section falls back to `row['file']` when no adaptive
conversion ran (input_format == 'airr'), but that's the raw samplesheet
value - an S3 URI in this pipeline, never a locally readable path.
workflows/tcrtoolkit_bulk.nf deliberately left convert_files empty for
the non-adaptive branch, so nothing was ever staged locally for this
notebook to read regardless of what's in that samplesheet column.
Populate convert_files from INPUT_CHECK's sample_map (not just
CONVERT's output) when input_format != 'adaptive', carrying [meta,
file] pairs through so bulktcr_analysis.nf can stage each one under
${meta.sample}_airr.tsv - the name the notebook looks for - regardless
of the source file's actual basename. This isn't specific to files
that happen to already end in "_airr.tsv" (true of this test dataset
only because it was itself pre-converted); it handles arbitrarily-named
native AIRR input the same way.
Verified against the actual notebook code (copied verbatim into a
harness): reproduced the original ValueError with an S3-URI samplesheet
and empty convert_dir, then confirmed it succeeds once convert_dir is
populated per this fix - using deliberately unrelated source filenames
to prove the fix doesn't depend on any naming coincidence.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Dataset 70266df6 got past the convert_dir fix (no more "not found" warnings) and crashed later in template_discovery_brief: AttributeError: 'Index' object has no attribute 'levels' (upsetplot/reformat.py:71, _check_index) plot_public_clones_upset groups by subject_col (patient) to build the UpSet plot's intersection data. This test dataset has exactly one patient (su005), so that groupby produces a single-key dict; upsetplot.from_contents() returns a plain Index instead of a MultiIndex for a single category, and UpSet()/.plot() then raises AttributeError trying to access .levels on it. The existing guard at this call site (`except TypeError as e: ... "This typically happens if only one intersection group remains"`) already anticipated this failure mode by description, but wasn't catching the exception type this upsetplot version actually raises for it, so nothing caught it. Multi-patient datasets never hit this - groupby(subject_col) always yields >=2 groups - which is presumably why this never surfaced before. Add an explicit check for fewer than 2 patients and skip with a message, same as the existing `if upset_data.empty` guard just above it, instead of relying on catching whatever exception the library happens to raise. Verified against the actual function (extracted from the file): single-patient input now skips cleanly instead of crashing; multi-patient input still renders as before. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…f notebooks template_details_compare (via template_overlap.qmd) and template_discovery_brief were the two most expensive report steps by cost/duration even after the earlier Fisher's-exact vectorization, because both still ran several row-wise .apply(axis=1)/.apply() calls, each of which builds a pandas Series per row. - template_overlap.qmd: row-wise z-score (`matrix_df.apply(zscore, axis=1, ...)`) replaced with `sub`/`div` against `mean`/`std(ddof=0)`; verified bit-for-bit equivalent (max abs diff: 0.0) against scipy.stats.zscore per-row. - template_overlap.qmd: `get_status` .apply(axis=1) replaced with a list comprehension over zip(); verified identical output against the original. - template_discovery_brief.qmd: `assign_status` .apply(axis=1) replaced the same way; verified identical output. - template_discovery_brief.qmd: three node_properties .apply()/.apply(axis=1) calls (color/category assignment, size, hover title) in the per-patient network-graph section replaced with list comprehensions / vectorized numpy; verified identical output (color, category, size, and title all match). All python code blocks in both files compile cleanly after the edits. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…/256GB All five report notebooks shared the same process_high + process_high_memory labels (16 cpu, 256GB, 16h), even though the last completed real run (ac1f14c8) showed durations ranging from 30s (template_qc) to 117 minutes (template_discovery_brief) - the three fast notebooks (template_qc, template_details_patient, template_details_sample) finish in under 5 minutes and don't touch the large VDJdb match files or build network graphs. Replaced the static labels with dynamic cpus/memory/time directives (same pattern already used in modules/local/sample/tcrdist3.nf) keyed on notebook.baseName, so only template_details_compare and template_discovery_brief keep the 16cpu/256GB tier; everything else drops to 4cpu/16GB/4h. Verified locally with -stub-run: template_qc resolves to the light tier and completes on a 10-cpu machine; template_discovery_brief resolves to the heavy tier's 16-cpu request (confirmed by it correctly exceeding the 10-cpu local executor's capacity). All 3 render_notebook.nf.test cases still pass. The two heavy notebooks are left at 256GB pending real peak-memory numbers from the Nextflow execution report, since the vectorization fixes in this branch may allow lowering that tier further. Co-Authored-By: Claude Sonnet 5 <[email protected]>
…nk heavy tier Per-notebook resource directives previously lived inline in render_notebook.nf. Moved them into conf/modules.config's existing withName: RENDER_NOTEBOOK block instead, keeping the process definition focused on rendering logic. This is safe unlike the earlier RENDER_NOTEBOOK OOM bug (config-level `label = [...]` reassignments never triggering the matching withLabel: block due to include ordering) because withName: here sets cpus/memory/time directly rather than routing through a label - verified locally against the real nextflow.config chain (-stub-run): the closures correctly read the notebook input variable and resolve template_qc to 4cpu/16GB and the two heavy notebooks to 16cpu/64GB. Also lowered the heavy-notebook memory tier from process_high_memory (256GB) to plain process_high (64GB): the Nextflow execution report for the last completed real run (dataset ac1f14c8) shows template_details_compare peaked at 5.1% of 256GB (~13GB) and template_discovery_brief at 0.2% (~0.5GB). The 256GB figure was never actually validated against real usage - it came from iteratively doubling memory while chasing the label-ordering bug above, which was the real blocker the whole time. 64GB keeps ~5x headroom over the observed peak for the larger of the two. All 3 render_notebook.nf.test cases still pass. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Longer rationale belongs in commit messages, not inline. No functional changes - comment text only. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Dataset 52249ba1 failed: template_details_sample OOM'd at cell 20/23 under the 16GB "light" tier assigned based on its short (4m20s) duration in the prior run. Duration was the wrong proxy - template_sample.qmd (included by template_details_sample.qmd) loads each sample's TCRdist distance matrix from HDF5 and converts it to a dense array via sparse_matrix.toarray(), then symmetrizes it with np.maximum(dense, dense.T), holding two full NxN float64 arrays at once. For su005's ~25-30k clones per sample that's a multi-GB memory spike that doesn't show up in wall-clock time. template_qc and template_details_patient both completed fine at 16GB in this same run, so they stay on the light tier. template_details_sample moves to the same 64GB tier already verified for template_details_compare/ template_discovery_brief, rather than guessing a new number. Verified locally against the real config chain (-stub-run): template_qc still resolves to 16GB, template_details_sample now resolves to 64GB. All 3 render_notebook.nf.test cases still pass. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Unit Test Results17 tests ±0 14 ✅ - 3 6m 46s ⏱️ -25s For more details on these failures, see this check. Results for commit a7e137b. ± Comparison against base commit 4664ae1. |
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.
Summary
run_<level>boolean flags always evaluating truthy, deprecatedworkflow_levelhandling,RENDER_NOTEBOOKreading an unstaged samplesheet path on remote executors,samplesheet_stats.csvlosing its row labels, missingalias_colfallback across several notebooks, a single-patient UpSet-plot crash, andtemplate_discovery_brief's VDJdb section not handling airr-native (non-adaptive) input.RENDER_NOTEBOOK's resource label never actually taking effect, caused by Nextflow's configincludeConfigload order (base.config'swithLabel:blocks resolve beforemodules.configcan reassign labels) - resources are now set directly viawithName: RENDER_NOTEBOOKinconf/modules.configinstead of label reassignment.RENDER_NOTEBOOK's per-notebook cpu/memory instead of a flat 16cpu/256GB for every report: light notebooks (template_qc,template_details_patient) get 4cpu/16GB;template_details_sample,template_details_compare, andtemplate_discovery_briefget 16cpu/64GB, based on real OOM/peak-memory evidence from production runs rather than guesswork..apply(axis=1)hotspots (Fisher's exact test, z-score, status-lookup, per-patient network-graph node properties) intemplate_overlap.qmd/template_discovery_brief.qmd, measurably cutting report render time (template_details_compare: 80m→56m,template_discovery_brief: 117m→93m on the same test dataset).Verified end-to-end on a real Cirro dataset (
BTC-DST-Developmentproject) - the full bulk pipeline now completes successfully, including all five report notebooks.Test plan
nf-testsuite passes forRENDER_NOTEBOOK(samplesheet staging, nested project_dir layout, renamed dest basename)acad0633-eebd-4657-b514-1710ec17f612)🤖 Generated with Claude Code