chore: test on arm #392
chore: test on arm #392
Conversation
WalkthroughAdded a GitHub Actions matrix to run tests on multiple runner images and replaced many Snakemake wrapper references with Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant Matrix as Matrix Expander
participant RunnerA as ubuntu-latest
participant RunnerB as ubuntu-24.04-arm
Dev->>GH: Push/PR triggers workflow
GH->>Matrix: Expand matrix (image)
Matrix-->>RunnerA: Schedule job (runs-on = ubuntu-latest)
Matrix-->>RunnerB: Schedule job (runs-on = ubuntu-24.04-arm)
RunnerA->>RunnerA: Execute job (name uses matrix.image)
RunnerB->>RunnerB: Execute job (name uses matrix.image)
RunnerA-->>GH: Report result
RunnerB-->>GH: Report result
GH-->>Dev: Aggregate results
sequenceDiagram
participant Snakemake as Snakemake Engine
participant Rule as build_sample_regions
participant Wrapper as mosdepth:v7.6.0
participant Downstream as Dependent Rules
Snakemake->>Rule: Request `results/regions/{group}/{sample}.*`
Rule->>Wrapper: Run with named outputs (dist, quant, bed, bed_dist)
Wrapper-->>Rule: Produce named outputs
Rule-->>Snakemake: Return named outputs
Snakemake->>Downstream: Bind downstream inputs to named outputs (dist/quant/bed/bed_dist)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| image: | ||
| - ubuntu-latest | ||
| - ubuntu-24.04-arm |
There was a problem hiding this comment.
Fix the ARM runner label.
ubuntu-24.04-arm is not a valid GitHub-hosted runner label, so the job will sit in “Waiting for a runner to pick up this job” and never execute. Use ubuntu-24.04-arm64 instead to target the official ARM image.
Also applies to: 66-67
🤖 Prompt for AI Agents
.github/workflows/main.yml around lines 20-22 (and also at 66-67): the workflow
uses an invalid runner label "ubuntu-24.04-arm" which prevents jobs from
starting; replace that label with the correct GitHub-hosted runner label
"ubuntu-24.04-arm64" in both places so the jobs will run on the official ARM64
image.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
workflow/rules/annotation.smk(3 hunks)workflow/rules/benchmarking.smk(5 hunks)workflow/rules/calling.smk(2 hunks)workflow/rules/candidate_calling.smk(3 hunks)workflow/rules/datavzrd.smk(4 hunks)workflow/rules/filtering.smk(2 hunks)workflow/rules/fusion_calling.smk(3 hunks)workflow/rules/maf.smk(1 hunks)workflow/rules/mapping.smk(11 hunks)workflow/rules/primers.smk(2 hunks)workflow/rules/qc.smk(4 hunks)workflow/rules/ref.smk(7 hunks)workflow/rules/testcase.smk(1 hunks)workflow/rules/trimming.smk(3 hunks)workflow/rules/utils.smk(3 hunks)
✅ Files skipped from review due to trivial changes (3)
- workflow/rules/maf.smk
- workflow/rules/annotation.smk
- workflow/rules/datavzrd.smk
🔇 Additional comments (29)
workflow/rules/candidate_calling.smk (1)
23-23: Verify wrapper compatibility and ARM test results
- Wrappers bio/freebayes, bio/delly and bio/bcftools/filter v7.6.0 are available; confirm no breaking changes to parameters or output formats.
- Ensure the updated rules have been tested and pass on ARM architecture.
workflow/rules/testcase.smk (1)
17-17: LGTM! Consistent wrapper upgrade.The bcftools concat wrapper upgrade to v7.6.0 is consistent with changes in other files (e.g., filtering.smk).
workflow/rules/primers.smk (1)
62-62: LGTM! Wrapper upgrades are consistent with PR objectives.The bwa mem (v2.13.0 → v7.6.0) and samtools view (v2.3.2 → v7.6.0) wrapper upgrades align with the ARM testing objectives mentioned in the PR.
Also applies to: 75-75
workflow/rules/calling.smk (1)
111-111: LGTM! Bcftools wrapper upgrades are consistent.The bcftools sort and concat wrapper upgrades to v7.6.0 are consistent with changes across the workflow.
Also applies to: 125-125
workflow/rules/benchmarking.smk (1)
16-16: LGTM! Comprehensive wrapper upgrade for benchmarking rules.All five wrapper references have been consistently upgraded to v7.6.0 across benchmarking rules: bcftools concat, chm-eval-sample, samtools sort, chm-eval-kit, and chm-eval.
Also applies to: 26-26, 40-40, 68-68, 114-114
workflow/rules/qc.smk (1)
12-12: LGTM! QC wrapper upgrades are comprehensive.All QC-related wrappers (fastqc, samtools idxstats, samtools stats, and multiqc) have been consistently upgraded to v7.6.0.
Also applies to: 24-24, 35-35, 53-53
workflow/rules/utils.smk (1)
22-22: LGTM! Utility wrapper upgrades complete the migration to v7.6.0.The bcftools view, samtools index, and tabix index wrappers have been upgraded to v7.6.0, completing the comprehensive wrapper migration across the workflow.
Also applies to: 33-33, 47-47
workflow/rules/trimming.smk (1)
8-8: Wrapper interfaces unchanged between versions
The v7.6.0 wrappers for both sra-tools/fasterq-dump and fastp use the same parameters and default behavior as v5.0.2 and v6.2.0, so no breaking changes to interfaces or defaults. Confirm that ARM-compatible binaries are available for these new versions.workflow/rules/fusion_calling.smk (3)
3-3: Wrapper bump looks good.New Arriba meta wrapper version aligns with the repo-wide upgrade. No further action needed.
98-98: Wrapper bump looks good.Updating to
v7.6.0/bio/bcftools/sortstays consistent with the other upgrades.
116-116: Wrapper bump looks good.
bcftools concatnow matches the v7.6.0 toolchain without requiring extra adjustments.workflow/rules/ref.smk (7)
14-14: Wrapper bump looks good.
ensembl-sequencev7.6.0 is fine; inputs/params remain compatible.
26-26: Wrapper bump looks good.
samtools/faidxupgrade is straightforward.
59-59: Wrapper bump looks good.
ensembl-variationmoves to v7.6.0 without downstream changes.
74-74: Wrapper bump looks good.
ensembl-annotationv7.6.0 aligns with the rest of the ref pipeline.
123-123: Wrapper bump looks good.
bwa/indexv7.6.0 should behave identically here.
137-137: Wrapper bump looks good.
vep/cacheupgrade keeps cache handling consistent.
148-148: Wrapper bump looks good.
vep/pluginsv7.6.0 slots in cleanly.workflow/rules/mapping.smk (11)
13-13: Wrapper bump looks good.
bwa/memv7.6.0 keeps the mapping rule aligned with the other upgrades.
76-76: Wrapper bump looks good.
vg/giraffeupgrade matches the updated wrapper set.
107-107: Wrapper bump looks good.
samtools/fixmatev7.6.0 should behave the same here.
150-150: Wrapper bump looks good.
samtools/sortversion bump is consistent with the rest of the workflow.
181-181: Wrapper bump looks good.
picard/markduplicatesv7.6.0 is a straight upgrade.
217-217: Wrapper bump looks good.Second
bwa/meminvocation now uses the updated wrapper.
231-231: Wrapper bump looks good.
samtools/mergev7.6.0 requires no other changes.
243-243: Wrapper bump looks good.
samtools/sortupgrade for consensus reads aligns with earlier changes.
265-265: Wrapper bump looks good.
gatk/splitncigarreadsv7.6.0 remains compatible.
288-288: Wrapper bump looks good.
gatk/baserecalibratorsparkv7.6.0 matches the toolkit refresh.
311-311: Wrapper bump looks good.
gatk/applybqsrv7.6.0 reflects the coordinated upgrade.
| extra="-a", | ||
| wrapper: | ||
| "v2.3.2/bio/bcftools/concat" | ||
| "v7.6.0/bio/bcftools/concat" |
There was a problem hiding this comment.
🧩 Analysis chain
LGTM! Wrapper version upgrade is consistent.
The bcftools concat wrapper has been upgraded to v7.6.0 in both gather_calls and merge_calls rules. Ensure that this version supports ARM architecture and is backwards compatible with your existing workflows.
Run the following script to verify the wrapper version exists and check for any ARM-specific compatibility notes:
Also applies to: 95-95
🏁 Script executed:
#!/bin/bash
# Description: Verify bcftools concat wrapper v7.6.0 exists and check ARM compatibility
# Check if the wrapper version exists in the Snakemake wrappers repository
curl -s "https://api.github.com/repos/snakemake/snakemake-wrappers/contents/bio/bcftools/concat" | \
jq -r '.[].name' | grep -q "v7.6.0" && echo "✓ v7.6.0 exists" || echo "✗ v7.6.0 not found"
# Fetch the wrapper meta.yaml to check for platform compatibility
curl -s "https://raw.githubusercontent.com/snakemake/snakemake-wrappers/v7.6.0/bio/bcftools/concat/meta.yaml"Length of output: 497
Use a valid bcftools concat wrapper version
v7.6.0 isn’t found in the Snakemake wrappers repository. Update both gather_calls and merge_calls rules (lines 65 & 95 in workflow/rules/filtering.smk) to a released version (e.g., v2.3.2).
🤖 Prompt for AI Agents
In workflow/rules/filtering.smk around lines 65 and 95, the bcftools concat
wrapper version "v7.6.0" is invalid; update both occurrences in the gather_calls
and merge_calls rules to a released wrapper version (for example replace
"v7.6.0/bio/bcftools/concat" with "v2.3.2/bio/bcftools/concat"). Ensure both
lines use the same valid version string and run a quick Snakemake dry-run to
confirm the wrapper is found.
Summary by CodeRabbit
Chores
Refactor