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

Remove AMRFinderPlus DB update on each invocation #5232

Merged
merged 2 commits into from
Mar 20, 2024
Merged

Remove AMRFinderPlus DB update on each invocation #5232

merged 2 commits into from
Mar 20, 2024

Conversation

oschwengers
Copy link
Contributor

I removed the AMRFinderPlus DB update from the Bakta main.nf which seems to be invoked on each execution of Bakta. AMRFinderPlus db should be downloaded and installed automatically and transparently during the Bakta DB download via bakta_db download -> https://github.com/oschwengers/bakta/blob/d6443639958750c3bece5822e84978271d1a4dc7/bakta/db.py#L247

Maybe I'm missing something, but if not, this would be a waste of bandwidth and significantly increase wall clock run times.

@oschwengers
Remove AMRFinderPlus DB update on each invocation
70bb1f9 
Remove the AMRFinderPlus DB update which is invoked on each Bakta execution, nevertheless, it should be downloaded and installed during the DB download via bakta_db download.
@jfy133
Copy link
Member

jfy133 commented Mar 19, 2024

I think it was @jasmezz who added this in because the module wasn't working without it IIRC ?

@jasmezz
Copy link
Contributor

jasmezz commented Mar 19, 2024 •
edited
Loading

Yes, the amrfinder update line comes from here (see also the original bug report in the PR description).
In short:

  • The user had bakta crashing using conda without the amrfinder update line, which I could reproduce. So I wanted to add it for conda-only executions to save resources for non-conda usage.
  • For some reason, bakta started to crash then for the singularity/docker containers as well without the line. Thus, I added it for all.

And now in your PR it seems to work again without the line! I am really confused about this flakiness.
Did you test your changes for conda locally?

@oschwengers
Copy link
Contributor Author

This is really interesting/odd. For Bakta we often see issues with AMRFinderPlus that we cannot reproduce or don't know what is causing these.

No, I haven't tested it locally. I just recognized this line and wanted to throw in my 0.02$ as a tool developer that, in principle, this should work w/o this extra invocation of amrfinder_update.

@jasmezz
Copy link
Contributor

jasmezz commented Mar 19, 2024

That's alright, thank you! I'd prefer to approve your PR of course. I am investigating the issue locally with conda. Stay tuned, this might take a while.

@jasmezz
Copy link
Contributor

jasmezz commented Mar 19, 2024

I cannot find any errors with removing the line on any of conda/singularity/docker. So let's go ahead with this PR. 👍
I did not really find the source from the earlier reported issue which lead to including the line in the first place. Hopefully I get better clues if it ever happens again (which will of course not happen!).

@jasmezz
Copy link
Contributor

jasmezz commented Mar 20, 2024

Go ahead and merge @oschwengers 🚀 I need the module asap, so will need to do merge myself if you won't ;)

@jasmezz jasmezz added this pull request to the merge queue Mar 20, 2024
@oschwengers
Copy link
Contributor Author

Thanks a lot @jasmezz! I'd love to merge this but, unfortunately, I do not have write permissions.

Merged via the queue into nf-core:master with commit 9d0f89b Mar 20, 2024
10 checks passed
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
@oschwengers @jasmezz @tucano
Remove AMRFinderPlus DB update on each invocation (nf-core#5232)
5b272e2 
Remove the AMRFinderPlus DB update which is invoked on each Bakta execution, nevertheless, it should be downloaded and installed during the DB download via bakta_db download.

Co-authored-by: Jasmin Frangenberg <[email protected]>
jennylsmith pushed a commit to RSC-RP/modules that referenced this pull request Mar 20, 2024
@oschwengers @jasmezz @jennylsmith
Remove AMRFinderPlus DB update on each invocation (nf-core#5232)
129830b 
Remove the AMRFinderPlus DB update which is invoked on each Bakta execution, nevertheless, it should be downloaded and installed during the DB download via bakta_db download.

Co-authored-by: Jasmin Frangenberg <[email protected]>
alexnater pushed a commit to alexnater/nf-core-modules that referenced this pull request Mar 21, 2024
@oschwengers @jasmezz
Remove AMRFinderPlus DB update on each invocation (nf-core#5232)
797a017 
Remove the AMRFinderPlus DB update which is invoked on each Bakta execution, nevertheless, it should be downloaded and installed during the DB download via bakta_db download.

Co-authored-by: Jasmin Frangenberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasmezz jasmezz jasmezz approved these changes

@rpetit3 rpetit3 Awaiting requested review from rpetit3

@jfy133 jfy133 Awaiting requested review from jfy133

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

Successfully merging this pull request may close these issues.
3 participants
@oschwengers @jfy133 @jasmezz