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.

Sign up for GitHub

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

Jump to bottom

Add README to modules build with Docker #4935

Merged
merged 10 commits into from Mar 20, 2024
Merged

Add README to modules build with Docker #4935

merged 10 commits into from Mar 20, 2024

Conversation

maxulysse
Copy link
Member

close #4934

@maxulysse maxulysse marked this pull request as ready for review February 16, 2024 12:28
@maxulysse
Copy link
Member Author

@nf-core-bot fix linting pretty please 🙏

@github-actions GitHub Actions
Copy link

Nothing for me to do here! 🤷
This is probably because the linting errors come from nf-core lint and have to be fixed manually (or with nf-core lint --fix).

mahesh-panchal
mahesh-panchal reviewed Feb 16, 2024
View reviewed changes
Copy link
Member

@mahesh-panchal mahesh-panchal left a comment

Choose a reason for hiding this comment

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

The Dockerfiles are duplicates no? Why not put the GATK Dockerfile in the parent so we know it's common to all?

@maxulysse
Copy link
Member Author

The Dockerfiles are duplicates no? Why not put the GATK Dockerfile in the parent so we know it's common to all?

because we use this Dockerfile for just 4 gatk modules over 59, for all the rest we just use biocontainers as usual

@mahesh-panchal
Copy link
Member

The Dockerfiles are duplicates no? Why not put the GATK Dockerfile in the parent so we know it's common to all?

because we use this Dockerfile for just 4 gatk modules over 59, for all the rest we just use biocontainers as usual

Ah, but there's still the issue the file could become out of sync between copies. Can they be symlinks instead?

@maxulysse
Copy link
Member Author

I'll make sure to check that whenever there's an update on the modules, and given that @matthdsm is actually making that change right now in #4900
I actually already used gatk 4.5, so let's wait for #4900 before merging this one please

@matthdsm
Copy link
Contributor

I'm not sure I like just copy pasting a dockerfile from another repo. I'd much rather just refer to the original file through a permalink or whatever.

Ideally we're able to fix the bioconda recipe/biocontainer and we can leave the bloated broad dockerfile behind

@edmundmiller
Copy link
Contributor

Yeah I'm against the Dockerfile copy pasting, they're probably just going to bitrot.

Could we build them with Spack and wave?

@edmundmiller
Copy link
Contributor

edmundmiller commented Feb 16, 2024 •
edited

https://github.com/spack/spack/blob/develop/var/spack/repos/builtin/packages/gatk/package.py

Yeah maybe we just toss a spack.yml in there?

@matthdsm
Copy link
Contributor

Not good enough, still missing the necessary custom python pkgs

@edmundmiller edmundmiller mentioned this pull request Feb 16, 2024
Draft
@maxulysse maxulysse changed the title Add Dockerfile to modules build with Docker Add README to modules build with Docker Feb 17, 2024
@maxulysse
Copy link
Member Author

Replaced the Dockerfiles with self-explanatory READMEs

matthdsm
matthdsm approved these changes Feb 17, 2024
View reviewed changes
Copy link
Contributor

@matthdsm matthdsm left a comment

Choose a reason for hiding this comment

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

Looks good!

@maxulysse
Copy link
Member Author

germlinecnvcaller tests are failing, did you fix theses in your PR?

@matthdsm
Copy link
Contributor

Yep, tests are passing in my PR

@maxulysse
Copy link
Member Author

Ok, then let's wait until yours is merged to merge this one

@edmundmiller
Copy link
Contributor

edmundmiller commented Feb 17, 2024 •
edited

Got wave working with Dockerfiles FYI https://github.com/nf-core/modules/actions/runs/7944492424/job/21690068698 (spack not so much) if there's any interest in being Guinea pigs.

@maxulysse maxulysse mentioned this pull request Feb 19, 2024
Merged
14 tasks
@maxulysse maxulysse merged commit 60a7dba into nf-core:master Mar 20, 2024
16 of 20 checks passed
@maxulysse maxulysse deleted the Dockerfile_environment_google_gatk branch March 20, 2024 08:32
vlebars pushed a commit to vlebars/modules that referenced this pull request Mar 20, 2024
@maxulysse @matthdsm
Add README to modules build with Docker (nf-core#4935)
933116d 
* add Dockerfile

* remove environment.yml

* EC lint

* add comment

* remove Dockerfile

* add README

---------

Co-authored-by: Matthias De Smet <11850640+matthdsm@users.noreply.github.com>
jvfe added a commit to jvfe/modules that referenced this pull request Mar 20, 2024
@jvfe
Merge branch 'master' into nf-test_islandpath
2be1844 
* master:
  Added contrast limited adaptive histogram equalization module (nf-core#5268)
  Leviosam2 index (nf-core#5316)
  Add subworkflow mapAD (nf-core#5239)
  Remove AMRFinderPlus DB update on each invocation (nf-core#5232)
  Revert "add paths in output directive in cellranger cout module" (nf-core#5306)
  Revert "update kallistobustools count output list" (nf-core#5307)
  Igv reports (nf-core#5263)
  Remove unnecessary .view() in subworkflows/nf-core/vcf_phase_shapeit5 & tests/modules/nf-core/shapeit5/ligate/main.nf (nf-core#5280)
  Add wittyer as module (nf-core#5171)
  Bamstats (nf-core#4474)
  gatk4_asereadcounter add updated meta and nf-tests (nf-core#5164)
  4557 new module kaijumergeoutputs + stub Kraken2/Kraken2 (nf-core#5249)
  add cram/index support to bwamem2 (nf-core#5248)
  Add README to modules build with Docker (nf-core#4935)
  update paths for VEP (nf-core#5281)
  nf-test bases2fastq (nf-core#5272)
  Add module seqfu/stats (nf-core#5275)
jvfe added a commit to jvfe/modules that referenced this pull request Mar 20, 2024
@jvfe
Merge branch 'master' into nf-test_phispy
294a9c0 
* master:
  Added contrast limited adaptive histogram equalization module (nf-core#5268)
  Leviosam2 index (nf-core#5316)
  Add subworkflow mapAD (nf-core#5239)
  Remove AMRFinderPlus DB update on each invocation (nf-core#5232)
  Revert "add paths in output directive in cellranger cout module" (nf-core#5306)
  Revert "update kallistobustools count output list" (nf-core#5307)
  Igv reports (nf-core#5263)
  Remove unnecessary .view() in subworkflows/nf-core/vcf_phase_shapeit5 & tests/modules/nf-core/shapeit5/ligate/main.nf (nf-core#5280)
  Add wittyer as module (nf-core#5171)
  Bamstats (nf-core#4474)
  gatk4_asereadcounter add updated meta and nf-tests (nf-core#5164)
  4557 new module kaijumergeoutputs + stub Kraken2/Kraken2 (nf-core#5249)
  add cram/index support to bwamem2 (nf-core#5248)
  Add README to modules build with Docker (nf-core#4935)
  update paths for VEP (nf-core#5281)
  nf-test bases2fastq (nf-core#5272)
  Add module seqfu/stats (nf-core#5275)
  Added gt/gff3validator (nf-core#4718)
  Added liftoff (nf-core#5209)
tucano pushed a commit to tucano/modules that referenced this pull request Mar 20, 2024
@maxulysse @matthdsm @tucano
Add README to modules build with Docker (nf-core#4935)
e9ad5b0 
* add Dockerfile

* remove environment.yml

* EC lint

* add comment

* remove Dockerfile

* add README

---------

Co-authored-by: Matthias De Smet <11850640+matthdsm@users.noreply.github.com>
jennylsmith pushed a commit to RSC-RP/modules that referenced this pull request Mar 20, 2024
@maxulysse @matthdsm @jennylsmith
Add README to modules build with Docker (nf-core#4935)
e4cc8d4 
* add Dockerfile

* remove environment.yml

* EC lint

* add comment

* remove Dockerfile

* add README

---------

Co-authored-by: Matthias De Smet <11850640+matthdsm@users.noreply.github.com>
alexnater pushed a commit to alexnater/nf-core-modules that referenced this pull request Mar 21, 2024
@maxulysse @matthdsm
Add README to modules build with Docker (nf-core#4935)
5671e5f 
* add Dockerfile

* remove environment.yml

* EC lint

* add comment

* remove Dockerfile

* add README

---------

Co-authored-by: Matthias De Smet <11850640+matthdsm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mahesh-panchal mahesh-panchal mahesh-panchal left review comments

@edmundmiller edmundmiller edmundmiller left review comments

@matthdsm matthdsm matthdsm approved these changes

@nvnieuwk nvnieuwk Awaiting requested review from nvnieuwk nvnieuwk is a code owner

@FriederikeHanssen FriederikeHanssen Awaiting requested review from FriederikeHanssen FriederikeHanssen is a code owner

@abhi18av abhi18av Awaiting requested review from abhi18av abhi18av is a code owner

@ramprasadn ramprasadn Awaiting requested review from ramprasadn ramprasadn is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dockerfile or container image provenance information missing from deepvariant module
4 participants
@maxulysse @mahesh-panchal @matthdsm @edmundmiller