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

Fix kraken db loading fromPath #673

Merged
merged 7 commits into from
Feb 3, 2021
Merged

Fix kraken db loading fromPath #673

merged 7 commits into from
Feb 3, 2021

Conversation

maxibor
Copy link
Member

@maxibor maxibor commented Feb 1, 2021

Kraken database loading fixed to fromPath method for channel creation

PR checklist

  • This comment contains a description of changes (with reason).
  • Make sure your code lints (nf-core lint .).
  • Ensure the test suite passes (nextflow run . -profile test,docker).
  • CHANGELOG.md is updated.

main.nf Show resolved Hide resolved
@jfy133 jfy133 self-requested a review February 1, 2021 20:28
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.

CI fail :(

Error executing process > 'decomp_kraken'
Caused by:
  Process `decomp_kraken` terminated with an error exit status (1)
Command executed:
  tar xvzf eager_test.tar.gz
  mkdir -p eager_test
  mv *.k2d eager_test
Command exit status:
  1
Command output:
  eager_test/
  eager_test/opts.k2d
  eager_test/seqid2taxid.map
  eager_test/taxo.k2d
  eager_test/hash.k2d
Command error:
  mv: cannot stat '*.k2d': No such file or directory

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.

See comment above about CI fail

main.nf Outdated Show resolved Hide resolved
@apeltzer
Copy link
Member

apeltzer commented Feb 2, 2021

Guessing from the conversation still some fixes required no ?

@jfy133 jfy133 self-requested a review February 3, 2021 07:42
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.

This in principle to me now looks good. CI tests do pass (the one fail is the flakiness on the BAM filtering thing).

@maxibor please decide if you want to change fromPath to file() (see Harshil's comment), and then proceed with merging!

@maxibor
Copy link
Member Author

maxibor commented Feb 3, 2021

As I can't see file() anywhere in the doc for channel creation, I think for my future self, it's better to keep fromPath() to understand what I did

@jfy133
Copy link
Member

jfy133 commented Feb 3, 2021

As I can't see file() anywhere in the doc for channel creation, I think for my future self, it's better to keep fromPath() to understand what I did

Feel free to merge! <- SCRAP THAT!

See Slack - only one sample was run, needs a .collect() somewhere

@apeltzer
Copy link
Member

apeltzer commented Feb 3, 2021

Yeah add either file() or an additional collect() in the respective process to get past this 👍🏻

@maxibor
Copy link
Member Author

maxibor commented Feb 3, 2021

Went with .first() 😉
It has the same effect: transforms a queue channel to a value channel (There is anyway only a single item in this channel, the kraken db directory)

@apeltzer
Copy link
Member

apeltzer commented Feb 3, 2021

See slack - solves the issue 👍🏻

@apeltzer apeltzer merged commit cc3ff23 into nf-core:dev Feb 3, 2021
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.

4 participants