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

Snapshot content of version files instead of a hash #6505

Open
Tracked by #5828
edmundmiller opened this issue Aug 30, 2024 · 8 comments · May be fixed by #7098
Open
Tracked by #5828

Snapshot content of version files instead of a hash #6505

edmundmiller opened this issue Aug 30, 2024 · 8 comments · May be fixed by #7098
Assignees

Comments

@edmundmiller
Copy link
Contributor

edmundmiller commented Aug 30, 2024

Currently we're snapshoting the versions.yml and getting a hash:

"versions": [
"versions.yml:md5,949da9c6297b613b50e24c421576f3f1"
]

                "versions": [
-                    "versions.yml:md5,949da9c6297b613b50e24c421576f3f1"
+                    "content": ["{ALE={ale=20180904}}"],
                ]
@edmundmiller
Copy link
Contributor Author

@nvnieuwk Pointed out that topic channels won't be hashable

#4517

@famosab
Copy link
Contributor

famosab commented Sep 3, 2024

How exactly would we assert for the versions then? I tried something like this:

{ assert snapshot(
             process.out,
             file(process.out.versions[0]).readLines()
             ).match()
}

We might need to adapt the linting or snapshot both md5 and the content. Otherwise this error occurs:

╭─ [✗] 1 Module Test Failed ───────────────────────────────────────────────────╮
│              ╷                               ╷                               │
│ Module name  │ File path                     │ Test message                  │
│╶─────────────┼───────────────────────────────┼──────────────────────────────╴│
│ wgsim        │ modules/nf-core/wgsim/tests/… │ versions not found in         │
│              │                               │ snapshot file                 │
│              ╵                               ╵                               │
╰──────────────────────────────────────────────────────────────────────────────╯
╭───────────────────────╮
│ LINT RESULTS SUMMARY  │
├───────────────────────┤
│ [✔]  50 Tests Passed  │
│ [!]   0 Test Warnings │
│ [✗]   1 Test Failed   │
╰───────────────────────╯
Error: Process completed with exit code 1.

Of course only if we do not assert process.out completely

{ assert snapshot(
             process.out.fastq[0][1].collect { file(it).name },
             file(process.out.versions[0]).readLines(),
             ).match()
}

Maybe there is a more elegant way?

@edmundmiller
Copy link
Contributor Author

We might need to adapt the linting or snapshot both md5 and the content. Otherwise this error occurs:

Luckily, we make the rules of the linter so we can ignore any errors like that and fix it right after we settle on a standard!

I like file(process.out.versions[0]).readLines() but let me see what the snapshot looks like.

Ideally I'd love for it to look like

"sarscov2 [fasta_gz] - paired-end sorted bam": {
        "content": [
            {
                "0": [
                    [
                        {
                            "id": "test",
                            "single_end": false
                        },
                        "test_ALEoutput.txt:md5,4abcbd60ae1dbf78138c97e5fed97f3e"
                    ]
                ],
                "1": [
                    "versions.yml:md5,949da9c6297b613b50e24c421576f3f1"
                ],
                "ale": [
                    [
                        {
                            "id": "test",
                            "single_end": false
                        },
                        "test_ALEoutput.txt:md5,4abcbd60ae1dbf78138c97e5fed97f3e"
                    ]
                ],
-                "versions": [
-                    "versions.yml:md5,949da9c6297b613b50e24c421576f3f1"
-                ]
+                "versions": {
+                    "ale": "20180904"
+                }
            }
        ],
        "meta": {
            "nf-test": "0.8.4",
            "nextflow": "23.10.1"
        },
        "timestamp": "2024-03-19T09:06:19.589167"
    },

@edmundmiller
Copy link
Contributor Author

Maybe there is a more elegant way?

I think we could write a lib function for this, either in this repo or the nf-test/utils plugin.

Two other things to consider:

  1. Migration to topic channels #4517 I think we need to see how these look when snapshotted.
  2. Renovate Workflow  #4306 and the automated bumping of conda dependancies coming soon™️. I was just thinking about this and I'm proposing maybe we move to just getting the versions through the stub workflow. This would:
    A) Make running them actually test something besides stub functionality
    B) Allow us to do nf-test test --tag stub --update-snapshot ... in CI to automatically bump the version snapshot, and then run the actual tests without the fear of bumping any real test outputs that aren't the version. I haven't found a better way around this without requesting specific snapshot updates in nf-test itself.

@SPPearce
Copy link
Contributor

SPPearce commented Sep 5, 2024

Maybe there is a more elegant way?

I think we could write a lib function for this, either in this repo or the nf-test/utils plugin.

Two other things to consider:

  1. Migration to topic channels #4517 I think we need to see how these look when snapshotted.
  2. Renovate Workflow  #4306 and the automated bumping of conda dependancies coming soon™️. I was just thinking about this and I'm proposing maybe we move to just getting the versions through the stub workflow. This would:
    A) Make running them actually test something besides stub functionality
    B) Allow us to do nf-test test --tag stub --update-snapshot ... in CI to automatically bump the version snapshot, and then run the actual tests without the fear of bumping any real test outputs that aren't the version. I haven't found a better way around this without requesting specific snapshot updates in nf-test itself.

I definitely support snapshoting the version information itself, think that is much clearer than the md5sum. The stub test could get the versions, it only needs to be once, that sounds reasonable. And every module should have a stub and stub test (eventually), so sounds good to me.

@edmundmiller
Copy link
Contributor Author

There's a function for yaml files in nf-test(that may have snuck in with 0.9.0) https://www.nf-test.com/docs/assertions/files/

@edmundmiller
Copy link
Contributor Author

Magic line is:

{ assert snapshot(path(process.out.versions.get(0)).yaml).match("versions") },

edmundmiller added a commit that referenced this issue Nov 7, 2024
* build: Add wave

* build: Set strategy to dockerfile, conda then container

* refactor: Remove container

* build: Add a repo to push to

* ci(wave): Add wave build

https://github.com/nodejs/docker-node/blob/3c4fa6daf06a4786d202f2f610351837806a0380/.github/workflows/build-test.yml#L29

* ci(wave): Switch to all_changed_files

* ci(wave): Only look for envronment.ymls

* dummy: Change env

* ci(wave): Remove raw format

* ci(wave): Try a bunch of different things at once

* ci(wave): Remove redundant fromJson and wrap in an array

* ci(wave): I have no idea what I'm doing

* ci(wave): Wrap it

* ci(wave): Found an example

https://github.com/tj-actions/changed-files/blob/main/.github/workflows/matrix-test.yml

* ci(wave): Maybe quotes?

* ci(wave): That'll do it

* ci(wave): Fix wave install

* ci(wave): Hard code an image

* ci(wave): Add secrets

* feat: Try a different files structure

* ci(wave): First stab at building singularity images

* fixup! feat: Try a different files structure

* ci(wave): Add profile to matrix

* ci(wave): Give up on fancy substitution

* ci(wave): Add await

Co-authored-by: ewels <[email protected]>

* ci(wave): Switch to quay

* test(wave): Add freeze and update build repo

* refactor(wave): What happens if I add a container?

* refactor(wave): Have both bowtie modules use the same env

For the sake of demonstration

* test: Cut out using wave on tests

* refactor: What happens if we use the singularity one?

* refactor: Keep container directives for offline download

seqeralabs/wave#323

* feat: Try new singularity OCI setting

nextflow-io/nextflow@f5362a7

* build: Update container name

Guess #4327 broke that

* chore: Bump wave-cli version

* ci: Install runc

* ci: Switch to singularityhub action

nextflow-io/nextflow#4543

* ci: Install new singularity manually

Why that action trys to build from source, idk.

* ci: Install dependancies for singularity

* ci: runc => crun

* ci: Fix cgroup error

https://blog.misharov.pro/2021-05-16/systemd-github-actions

* ci: That'll do it

* ci: Remove Dockerfile

We'll have a seperate action for this I think

* ci: Update name

* ci: Push to the correct repos

* ci: Remove OCI stuff

* ci: Need a full URL

* ci: Fix // in container name

* ci: Remove push

Build once, renovate should bump the images automagically

* build: Add containers back

* ci: Add cache repos

Idk what this does exactly

* ci: Change registry name to use _

Because "build" is a api end point on quay.io.

So `bowtie/build` doesn't work.

Other plus is this matches the conda env name.

* build: / => _ in container name

* Try ociAutoPull

* chore: Add renovate comments to samtools

Just to trigger wave build

* test: Add ociAutoPull to nf-test

* ci: Bump wave version

* chore: Bump containers with new wave version

Not sure why that's happening...

* build: Update to use commity.wave.seqera.io

* ci: Bump wave-cli to 1.4.1

* ci: Try apptainer

* ci: Remove build-repo to see what happens

* build: Bump Nextflow version requirement

* fix: Get rid of the environment name?

Maybe this will get the auto generated tag?

* ci: Bump action versions

* ci: Try name-strategy tagPrefix

seqeralabs/wave-cli@269df0e

* ci: Remove singularity build for now

* ci: Try imageSuffix

* ci: Try none

* ci: What is the bowtie container name

* ci: Remove --name-strategy

* style: Add back in container elvis operator

* ci: Remove cache repo

* Revert "build: Bump Nextflow version requirement"

This reverts commit 69e1ea5.

* Revert "test: Add ociAutoPull to nf-test"

This reverts commit 5a3d546.

* test(#6505): Snapshot the versions contents, not the hash

* ci(#6505): Update version snapshot after building containers

* test(#6505): Attempt a topic channel with tests

askimed/nf-test#258

* chore: Bump to 1.5.0

* fix: Remove shard and filter on test bumping

* build: Bump images to match environment

* ci: Fix nf-test setup

* ci: Remove snapshot bumping

* build: Fix containers in bowtie

---------

Co-authored-by: ewels <[email protected]>
edmundmiller added a commit that referenced this issue Nov 18, 2024
* ci: Attempt to split everything out

Don't hate me Adam

* 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"

* ci: Remove skipped tests

Idk what we're going to do about these...

* ci: Actually use the nf-test version

* ci: module => process

* ci: Clean up job names

* dummy: Make a change

* ci: Skip test.tap

* ci: Add fetch-depth

* ci: Clean up name

* ci: Lint everything

* ci: Get the job names clean

* ci: Add hide-progress on linting

* ci: Add psuedocode for conda-fail.yml

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

* test: Snapshot the versions contents, not the hash

#6505

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

Blocked by nf-core/tools#3140

* ci: Move conda skips out

* ci: Address a comment

* style: Move prettier and editorconfig to pre-commit

#4554 (comment)

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

* chore: Copy over conda skips

a004c86
2da71b7

* ci: only-changed => changed-since

* Add confirm-pass

* ci: dynamically set shards

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

* install nf-test

* fix install nf-test

* fix install nf-test

* ci: Remove dynamic number of shards, and combine process and workflow filters

* ci: Remove variable Nextflow versions

* test: Update bowtie versions

* ci: Fix indention

* ci: We're not testing multiple python versions

* style: Clean up job names

* build: Remove invisible characters?

* test(bowtie): Bump snapshots

* style: Remove stray comment

* ci: Add GPU tests in their own workflows

These will only get trigger on the paths listed

* ci(gpu): Clean up triggers and make a note about

how to make the includes smarter

* ci: pip install cryptography

* ci: Try removing path

* ci: Add fetch-depth and remove duplicate checkout

* test: Remove the second gpu profile

* ci: Add path back in

* ci(gpu): Remove changed-since

* ci: Fix missed dorny/paths-filter update

Co-authored-by: mashehu <[email protected]>

* Add changes from codereview

Co-authored-by: mashehu <[email protected]>

* style: Remove example TODO comments

https://github.com/nf-core/modules/pull/6286/files#r1846856369
Co-authored-by: mashehu <[email protected]>

* refactor: Try tags for GPU CI

askimed/nf-test#272

* style: Set NFT_WORKDIR as an ENV variable

* style: Run lsp format on a parabricks module

For science

* test: Add gpu tags to parabricks

* ci: Split tags out of matrix

* Revert "style: Run lsp format on a parabricks module"

This reverts commit d134e78.

* ci: Fix name

---------

Co-authored-by: Edmund Miller <[email protected]>
Co-authored-by: maxulysse <[email protected]>
Co-authored-by: mashehu <[email protected]>
@edmundmiller
Copy link
Contributor Author

askimed/nf-test#197

github-merge-queue bot pushed a commit that referenced this issue Dec 6, 2024
* test(#6505): Add pigz to versions.yml

* feat: Add pigz to trimgalore

nf-core/atacseq#394
FelixKrueger/TrimGalore#16 (comment)
https://github.com/FelixKrueger/TrimGalore/blob/master/CHANGELOG.md#version-060-release-on-1-mar-2019

* style: Run lsp format

* Update modules/nf-core/trimgalore/main.nf

---------

Co-authored-by: Simon Pearce <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants