Add scatter CI #350
Merged
Add scatter CI #350
Conversation
This file contains 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
|
sateeshperi
reviewed
Oct 29, 2023
9 tasks
edmundmiller
force-pushed
the
scatter
branch
2 times, most recently
from
7fdbeae to
032c36b
Compare
sateeshperi
approved these changes
Nov 6, 2023
auto-merge was automatically disabled
November 7, 2023 03:08
Pull Request is not mergeable
|
Sounds good, once I get the tests passing in this PR at least 😆 |
sateeshperi
reviewed
Dec 22, 2023
adamrtalbot
reviewed
Dec 22, 2023
Comment on lines
+69
to
+76
| - 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 }} |
There was a problem hiding this comment.
Let's add Nextflow to this cache as well.
Comment on lines
+34
to
+35
| - name: Setup Nextflow | ||
| uses: nf-core/setup-nextflow@v1 |
There was a problem hiding this comment.
We're going to hit the API limits again, I think we should add Nextflow to the software cache (see other comment).
| - name: List nf-test | ||
| id: list | ||
| run: | | ||
| echo "tests=$(nf-test list --silent --format=json)" >> "$GITHUB_OUTPUT" |
There was a problem hiding this comment.
I didn't know about this...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We could create an nf-test pre-commit...
There was a problem hiding this comment.
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.
| - 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 }}" \ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You don't seem to have any check that could be used for branch protection?
Co-authored-by: Adam Talbot <[email protected]>
Co-authored-by: Sateesh_Peri <[email protected]>
sateeshperi
reviewed
Dec 22, 2023
Again Co-authored-by: Sateesh_Peri <[email protected]>
Co-authored-by: sateeshperi <[email protected]>
edmundmiller
force-pushed
the
scatter
branch
from
9ccf62e to
9d88d67
Compare
Closed
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Uses a GitHub runner per test to significantly speed up CI times.
It does break the flaky index tests.