Code refactor - #36
Merged
Merged
Conversation
`BaseDriver.interval_padding` now defaults to `None` instead of `0`, so driver commands carry `--interval_padding` only when a caller passes a value. The dnascope pipeline's attribute follows the argparse option, which has no default. The hybrid pipeline's DNAModelApply no longer passes `--interval`, matching every other model-apply invocation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Introduce `sentieon_cli/stages/`, a layer of reusable pipeline stages that build their jobs and insert them into the DAG with the right edges. `stages/base.py` defines `StageContext` (run-wide settings), `StageResult` (the inserted jobs and the terminal jobs downstream stages depend on), the `Stage` base class with its `add_to` contract, and the `driver_job` / `rm_job` helpers for the two most common job shapes. `BasePipeline` gains `stage_context()` and `required()`, and `util` gains `require_versions()` / `versions_available()` for the version check idioms. Nothing uses them yet; the pipelines migrate stage by stage. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`CNVscopeStage` (CNVscope followed by CNVModelApply) replaces the module-level `call_cnvs` used by dnascope-hybrid and the two private builders in dnascope-pangenome; `PloidyStage` replaces `BasePipeline.build_ploidy_job` and derives the `_ploidy.json` path itself. `CNV_MIN_VERSIONS` moves to `stages/cnv.py`. The duplicate version check that ran inside `call_cnvs` is gone; `validate_cnv` already performs it before the first DAG runs. The hybrid pipeline's CNV jobs are now named `cnvscope` and `cnv-model-apply`, matching the pangenome pipeline. Commands and dependency edges are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`DedupStage` (LocusCollector followed by Dedup) replaces the short-read `dedup_and_metrics` builder and the pangenome `build_dedup_job`; `MetricsStage` (one metrics pass plus the optional WGS rehead job) replaces the post-dedup metrics code in both places. `MetricsPaths` derives the metrics directory and every metrics file name once, instead of at five call sites. `DNAscopePipeline.add_sr_preprocessing` composes the two stages and inserts the jobs itself, returning the deduplicated alignment, the Dedup job, and the QC jobs MultiQC waits on; the hybrid pipeline inherits it. Renames: the rehead job is `rehead-metrics`, and with `--duplicate_marking none` the metrics-only pass is now the `metrics` job under the `metrics` task rather than `locuscollector` under `dedup`. Commands and dependency edges are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Add the small-variant stages: `DNAscopeStage` (any DNAscope-family pass), `ModelApplyStage`, `GVCFtyperStage`, `TransferStage` (the sharded annotation transfer, wrapping `build_transfer_jobs`, which gains a `tag` for pipelines that transfer more than once), and the `TransferApplyStage` composite for the common "transfer, then apply the model" tail, whose DNAModelApply waits on the raw-VCF producer as well as the transfer. The dnascope pipeline's `sr_call_variants` (a 6-tuple) becomes `add_small_variant_calling`, and the pangenome pipelines drop their `build_dnascope_job` / `build_dnamodelapply_job` / `build_gvcftyper_job` builders. The DNAscope job is now named `dnascope` everywhere (was `variant-calling` and `dnascope-raw`). Commands and dependency edges are unchanged, except that a dnascope-pangenome run without a population VCF now feeds DNAModelApply the raw VCF rather than the transfer output that no job would have written; validation requires `--pop_vcf`, so real runs are unaffected. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`DNAscopeHybridPipeline.call_variants` returned its 23 jobs as a tuple for `build_dag` to wire by hand. `add_variant_calling` now builds each job and inserts it with its dependencies as it goes, using `DNAscopeStage`, `ModelApplyStage`, `TransferStage` and the `driver_job` / `rm_job` helpers where they fit. Cleanup jobs are simply not created under `--retain_tmpdir`. The two DNAscope passes are now named `dnascope-1` and `dnascope-2` (were `calling-1` and `call2`). Commands and dependency edges are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`DNAscopeLRPipeline.lr_call_variants` returned a 27-field dataclass of jobs that `build_dag` wired by hand. `add_small_variant_calling` now composes the small-variant stages and inserts each job with its dependencies as it goes: `DNAscopeStage` for the five calling passes, `TransferApplyStage` for the diploid and unphased tails, and `TransferStage` plus `ModelApplyStage` for the two haplotypes, whose model-apply jobs wait on both haplotype transfers. The passes are now named after the stage: `dnascope-diploid`, `dnascope-hap1`/`hap2`, `dnascope-unphased`, `dnascope-haploid`, with matching `model-apply-*` jobs, and the four annotation transfers carry `diploid`/`hap1`/`hap2`/`unphased` tags in their job names. Commands and dependency edges are unchanged; a new unit test pins the non-obvious edges. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`stages/alignment.py` holds the bwa and minimap2 alignment stages (from BAM/CRAM input and from fastq), the pangenome bwa-plus-extract stage, and a ReadWriter stage, together with the shared `aln_suffix` and `find_unzip` helpers. Alignment jobs are always DAG roots, so these stages expose `build()` for callers that insert the jobs themselves as well as the usual `add_to()`. The hybrid-pangenome pipeline's copy of the minimap2 realignment and both pangenomes' bwa-extract builders are gone; the dnascope pipeline builds its alignment cleanup jobs from the stage's `cleanup_paths`. The long-read pipeline keeps its `lr_align_inputs`, `lr_align_fastq` and `merge_input_files` methods as thin wrappers. Commands, job names and dependency edges are unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Replace the repeated version-check loops with `require_versions` / `versions_available`, and the repeated "argument is required" exits with `BasePipeline.required` or the values already carried by the stage context. Drop `BasePipeline.cnv_sex_args`, which lost its last caller when CNV calling moved into a stage. The long-read and hybrid tests no longer patch `check_version` in the pipeline modules: they skip version checks, so the patches were inert, and the modules no longer import the function. The pangenome KMC test, which does exercise the check, patches it in `util` instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Drop `cmd_bedtools_merge`, `cmd_vg_giraffe`, `cmd_vg_pack`, `cmd_sv_call`, `cmd_vg_surject` and `strip_ctg_prefix` from `command_strings`, `Shard.bcftools_str` from `shard`, and `get_read_length_aln` (with its regex and `shlex` import) from `util`. None had a caller in the package, the tests, or the docs. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
- `find_unzip` always warns; the per-caller log level is gone. - The bwa alignment stages report `cleanup_paths` only when the alignment is an intermediate that duplicate marking consumes, as the field's documentation already said; final outputs get none, and the pipelines build the `rm-*-aln` job only when there is something to remove. - `TransferApplyStage`'s DNAModelApply depends on the transfer's concat alone (the concat already depends on the raw-VCF producer), or on `upstream` when there is no transfer. The hybrid pipelines now use the composite too, and the long-read haplotype passes each run their own transfer-then-apply, so a haplotype's model-apply waits only on its own transfer. - `build_transfer_jobs` moves into `stages/transfer.py`, its only caller, and the top-level `transfer.py` is gone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`ShortReadPreprocessingStage` carries the duplicate-marking and metrics policy that lived in `DNAscopePipeline.add_sr_preprocessing`; `MosdepthStage` and `LongReadSVStage` replace the raw job construction in the long-read pipeline's `mosdepth` and `call_svs`. The alignment, mosdepth and LongReadSV version constants move next to the stages they guard. The pipeline methods remain as thin wrappers that hold the version-check policy, so the hybrid pipeline, which still inherits them, is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`DNAscopeHybridPipeline` inherited from both `DNAscopePipeline` and `DNAscopeLRPipeline` to reuse six methods. It now subclasses `BasePipeline`, declares every attribute its arguments need, and composes the short-read preprocessing, minimap2 realignment, mosdepth and LongReadSV stages through its own small wrappers. The bwt_max_mem computation becomes `util.set_bwt_max_mem`, shared with the dnascope pipeline, and the long-read `call_svs` loses the `replace_rg` parameter only the hybrid used. Two latent defects go away with the inheritance: `--lr_fastq_taglist` now reaches the minimap2 command (the inherited method read the long-read pipeline's `fastq_taglist`), and a hybrid built without the argument parser gets `minimap2_args="-Y"`, its documented default, instead of the long-read pipeline's `-YL`. A new test asserts, for every pipeline, that each argument is declared in `__init__` with its argparse default. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The dnascope and long-read pipelines renamed their input attributes during validation (`r1_fastq` to `sr_r1_fastq`, `sample_input` to `lr_aln`, and so on) so that the hybrid pipeline could inherit from both without the names colliding. The hybrid no longer inherits from either, so each pipeline now reads its own argument names throughout. Two validation tests that set the renamed attributes before `validate()` ran, and so exercised the wrong branch, now assert the messages they were written for. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The version constants no longer all live in the pipeline modules. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The hybrid pipeline's long-read input may be ONT, whose very long reads can carry more than 65535 CIGAR operations, which the BAM format cannot store; `-L` moves such CIGAR strings into the `CG` tag. The hidden `--minimap2_args` default therefore becomes `-YL`, matching the long-read pipeline and the minimap2 stages, instead of `-Y`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`SentieonPangenome` overrode `main()` to parse the reference index and population VCF before validation, call `build_first_dag()` instead of `build_dag()`, and gate the second, sex-aware DAG itself. It now uses the base flow like every other pipeline: `validate()` parses the index, shards and VCF contigs (as the hybrid pangenome already did), the main DAG builder is `build_dag()`, and `build_second_dag()` returns `None` when no sex-aware caller is enabled and calls `get_sex` itself, as the hybrid DNAscope pipeline does. The reference index is validated once rather than twice. A new test asserts that no pipeline overrides `main()`. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Four pipelines carried an identical `transfer_config()` helper building a `TransferConfig` from `pop_vcf`, `shards`, `pop_vcf_contigs` and `fai_data`. A `TransferInputs` protocol names those four attributes and `TransferConfig.from_pipeline` reads them structurally, raising a clear error when there is no population VCF. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
`LongReadSVStage` gains an explicit `output` (the derived `.sv.vcf.gz` path stays the default) and the LongReadSV algo options (`min_map_qual`, `min_sv_size`, `min_dp`, `min_af`), which is what the hybrid pangenome's graph-update SV call needed. Its hand-built `build_longreadsv_job` is gone; the job keeps its `longreadsv` name under the `pangenome-update` task and its command is unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
The smoke reference is not a recognised genome build, so no PAR BED resolves and CNV calling now fails validation up front. The smoke runs exercise alignment, duplicate marking and variant calling, not CNVscope, so they skip it. Co-Authored-By: Claude Fable 5.1 <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.
Refactor the pipeline code to introduce reusable modules (stages) that can be shared across pipelines, reducing code duplication.