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

Updated tests and fusion containers and conda versions, general confi… #550

Open
wants to merge 19 commits into
base: dev
Choose a base branch
from

Conversation

nschcolnicov
Copy link

…g, and ci.yml

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/rnafusion branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Check for unexpected warnings in debug mode (nextflow run . -profile debug,test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

Copy link

github-actions bot commented Nov 28, 2024

nf-core pipelines lint overall result: Passed ✅

Posted for pipeline commit 474a89e

+| ✅ 222 tests passed       |+
#| ❔   2 tests were ignored |#

❔ Tests ignored:

  • files_unchanged - File ignored due to lint config: .github/CONTRIBUTING.md
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md

✅ Tests passed:

Run details

  • nf-core/tools version 3.0.2
  • Run at 2024-12-03 20:07:05

@nschcolnicov
Copy link
Author

@rannick This is a POC for some changes in ci.yml, so that it doesn't just run the pipeline and it also checks the output it generates. Since I don't have access to COSMIC database, I added a section in the ci.yml file that will only run the test profiles that require this access, only when the nextflow secrets are not null. Let me know what you think!

@atrigila
Copy link

Perhaps apart from the -stub we could add a test with the -no-cosmic flag introduced in this PR: #547

@nschcolnicov
Copy link
Author

Perhaps apart from the -stub we could add a test with the -no-cosmic flag introduced in this PR: #547

Sounds good, lets wait until the other PR gets merged before moving on with this one then

@rannick
Copy link
Collaborator

rannick commented Nov 29, 2024

Exactly, I think the tests should run with the --no_cosmic option

@rannick
Copy link
Collaborator

rannick commented Nov 29, 2024

#547 is merged

@nschcolnicov nschcolnicov marked this pull request as ready for review December 3, 2024 14:47
@nschcolnicov
Copy link
Author

nschcolnicov commented Dec 3, 2024

@atrigila @rannick Using latest-stable instead of latest-everything so that CI tests don't fail

config_profile_description = 'Minimal test dataset to check pipeline function'

// Input data
build_references = true
Copy link

Choose a reason for hiding this comment

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

shouldn't we have the --no_cosmic flag in test_build?

Copy link
Author

Choose a reason for hiding this comment

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

Wasn't sure what could be considered "standard" for running the test profile, but I think you are right, i'll add it

Copy link

Choose a reason for hiding this comment

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

I am not sure if we are supposed to run this test in CI or somewhere else? If we are running the full --build_references, perhaps we should add the --all flag? so that all references are downloaded

Copy link

Choose a reason for hiding this comment

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

I imagine test full does not run --build_references. Therefore, the path to these references needs to be given (probably from an s3 bucket). I know we don't have it yet, but perhaps we can add a TODO for the future.

@@ -2,8 +2,8 @@ process FUSIONINSPECTOR {
tag "$meta.id"
label 'process_high'

conda "bioconda::dfam=3.7 bioconda::hmmer=3.4 bioconda::star-fusion=1.13.0 bioconda::trinity=2.15.1 bioconda::samtools=1.19.2 bioconda::star=2.7.11b"
container 'docker.io/trinityctat/starfusion:1.13.0'
conda "bioconda::dfam=3.7 bioconda::hmmer=3.4 bioconda::star-fusion=1.7.0 bioconda::trinity=2.15.2 bioconda::samtools=1.21 bioconda::star=2.7.11b"
Copy link

Choose a reason for hiding this comment

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

the conda declaration could be with the environmentl.yml as we do in nf-core modules.

you can get it directly from the seqera containers :) they already provide it

@@ -1,8 +1,8 @@
process STARFUSION_BUILD {
tag 'star-fusion'

conda "bioconda::dfam=3.7 bioconda::hmmer=3.4 bioconda::star-fusion=1.13.0 bioconda::trinity=2.15.1 bioconda::samtools=1.19.2 bioconda::star=2.7.11b"
container "docker.io/trinityctat/starfusion:1.13.0"
conda "bioconda::dfam=3.7 bioconda::hmmer=3.4 bioconda::star-fusion=1.7.0 bioconda::trinity=2.15.2 bioconda::samtools=1.21 bioconda::star=2.7.11b"
Copy link

Choose a reason for hiding this comment

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

same thing about conda declaration, see above.

Copy link

Choose a reason for hiding this comment

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

I think you'll need to run this test as some content was not being generated and I fixed those bugs in previous PRs.

Comment on lines 61 to 65
"Homo_sapiens.GRCh38.102_rrna_intervals.gtf.bed:md5,d41d8cd98f00b204e9800998ecf8427e",
"Homo_sapiens.GRCh38.102.dna.primary_assembly.dict:md5,d41d8cd98f00b204e9800998ecf8427e",
"Homo_sapiens.GRCh38.102.gff3:md5,d41d8cd98f00b204e9800998ecf8427e",
"cytobands_hg38_GRCh38_v2.4.0.tsv:md5,d41d8cd98f00b204e9800998ecf8427e",
"protein_domains_hg38_GRCh38_v2.4.0.gff3:md5,d41d8cd98f00b204e9800998ecf8427e",
Copy link

Choose a reason for hiding this comment

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

I don't know if it makes sense to snapshot md5sum as they are all the same because they are empty...

Copy link
Author

Choose a reason for hiding this comment

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

I see what you mean and how this could get confusing after we update the test profiles. I'll remove it

Comment on lines 12 to 17
when {
params {
outdir = "$outputDir"
no_cosmic = true
}
}
Copy link

Choose a reason for hiding this comment

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

I would pass the params in the profile, to be more clear, because you now have 2 sources of params:

  • the test_build profile.config itself
  • the nf-test
    I would expect the nf-test to run exactly as the test profile, so I would add all necessary parameters in the config.
    This comment applies to all nf-tests

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense!

// All stable path name, with a relative path
stable_name,
// All files with stable contents
stable_path
Copy link

Choose a reason for hiding this comment

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

Consider removing

// All stable path name, with a relative path
stable_name,
// All files with stable contents
stable_path
Copy link

Choose a reason for hiding this comment

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

Consider removing

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 this pull request may close these issues.

3 participants