Debug - #12
Merged
Merged
Conversation
A 26-dataset run failed with ~190 errors across 13 rules. Diagnosis: 179 were OUT_OF_MEMORY (mislabeled "FAILED" by the SLURM executor), hitting the largest datasets across nearly every rule; the remaining 3 were genuine data-driven errors. Data guards: - cellbender2seurat.R: CreateSeuratObject(min.features=1) drops the rare denoised-to-zero barcode in CellBender *_filtered.h5 that made log_umi=-Inf abort SCTransform. - find_markers.R: skip clusters with <3 cells (FindMarkers minimum), writing a schema-correct empty file instead of crashing. - combine_markers.R: drop empty marker tables before bind_rows to avoid a logical/character type clash; handle the all-empty case. - soupx.R: fall back to autoEstCont(forceAccept=TRUE) when contamination estimation aborts (e.g. estimated fraction >0.8). Memory right-sizing (from observed peak RSS; MaxRSS undercounts killed jobs): - doubletfinder/_cellbender: scale by input size, floor 48GB (paramSweep peaks ~43x the input rds; 92GB observed for the largest sample). - downsample_clusters: scale by input size, floor 32GB (all replicates run in one job; ~41GB observed). - emptydrops 24->64GB; tenx2seuratrds/soupx/soupx_emptydrops/scdblfinder/ posthocfilter_mad/posthocfilter_threshold 24->48GB; find_markers 12->24GB. Right-sizing attempt-1 baselines also sidesteps the unreliable retry path (observed: ~half of OOM restart intents were never resubmitted). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Second debugging round on 26-dataset runs: the prior fixes cut failures from 193 to 82 and reached 99.7%, but downsample_clusters and a few heavy-dataset jobs still failed. Root causes were the serial single-job downsampling design, under-sized memory for the largest (emptyDrops) cell counts, a stray small future limit, and an unguarded PCA on over-filtered samples. downsample_clusters (all 100 replicates run serially in one job): - Resize to a flat 64 GB / 15 h: peak is a single replicate's SCTransform footprint (~42 GB) and wall-time is ~100x per-replicate (worst ~10.2 h); input-size scaling mis-modeled this (small cteleta was among the heaviest). - Fix future.globals.maxSize 2 GB -> 16 GB (every other script already used 16 GB); the 2 GB cap hard-failed SCTransform on large subsamples (5 jobs). - Strip the input to a counts-only object before the loop: the upstream SCT assay / PCA / UMAP / graphs were pinned across all replicates and re-copied into every subset() but never used (~36 -> ~30 GB peak; grows with cells). Memory right-sizing for the largest emptyDrops cell counts: - doubletfinder / _cellbender: input scaling 48x -> 64x (amellifera peaked ~105 GB from DoubletFinder's O(N^2) distance matrix). - cellbender2seurat 24 -> 48 GB; posthocfilter (mad+threshold), soupx, soupx_emptydrops 48 -> 64 GB. Robustness guard: - require_min_cells_for_pca() in silhouette_utils.R, called before RunPCA in all RunPCA scripts. Over-filtered samples (e.g. cteleta: 21 of 54k cells survive the fixed threshold) now stop with an actionable message instead of a cryptic SVD error. Memory efficiency (results-identical): - posthocfilter (mad+threshold): rebuild a counts-only object after subset() so the upstream SCT/PCA/graph baggage is dropped before recomputation; posthocfilter_mad also frees the SingleCellExperiment QC copy early. - soupx: extract cluster labels + UMAP from seurat_base and drop it before autoEstCont/adjustCounts. Scalability docs: - Document DoubletFinder's O(N^2) memory in doubletfinder.R and README, and recommend scDblFinder for >100k-cell datasets. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When post-hoc filtering leaves too few cells to cluster, the require_min_cells_for_pca guard stops the job. This adds two decoupled mechanisms to handle such samples cleanly. Quarantine (automatic, advisory): - The guard now embeds a stable "[LOW_QUALITY_SAMPLE]" token in its message. - New workflow/scripts/quarantine_low_quality_samples.py scans run logs for the token, identifies the affected samples, and moves ALL of each flagged sample's outputs from results/<subdir>/ into results/low_quality_samples/<subdir>/, preserving structure (job logs stay in results/logs/ for debugging and stable detection). It writes an advisory results/low_quality_samples/flagged_samples.txt and points the user at excluded_samples. It never edits config and never drives the DAG. - Wired into the runner after the snakemake call (runs on completion, preserving the workflow's exit status). Exclusion (explicit, user-controlled): - New excluded_samples list in config/config.yaml (validated in common.smk) drops the listed sample IDs from the DAG, with a visible "[excluded_samples] skipping ..." message at startup and a note for IDs not found in the sample sheet. Also filters downsample targets (for downsample_only mode). - This is the ONLY thing that changes the DAG, so nothing is skipped automatically: a re-sequenced library that reuses a sample ID is processed normally unless the user deliberately lists that ID. Verified by dry-run: excluding 2 samples removes exactly their jobs (2394 -> 2210) with no leaked targets; quarantine moves flagged outputs, writes the advisory list, and leaves other samples untouched; idempotent and a no-op when nothing is flagged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Previously the SLURM submission throttling lived on the runner command line and the low-quality-sample quarantine ran as a step appended after the snakemake call. Both are workflow behavior, so a run launched with a different runner script silently skipped them (as happened with a personal runner that lacked the quarantine step). Move both into the workflow so any launcher gets them via --workflow-profile / the Snakefile. - profiles/slurm/config.yaml: add keep-going, jobs, max-jobs-per-timespan, max-status-checks-per-second, latency-wait, rerun-incomplete alongside the existing retries. Remove stale downsample_cluster_replicate set-threads/set-resources entries (that rule was consolidated into downsample_clusters). - Snakefile: run the quarantine from BOTH onsuccess and onerror handlers, so it fires on completion whether or not every job succeeded (flagged samples fail the guard, so such runs end via onerror). It is intentionally not a rule - it moves other rules' outputs, which would fight Snakemake's dependency tracking. - scrnaseq_preprocess_slurmrunner.sh: slim to the invocation-level flags only. - quarantine_low_quality_samples.py: scan logs with grep and skip the find_markers log subtree (which cannot carry the guard token), cutting an 18k-file scan that timed out to a few seconds. Falls back to a Python scan if grep is unavailable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tion With the SLURM executor, Snakemake can exit 0 even when a job fails terminally (e.g. an OOM that exhausts retries), so the runner batch job shows COMPLETED and a real failure escapes detection. This adds a verification that drives the true exit state, independent of Snakemake's own exit code, and honors the rule that low-quality-sample failures are excused. - verify_run_complete.py (new): checks that every required target (the rule-all inputs, passed via a targets file) exists. A missing output is a real failure only if its sample was NOT flagged low quality; failures confined to flagged samples - which are quarantined and cannot be diagnosed without running - are excused. Exit 1 on real failure, else 0. - Snakefile: onsuccess/onerror now call _finalize_run(), which quarantines, verifies, then sys.exit(verify_code). sys.exit() in a handler deterministically sets Snakemake's process exit code in BOTH directions (unlike shell, which can only force failure), so the runner job's COMPLETED/FAILED status reflects reality even when Snakemake would exit 0 on a terminal failure. Being in the handlers, it applies to any launcher. - quarantine: write flagged_samples.txt before moving outputs, so the verification can excuse flagged samples even if a later move errors. Detection is by missing required outputs, so it is executor-exit-code-agnostic and recovered retries (output present) correctly do not count as failures. Verified end-to-end: a hidden missing target for a non-flagged sample forces the process to exit 1 (runner FAILED) while a complete run exits 0, and failures confined to flagged samples are excused. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The push failed the pytest and downsample-rule CI jobs for two reasons introduced with the run-verification handlers: - snakemake --lint flagged "mixed rules and functions in same snakefile" because _finalize_run was defined in the Snakefile. Move it into rules/common.smk as finalize_run(). - The onsuccess/onerror finalization verified against ALL_TARGETS on every run, so tests that build a single target (e.g. the marker checkpoint and downsample-rule tests) saw the other targets reported missing and the process forced to exit 1. Gate finalization on workflow.non_local_exec so it runs only under a remote (SLURM) executor - where it is needed to correct that executor's unreliable exit code - and is skipped for local runs (CI, tests, ad-hoc builds), which have reliable exit codes and may build only a subset of targets. Verified locally: snakemake --lint passes, and test_snakemake_lint, test_marker_checkpoint_ expansion, test_cellbender_rule, test_r_rule_execution, and the downsampling rule test all pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ow CI) The posthocfilter memory optimization rebuilt the filtered object with CreateSeuratObject keeping only percent.mt, which dropped the upstream metadata columns (orig.ident extras, scDblFinder.class, upstream silhouette/purity, etc.). The full-workflow reference comparison checks metadata column names, so the saved posthocfilter objects no longer matched and the pytest --run-workflow CI job failed. Switch both posthocfilter scripts to DietSeurat(assays="RNA", dimreducs=NULL, graphs=NULL) (with DefaultAssay set to RNA first). This still drops the heavy SCT assay, PCA/UMAP embeddings and neighbor graphs before recomputation - the intended memory savings - but keeps the RNA counts and the full cell metadata intact, so the outputs match the reference exactly. Verified locally: pytest tests --run-workflow passes (37 passed, 12 skipped) reusing the prebuilt conda envs. Co-Authored-By: Claude Opus 4.8 <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
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.
debugging of GitHub actions, including post run quarantine script and catch of SLURM jobs that failed not captured by runner process