Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add scatter CI #350

Merged
merged 14 commits into from
Dec 22, 2023
Merged
126 changes: 70 additions & 56 deletions .github/workflows/ci.yml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't seem to have any check that could be used for branch protection?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

Original file line number Diff line number Diff line change
Expand Up @@ -14,97 +14,111 @@ on:

env:
NXF_ANSI_LOG: false
NFTEST_VER: "0.7.3"
NFTEST_VER: "0.8.1"
edmundmiller marked this conversation as resolved.
Show resolved Hide resolved

concurrency:
group: "${{ github.workflow }}-${{ github.event.pull_request.number || github.ref }}"
cancel-in-progress: true

jobs:
define_nxf_versions:
name: Choose nextflow versions to test against depending on target branch
runs-on: ubuntu-latest
list:
name: List Tests
if: "${{ github.event_name != 'push' || (github.event_name == 'push' && github.repository == 'nf-core/methylseq') }}"
outputs:
matrix: ${{ steps.nxf_versions.outputs.matrix }}
# Expose matched filters as job 'modules' output variable
tests: ${{ steps.list.outputs.tests }}
runs-on: ubuntu-latest
steps:
- id: nxf_versions
- uses: actions/checkout@v3

- name: Setup Nextflow
uses: nf-core/setup-nextflow@v1
Comment on lines +34 to +35
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're going to hit the API limits again, I think we should add Nextflow to the software cache (see other comment).


- name: Install nf-test
if: steps.cache-software.outputs.cache-hit != 'true'
run: |
if [[ "${{ github.event_name }}" == "pull_request" && "${{ github.base_ref }}" == "dev" && "${{ matrix.NXF_VER }}" != "latest-everything" ]]; then
echo matrix='["latest-everything"]' | tee -a $GITHUB_OUTPUT
else
echo matrix='["latest-everything", "23.04.0"]' | tee -a $GITHUB_OUTPUT
fi
wget -qO- https://code.askimed.com/install/nf-test | bash
sudo mv nf-test /usr/local/bin/

- name: List nf-test
id: list
run: |
echo "tests=$(nf-test list --silent --format=json)" >> "$GITHUB_OUTPUT"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know about this...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you run this at dev time instead of in the action? It seems wasteful to create an entire job that runs this every single time when it only changes when you run the test.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could create an nf-test pre-commit...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's an option. But how is this worse than checking for changes in modules?

We'll also be adding the check for changes to this job as well to run only CI jobs on changed code.


test:
name: Run pipeline with test data
needs: define_nxf_versions
name: ${{ matrix.tags }} (${{ matrix.profile }}-${{ matrix.NXF_VER }})
# Only run on push if this is the nf-core dev branch (merged PRs)
if: needs.list.outputs.tests != '[]'
needs: [list]
runs-on: ubuntu-latest
strategy:
fail-fast: false
matrix:
NXF_VER: ${{ fromJson(needs.define_nxf_versions.outputs.matrix) }}
test_tags:
- "bismark"
- "bismark_hisat"
- "bwameth"
profile:
- "docker"

NXF_VER:
- "23.04.0"
- "latest-everything"
profile: ["docker"] # TODO , "singularity", "conda"]
test: ["${{ fromJson(needs.list.outputs.tests) }}"]
exclude:
- test: "tests/subworkflows/local/grohmm/main.nf.test@3508b8c9"
- test: "tests/subworkflows/local/grohmm/main.nf.test@716555be"
- test: "tests/subworkflows/local/grohmm/main.nf.test@306b4b15"
steps:
- name: Check out pipeline code
uses: actions/checkout@v3

- name: Hash Github Workspace
id: hash_workspace
run: |
echo "digest=$(echo methylseq2_${{ github.workspace }} | md5sum | cut -c 1-25)" >> $GITHUB_OUTPUT

- name: Cache test data
id: cache-testdata
- name: Cache Nextflow installation
id: cache-software
uses: actions/cache@v3
with:
path: test-datasets/
key: ${{ steps.hash_workspace.outputs.digest }}

- name: Check out test data
uses: actions/checkout@v3
with:
repository: nf-core/test-datasets
ref: methylseq
path: test-datasets/

- name: Replace remote paths in samplesheets
run: |
for f in ./test-datasets/samplesheet/*csv; do
sed -i 's=https://github.com/nf-core/test-datasets/raw/methylseq/=./test-datasets/=g' $f
sed -i 's=https://raw.githubusercontent.com/nf-core/test-datasets/methylseq/=./test-datasets/=g' $f
echo "========== $f ============"
cat $f
echo "========================================"
done;
path: |
/usr/local/bin/nf-test
/home/runner/.nf-test/nf-test.jar
key: methylseq-${{ runner.os }}-${{ matrix.NXF_VER }}
Comment on lines +69 to +76
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add Nextflow to this cache as well.


- name: Install Nextflow
uses: nf-core/setup-nextflow@v1
with:
version: "${{ matrix.NXF_VER }}"

- name: Install nf-test
if: steps.cache-software.outputs.cache-hit != 'true'
run: |
wget -qO- https://code.askimed.com/install/nf-test | bash -s $NFTEST_VER
wget -qO- https://code.askimed.com/install/nf-test | bash
sudo mv nf-test /usr/local/bin/

- name: Set up Singularity
if: matrix.profile == 'singularity'
uses: eWaterCycle/setup-singularity@v5
with:
singularity-version: 3.7.1

- name: Set up miniconda
if: matrix.profile == 'conda'
uses: conda-incubator/setup-miniconda@v2
with:
auto-update-conda: true
channels: conda-forge,bioconda,defaults
python-version: ${{ matrix.python-version }}

- name: Conda clean
if: matrix.profile == 'conda'
run: conda clean -a

- name: Run nf-test
run: |
nf-test test --tag ${{ matrix.test_tags }} --profile "test,${{ matrix.profile }}" --junitxml=test.xml
nf-test test \
--profile="test,${{ matrix.profile }}" \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still tempted to group up conda, singularity and docker onto the same job to make things more efficient. Sure, they will run sequentially which will slow things down but we will reduce our load on GHA by 1/3rd. Food for thought, not necessary right now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's my arguement against that. I don't think efficiency is the only thing to optomize for here.

It's a developer tool, and I think that will confuse the pipeline developers and new contributors.

It's also the same amount of wall time, if you don't include the setup. I'd rather spend our time optimizing the setup and cacheing properly to cut down on that.

The other thing splitting up the jobs allow us to do is to fail fast. Instead of getting through docker and singularity and then condad failing a checksum, we then have to rerun all three, instead of just conda. So we end up using 3x the walltime anyways if the CI doesn't work perfectly the first time.

${{ matrix.test }} \
--junitxml=${{ matrix.test }}-${{ matrix.profile }}.xml

- name: Publish Test Report
uses: mikepenz/action-junit-report@v3
if: success() || failure() # run this step even if previous step failed
with:
path: ${{ matrix.test }}-${{ matrix.profile }}.xml

- name: Output log on failure
if: failure()
run: |
sudo apt install bat > /dev/null
batcat --decorations=always --color=always ${{ github.workspace }}/.nf-test/tests/*/output/pipeline_info/software_versions.yml

- name: Publish Test Report
uses: mikepenz/action-junit-report@v3
if: always() # always run even if the previous step fails
with:
report_paths: test.xml
5 changes: 5 additions & 0 deletions modules.json
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,11 @@
"branch": "master",
"git_sha": "911696ea0b62df80e900ef244d7867d177971f73",
"installed_by": ["modules"]
},
"untar": {
"branch": "master",
"git_sha": "8fc1d24c710ebe1d5de0f2447ec9439fd3d9d66a",
"installed_by": ["modules"]
}
}
}
Expand Down
8 changes: 8 additions & 0 deletions modules/nf-core/untar/environment.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

63 changes: 63 additions & 0 deletions modules/nf-core/untar/main.nf

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

46 changes: 46 additions & 0 deletions modules/nf-core/untar/meta.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions nf-test.config
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
config {
// location for all nf-tests
testsDir "tests"
testsDir "."

// nf-test directory including temporary files for each test
workDir ".nf-test"
workDir "/tmp"

// location of library folder that is added automatically to the classpath
libDir "tests/pipeline/lib/"
libDir "lib/"

// location of an optional nextflow.config file specific for executing tests
configFile "nextflow.config"
Expand Down
13 changes: 11 additions & 2 deletions subworkflows/local/prepare_genome.nf
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Prepare reference genome files
//

include { UNTAR } from '../../modules/nf-core/untar/main'
include { BISMARK_GENOMEPREPARATION } from '../../modules/nf-core/bismark/genomepreparation/main'
include { BWAMETH_INDEX } from '../../modules/nf-core/bwameth/index/main'
include { SAMTOOLS_FAIDX } from '../../modules/nf-core/samtools/faidx/main'
Expand All @@ -27,7 +28,11 @@ workflow PREPARE_GENOME {
* Generate bismark index if not supplied
*/
if (params.bismark_index) {
ch_bismark_index = Channel.value(file(params.bismark_index))
if (params.bismark_index.endsWith('.gz')) {
ch_bismark_index = UNTAR ( [ [:], file(params.bismark_index) ] ).untar.map { it[1] }
} else {
ch_bismark_index = Channel.value(file(params.bismark_index))
}
} else {
BISMARK_GENOMEPREPARATION(ch_fasta)
ch_bismark_index = BISMARK_GENOMEPREPARATION.out.index
Expand All @@ -42,7 +47,11 @@ workflow PREPARE_GENOME {
* Generate bwameth index if not supplied
*/
if (params.bwa_meth_index) {
ch_bwameth_index = Channel.value(file(params.bwa_meth_index))
if (params.bwa_meth_index.endsWith('.gz')) {
edmundmiller marked this conversation as resolved.
Show resolved Hide resolved
ch_bismark_index = UNTAR ( [ [:], file(params.bwa_meth_index) ] ).untar.map { it[1] }
} else {
ch_bismark_index = Channel.value(file(params.bwa_meth_index))
}
} else {
BWAMETH_INDEX(ch_fasta)
ch_bwameth_index = BWAMETH_INDEX.out.index
Expand Down
Loading
Loading