Use new nf-test features #6286

edmundmiller · 2024-08-23T16:30:16Z

ci: Attempt to split everything out
ci: Add changed since, sharding, and ci
ci: Add filter to try to get jobs split up
ci: Switch to only-changed
ci: See if follow-dependencies works without "related-tests"

edmundmiller · 2024-08-26T16:34:12Z

We do have adamrtalbot/detect-nf-test-changes as a fallback option?

That's what we're currently using, so I don't really see it as a fallback 😆 It works fine, I think the sharding can work better and waste less resources spinning up a runner for each test instead of spinning up a consistent amount per PR.

That would close #391 as well, and might encourage people to make smaller PRs if we're only giving them 9 CI runners per PR anyway.

edmundmiller · 2024-09-18T17:13:09Z

Current things blocking this is nf-core/tools#3140 or we can give up and keep the paths-filter step for that

edmundmiller · 2024-09-18T21:05:38Z

Removed the need to use nf-core lint with pre-commit, as that wasn't truely blocking this from getting merged.

We can make a follow up issue to run every linting step through pre-commit.

maxulysse · 2024-09-19T06:34:59Z

Looking good, there's still some leftover comments in the nf-test yml, but I like it

edmundmiller · 2024-09-24T13:48:30Z

@maxulysse I think I've address the comments that can be addressed.

@adamrtalbot I think this goes slightly against #4545 (comment) but I think it's worth splitting up since we should be running pytest-workflow less and less, and the sharding is a completely different flow. Though it will probably Start up the pytest-workflow workflow every PR...

adamrtalbot

My main concern is getting stuck in a loop where there is too much data per runner and having no way of resolving it.

We over shard now, but at least it's a 1:1 relationship which makes it easy to diagnose an issue. We need to be confident we don't end up in a death loop in the middle of a hackathon.

In addition, we're over-provisioning 3 machines when 1 would suffice.

Still, this has fewer moving parts and is easier to replicate on a local machine, which makes it better under certain situations.

adamrtalbot · 2024-09-30T16:36:42Z

.github/workflows/lint.yml

+      - uses: mirpedrol/paths-filter@main
+        id: filter
+        with:
+          filters: "tests/config/pytest_modules.yml"


This relies on the pytest_modules.yml tags, is that what you want?

Nah, probably just want it to run on any module that's changed. We could include both the nf-test and pytest_modules ymls though I think.

adamrtalbot · 2024-09-30T16:41:34Z

.github/workflows/nf-test.yml

+      matrix:
+        filter: [process, workflow]
+        profile: [conda, docker_self_hosted, singularity]
+        shard: [1, 2, 3]


3 is a very small number for sharding.

If we edit UNTAR, we will run hundreds of tests packed onto the same machines. Remember, if too many tests are packed onto 1 machine there is no way to fix it other than editing this file which essentially blocks the person who opened the PR.

Conversely, if we run 1 nf-test we will add 2 machines that do nothing but install nf-test and close down.

This might be the most practical way of doing this, so I'm not 100% against it but what's the rationale for the number 3 and can we document it somewhere.

Is it possible to set it dynamically based on number of tests with an estimate of how many we can pack onto a machine? e.g. n_tests %% 5?

Of course, by making it dyanmic we're back to the current problem of having a dynamic workflow ID which causes issues with GH branch protection.

So let's go back to my initial point. If you modify a major process like UNTAR, will this still succeed or will it run out of storage? Remember every nf-test invocation downloads all the data it needs.

Is it possible to set it dynamically based on number of tests with an estimate of how many we can pack onto a machine? e.g. n_tests %% 5?

That would be the dream. But yeah I also wanted to avoid the dynamic number of jobs.

3 was just a random number that I picked. Trying to walk the line between runners sitting there, and not enough jobs.

Hot take: What if we just did a --dryRun at the beginning of the shard and if there's no jobs, exit before we set anything else up.

adamrtalbot · 2024-09-30T16:41:51Z

.github/workflows/nf-test.yml

+    strategy:
+      fail-fast: false
+      matrix:
+        filter: [process, workflow]


Suggested change

filter: [process, workflow]

filter: [process, workflow]

Function?

Do we have any in this repo?

Yep: https://github.com/nf-core/modules/blob/master/subworkflows/nf-core/utils_nfcore_pipeline/main.nf

Also, I'd like to encourage people to put this stuff in modules or a plugin so we don't fix it to the template.

adamrtalbot · 2024-09-30T16:51:17Z

.github/workflows/pytest-workflow.yml

+  confirm-pass:
+    runs-on: ubuntu-latest
+    needs: [pytest-changes, pytest]
+    if: always()
+    steps:
+      - name: All tests ok
+        if: ${{ success() || !contains(needs.*.result, 'failure') }}
+        run: exit 0
+      - name: One or more tests failed
+        if: ${{ contains(needs.*.result, 'failure') }}
+        run: exit 1
+
+      - name: debug-print
+        if: always()
+        run: |
+          echo "toJSON(needs) = ${{ toJSON(needs) }}"
+          echo "toJSON(needs.*.result) = ${{ toJSON(needs.*.result) }}"


You need to add one of these to every dynamic matrix, so it must be added to all the linting workflow as well.

Yeah, I left it out orginally because I'm hoping to get everything to run in pre-commit and we can do away with the filters and make the branch protections happy.

There's no way of forcing someone to use a pre-commit and I'm wary of forcing people to install stuff on their laptops. In general, it's a good idea to verify code at the source of truth (github), not in isolation. It's much easier to prevent bad code being added then remove it later.

Perhaps we could run pre-commit before merging? Never really looked into it.

https://github.com/nf-core/modules/blob/3c464e75051db485c1b37ab9f1ea2182fb3d3533/.github/workflows/test.yml#L29C2-L39C36
We already are ✨

I was talking about pre-commit the tool, not a pre-commit git hook.

Agreed on not expecting people to jump through a ton of hoops to get their local dev environments set up. (But I think this might be over since gitpod switched to self-hosted, but they also adopt devcontainers, which if we can get those setup well, anyone with vscode and docker should be able to start hacking quickly.)

adamrtalbot · 2024-09-30T18:30:30Z

Also - the GHA says it uses actions/setup-java v4 but so does master and they're in conflict 😬

You should add v4.4.0 for that action as a comment since you are referencing an explicit commit.

adamrtalbot · 2024-09-30T18:35:27Z

Stress test: #6716

adamrtalbot

Oh just spotted. Currently the tests aren't doing anything (this is every test!):

🚀 nf-test 0.9.0
https://www.nf-test.com/
(c) 2021 - 2024 Lukas Forer and Sebastian Schoenherr

Load .nf-test/plugins/nft-bam/0.3.0/nft-bam-0.3.0.jar
Load .nf-test/plugins/nft-vcf/1.0.7/nft-vcf-1.0.7.jar
Nothing to do.

https://nfcore.slack.com/archives/CJRH30T6V/p1724406283145319

#6505

Blocked by nf-core/tools#3140

#4554 (comment)

a004c86 2da71b7

adamrtalbot · 2024-10-01T09:57:46Z

number of tests before running out of space:

test	method	tests
process	conda 2	33+
process	conda 3	29+
process	docker_self_hosted 1	18
process	docker_self_hosted 2	30+
process	docker_self_hosted 3
process	singularity 3	37+

Then GHA started clearing out the logs so I couldn't count 😒 .

Still, it looks like 30 is the max for a normal set of process tests. Workflows are fine.

edmundmiller · 2024-10-17T03:08:07Z

Still, it looks like 30 is the max for a normal set of process tests. Workflows are fine.

What's your take on it then? Is 30 enough?

What if we used

    strategy:
      max-parallel: 2

or 3 or whatever we decide. Then we could do 6 or 7 for the number of shards and ideally the if there's a small number of tests then they'll finish quickly, but one massive PR won't hold up the CI runners from other small PRs.

edmundmiller · 2024-10-17T03:18:37Z

What if

$ nf-test test --dry-run   --filter process --changedSince origin/master

🚀 nf-test 0.9.0
https://www.nf-test.com
(c) 2021 - 2024 Lukas Forer and Sebastian Schoenherr

Load .nf-test/plugins/nft-bam/0.3.0/nft-bam-0.3.0.jar
Load .nf-test/plugins/nft-vcf/1.0.7/nft-vcf-1.0.7.jar
Detected 9 changed files
  • .github/conda_skip.yml
  • .github/workflows/lint.yml
  • .github/workflows/nf-test.yml
  • .github/workflows/pytest-workflow.yml
  • .github/workflows/test.yml
  • .pre-commit-config.yaml
  • modules/nf-core/bowtie/align/main.nf
  • modules/nf-core/bowtie/build/tests/main.nf.test
  • modules/nf-core/bowtie/build/tests/main.nf.test.snap
Found 2 related test(s)
  • /Users/emiller/src/nf-core/modules/modules/nf-core/bowtie/build/tests/main.nf.test
  • /Users/emiller/src/nf-core/modules/modules/nf-core/bowtie/align/tests/main.nf.test
Dry run mode activated: tests are not executed, just listed.

Test Process BOWTIE_ALIGN

  Test [66a82e02] 'sarscov2 - single_end' PASSED (0.0s)
  Test [5cac2e90] 'sarscov2 - single_end - stub' PASSED (0.0s)
  Test [c7794ac1] 'sarscov2 - paired_end' PASSED (0.0s)
  Test [c3125b42] 'sarscov2 - paired_end - stub' PASSED (0.0s)

Test Process BOWTIE_BUILD

  Test [9a210173] 'sarscov2 - fasta' PASSED (0.0s)
  Test [706b144c] 'sarscov2 - fasta - stub' PASSED (0.0s)


SUCCESS: Executed 6 tests in 0.06s

We parse this for the related tests line, then pick the number of shards based off the number of tests found.

maxulysse · 2024-10-23T09:52:43Z

We parse this for the related tests line, then pick the number of shards based off the number of tests found.

that would work, but we probably can do something smarter than that.
But that's good enough for me now

edmundmiller self-assigned this Aug 23, 2024

edmundmiller force-pushed the fresh-ci branch 3 times, most recently from 489e5fb to 26653c8 Compare August 23, 2024 17:36

edmundmiller mentioned this pull request Aug 26, 2024

Pass a list of modules/subworkflows(files) to nf-core modules lint nf-core/tools#3140

Open

edmundmiller mentioned this pull request Sep 18, 2024

Caused by: java.nio.channels.OverlappingFileLockException #6664

Open

edmundmiller force-pushed the fresh-ci branch from 6030ae8 to 5594071 Compare September 18, 2024 16:52

edmundmiller force-pushed the fresh-ci branch 3 times, most recently from 9cd5419 to 8748f04 Compare September 18, 2024 21:01

edmundmiller marked this pull request as ready for review September 18, 2024 21:03

edmundmiller requested review from KevinMenden, drpatelh and a team as code owners September 18, 2024 21:03

edmundmiller requested review from ewels and maxulysse September 18, 2024 21:04

edmundmiller linked an issue Sep 18, 2024 that may be closed by this pull request

Caused by: java.nio.channels.OverlappingFileLockException #6664

Open

edmundmiller mentioned this pull request Sep 24, 2024

In-verse tag selection askimed/nf-test#260

Open

edmundmiller force-pushed the fresh-ci branch from da25f80 to 2516a34 Compare September 24, 2024 13:54

edmundmiller mentioned this pull request Sep 27, 2024

ci: One changes job to rule them all #4284

Closed

edmundmiller requested a review from adamrtalbot September 30, 2024 13:18

adamrtalbot reviewed Sep 30, 2024

View reviewed changes

adamrtalbot requested changes Sep 30, 2024

View reviewed changes

edmundmiller added 5 commits September 30, 2024 15:25

ci: Lint everything

52a826d

ci: Get the job names clean

1043d64

ci: Add hide-progress on linting

94c2453

ci: Add psuedocode for conda-fail.yml

ab5194b

https://nfcore.slack.com/archives/CJRH30T6V/p1724406283145319

test: Snapshot the versions contents, not the hash

2a218e3

#6505

edmundmiller force-pushed the fresh-ci branch from 089f23f to e699438 Compare September 30, 2024 20:26

Edmund Miller and others added 6 commits September 30, 2024 15:27

ci: Keep running nf-core lint the way it was

e833cd5

Blocked by nf-core/tools#3140

ci: Move conda skips out

392ffe7

ci: Address a comment

af2d917

style: Move prettier and editorconfig to pre-commit

372625e

#4554 (comment)

ci: Add note about nf-core lint pre-commit

1670c52

chore: Copy over conda skips

adf6658

a004c86 2da71b7

edmundmiller force-pushed the fresh-ci branch from e699438 to 89cd153 Compare September 30, 2024 20:28

ci: only-changed => changed-since

d0caf9e

edmundmiller force-pushed the fresh-ci branch from 89cd153 to d0caf9e Compare September 30, 2024 20:29

edmundmiller requested a review from jfy133 as a code owner September 30, 2024 20:48

edmundmiller force-pushed the fresh-ci branch from fed0e6c to d0caf9e Compare September 30, 2024 20:49

jfy133 removed their request for review October 14, 2024 09:04

Add confirm-pass

3b97a33

edmundmiller requested a review from a team as a code owner October 14, 2024 18:37

edmundmiller and others added 6 commits October 16, 2024 22:39

ci: dynamically set shards

865787a

ci: Run 3 process jobs per CI run as an example

c141a14

Merge branch 'master' into fresh-ci

c984d91

install nf-test

d355521

fix install nf-test

f42d82a

fix install nf-test

c99a2fc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use new nf-test features #6286

Use new nf-test features #6286

edmundmiller commented Aug 23, 2024

edmundmiller commented Aug 26, 2024 •

edited

Loading

edmundmiller commented Sep 18, 2024

edmundmiller commented Sep 18, 2024

maxulysse commented Sep 19, 2024

edmundmiller commented Sep 24, 2024 •

edited

Loading

adamrtalbot left a comment

adamrtalbot Sep 30, 2024

edmundmiller Sep 30, 2024

adamrtalbot Sep 30, 2024 •

edited

Loading

adamrtalbot Sep 30, 2024

adamrtalbot Sep 30, 2024

edmundmiller Sep 30, 2024

adamrtalbot Sep 30, 2024

edmundmiller Sep 30, 2024

adamrtalbot Oct 1, 2024

adamrtalbot Sep 30, 2024

edmundmiller Sep 30, 2024

adamrtalbot Oct 1, 2024

edmundmiller Oct 17, 2024 •

edited

Loading

adamrtalbot commented Sep 30, 2024

adamrtalbot commented Sep 30, 2024

adamrtalbot left a comment

adamrtalbot commented Oct 1, 2024

edmundmiller commented Oct 17, 2024

edmundmiller commented Oct 17, 2024

maxulysse commented Oct 23, 2024

Use new nf-test features #6286

Are you sure you want to change the base?

Use new nf-test features #6286

Conversation

edmundmiller commented Aug 23, 2024

edmundmiller commented Aug 26, 2024 • edited Loading

edmundmiller commented Sep 18, 2024

edmundmiller commented Sep 18, 2024

maxulysse commented Sep 19, 2024

edmundmiller commented Sep 24, 2024 • edited Loading

adamrtalbot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamrtalbot Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edmundmiller Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

adamrtalbot commented Sep 30, 2024

adamrtalbot commented Sep 30, 2024

adamrtalbot left a comment

Choose a reason for hiding this comment

adamrtalbot commented Oct 1, 2024

edmundmiller commented Oct 17, 2024

edmundmiller commented Oct 17, 2024

maxulysse commented Oct 23, 2024

edmundmiller commented Aug 26, 2024 •

edited

Loading

edmundmiller commented Sep 24, 2024 •

edited

Loading

adamrtalbot Sep 30, 2024 •

edited

Loading

edmundmiller Oct 17, 2024 •

edited

Loading