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

Adds --krona_db parameter to point at a local copy of the db needed for Krona. Suggested in Issue #404. #497

Merged
merged 10 commits into from
Sep 2, 2023
Merged

Adds --krona_db parameter to point at a local copy of the db needed for Krona. Suggested in Issue #404. #497

merged 10 commits into from
Sep 2, 2023

Conversation

willros
Copy link
Contributor

@willros willros commented Aug 29, 2023 •
edited by jfy133
Loading

closes #404

The user can now point at a local copy of the the taxonomy.tab file needed for running Krona.

PR checklist

  • [x ] 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/mag branch on the nf-core/test-datasets repository.
  • [x ] Make sure your code lints (nf-core lint).
  • [ x] Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • [x ] Usage Documentation in docs/usage.md is updated.
  • [ x] Output Documentation in docs/output.md is updated.
  • [ x] CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@github-actions
Copy link

github-actions bot commented Aug 29, 2023 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 929b54d

+| ✅ 156 tests passed       |+
#| ❔   1 tests were ignored |#
!| ❗   1 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your preferred methods description, e.g. add publication citation for this pipeline

❔ Tests ignored:

  • files_unchanged - File ignored due to lint config: lib/NfcoreTemplate.groovy

✅ Tests passed:

Run details

  • nf-core/tools version 2.9
  • Run at 2023-09-01 11:58:46

d4straub
d4straub reviewed Aug 29, 2023
View reviewed changes
Copy link
Collaborator

@d4straub d4straub 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 to me.
Is there a way to include that into tests? Potentially with files from the test of the nf-core module ktimporttaxonomy?

modules/local/krona.nf
Copy link
Collaborator

Choose a reason for hiding this comment

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

Eventually it might be good to go to https://nf-co.re/modules/krona_ktimporttaxonomy (which seems to do the same), but I agree that --krona_db parameter is in any case a good addition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will look into how the test is implemented in https://nf-co.re/modules/krona_ktimporttaxonomy.

jfy133
jfy133 reviewed Aug 29, 2023
View reviewed changes
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

Overall LGTM thanks @willros !

Just a question about the implementation with the file rather than a directory (containing the file). If there is an issue with symlinks 👍 I have an alternative suggestion

willros and others added 3 commits August 30, 2023 13:48
@willros @d4straub
Update nextflow_schema.json
c523702 
Co-authored-by: Daniel Straub <[email protected]>
@willros @jfy133
Update CHANGELOG.md
9084d68 
Co-authored-by: James A. Fellows Yates <[email protected]>
@willros
removed toRealPath
8bb89d8
@jfy133
Copy link
Member

jfy133 commented Aug 31, 2023

Just running a little test, and then can merge

@jfy133
Copy link
Member

jfy133 commented Aug 31, 2023

Hm, auto download didn't work....

james@bionb103:~/git/nf-core/mag/tests/work/5e/93d9d90d1c6bd699d78b6c9d1e4f58 (local-krona-db)$ cat .command.sh 
#!/bin/bash -euo pipefail
ktImportTaxonomy "results.krona" -tax null

cat <<-END_VERSIONS > versions.yml
"NFCORE_MAG:MAG:KRONA":
    ktImportTaxonomy: $(ktImportTaxonomy 2>&1 | sed -n '/KronaTools /p' | sed 's/^.*KronaTools //; s/ - ktImportTaxonomy.*//')
END_VERSIONS

@jfy133 jfy133 self-requested a review August 31, 2023 11:00
jfy133
jfy133 requested changes Aug 31, 2023
View reviewed changes
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

The KRONA module received null rather than the .getParent() output for autodownloading...

@jfy133
Copy link
Member

jfy133 commented Aug 31, 2023 •
edited
Loading

Ah I think this is because what is literally given to the module is taxonomy.tab not the whole path itself, so getParent() returns null as there is no parent of the string provided - so I get the purpose now of your toRealPath thing 😅.

@jfy133
Copy link
Member

jfy133 commented Aug 31, 2023

After thinking about it, my suggestion is to follow what is done in various bwa and Bowtie2 modules, i.e.,

https://github.com/nf-core/modules/blob/868d4c3dc7a3db39d36184173e4fe3484499396e/modules/nf-core/bwa/mem/main.nf#L28

Supply the whole directory containing the taxonomy files, then search for tab, and resolve the path that way.

@willros
Copy link
Contributor Author

willros commented Sep 1, 2023 •
edited
Loading

Looking deeper into the krona module of nf-core, why not use the process implemented there? https://github.com/nf-core/modules/blob/868d4c3dc7a3db39d36184173e4fe3484499396e/modules/nf-core/krona/ktimporttaxonomy/main.nf

Anyways, I implemented the same solution in the local process for this pipeline.

jfy133
jfy133 approved these changes Sep 2, 2023
View reviewed changes
Copy link
Member

@jfy133 jfy133 left a comment

Choose a reason for hiding this comment

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

All works for me now, thank you very much @willros this is VERY helpful (as I'm having to do a manual 'full test' of the AWS runs on my offline cluster 😅, which obiovusly wasn't working before...)

@jfy133
Copy link
Member

jfy133 commented Sep 2, 2023

Looking deeper into the krona module of nf-core, why not use the process implemented there? https://github.com/nf-core/modules/blob/868d4c3dc7a3db39d36184173e4fe3484499396e/modules/nf-core/krona/ktimporttaxonomy/main.nf

Anyways, I implemented the same solution in the local process for this pipeline.

We also need to do a big pass through the pipeline replaceing local modules with now-official moduels... but this will requrie a big overhaul in the future! So thanks again for contribution and we hope you will keep contributing!

@jfy133 jfy133 merged commit 21c9716 into nf-core:dev Sep 2, 2023
@willros willros deleted the local-krona-db branch September 4, 2023 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d4straub d4straub d4straub left review comments

@jfy133 jfy133 jfy133 approved these changes

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
@willros @jfy133 @d4straub