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

Add module kleborate #711

Merged
merged 9 commits into from
Sep 21, 2021
Merged

Add module kleborate #711

merged 9 commits into from
Sep 21, 2021

Conversation

abhi18av
Copy link
Member

@abhi18av abhi18av commented Sep 16, 2021

PR checklist

Closes #XXX

  • 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 module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Emit the <SOFTWARE>.version.txt file.
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • PROFILE=docker pytest --tag <MODULE> --symlink --keep-workflow-wd
    • PROFILE=singularity pytest --tag <MODULE> --symlink --keep-workflow-wd
    • PROFILE=conda pytest --tag <MODULE> --symlink --keep-workflow-wd

@abhi18av
Copy link
Member Author

abhi18av commented Sep 16, 2021

@rpetit3 , regarding this one I am running into a bit of an issue while writing tests.

It seems that the path expected by makeblastdb isn't writeable. I've narrowed it down to the use of https://github.com/katholt/Kleborate/blob/eae8e794a77bbdf24a7c111ad8322d209daab742/kleborate/__main__.py#L366

Also, I noticed that you've recently patched the bioconda package for kleborate, do you reckon that we should add a new parameter for allowing this data to be in a different position? I'm not sure if it's possible to alter permissions using the build.sh on the bioconda recipe.


[c4/c242ef] process > test_kleborate:KLEBORATE (test) [100%] 1 of 1, failed: 1 ✘
Error executing process > 'test_kleborate:KLEBORATE (test)'

Caused by:
  Process `test_kleborate:KLEBORATE (test)` terminated with an error exit status (1)

Command executed:

  kleborate \
       \
      --outfile test.results.txt \
      --assemblies  *.fasta
  
  echo $(kleborate -v 2>&1) | sed 's/kleborate //;' > kleborate.version.txt

Command exit status:
  1

Command output:
  strain        species ST      virulence_score Yersiniabactin  YbST    Colibactin      CbST    Aerobactin      AbST    SalmochelinSmST     RmpADC  RmST    rmpA2   wzi     K_locus

Command error:
  Error: NCBI C++ Exception:
      T0 "/opt/conda/conda-bld/blast_1626166529662/work/blast/c++/src/objtools/blast/seqdb_writer/build_db.cpp", line 1053: Error: (CMultisourceException::eInvalid) BLASTDB::ncbi::CBuildDatabase::CreateDirectories() - You do not have write permissions on 'data'
  
  Traceback (most recent call last):
    File "/usr/local/bin/kleborate", line 10, in <module>
      sys.exit(main())
    File "/usr/local/lib/python3.8/site-packages/kleborate/__main__.py", line 61, in main
      results.update(get_all_virulence_results(data_folder, contigs, args))
    File "/usr/local/lib/python3.8/site-packages/kleborate/__main__.py", line 477, in get_all_virulence_results
      virulence_results = [get_ybt_mlst_results(data_folder, contigs, args),
    File "/usr/local/lib/python3.8/site-packages/kleborate/__main__.py", line 504, in get_ybt_mlst_results
      return get_virulence_cluster_results(data_folder, contigs, 'ybt_alleles.fasta',
    File "/usr/local/lib/python3.8/site-packages/kleborate/__main__.py", line 460, in get_virulence_cluster_results
      mlst_blast(seqs, database, 'yes', [contigs], min_cov=args.min_coverage,
    File "/usr/local/lib/python3.8/site-packages/kleborate/mlstBLAST.py", line 46, in mlst_blast
      hits = run_blastn(seqs, contigs, min_spurious_cov, min_spurious_ident)
    File "/usr/local/lib/python3.8/site-packages/kleborate/blastn.py", line 22, in run_blastn
      build_blast_database_if_needed(db)
    File "/usr/local/lib/python3.8/site-packages/kleborate/blastn.py", line 138, in build_blast_database_if_needed
      subprocess.check_call('makeblastdb -dbtype nucl -in ' + seqs, stdout=devnull,
    File "/usr/local/lib/python3.8/subprocess.py", line 364, in check_call
      raise CalledProcessError(retcode, cmd)
  subprocess.CalledProcessError: Command 'makeblastdb -dbtype nucl -in /usr/local/lib/python3.8/site-packages/kleborate/data/ybt_alleles.fasta' returned non-zero exit status 255.

Work dir:
  /tmp/pytest_workflow_3la4lze1/kleborate/work/c4/c242efe06908cd9dfb240471a9d505

Tip: you can try to figure out what's wrong by changing to the process work dir and showing the script file named `.command.sh`

@abhi18av abhi18av mentioned this pull request Sep 16, 2021
80 tasks
@rpetit3
Copy link
Contributor

rpetit3 commented Sep 16, 2021

@abhi18av is the test with Conda, Docker, or Singularity? I'll probably be able to play around with this tomorrow

@abhi18av
Copy link
Member Author

The test was run using the docker profile

PROFILE=docker pytest --tag kleborate --symlink --keep-workflow-wd

@rpetit3
Copy link
Contributor

rpetit3 commented Sep 17, 2021

This one is going to be problematic because you cannot change the path of the database at run time (e.g. --db /data/kleborate).

Let me play around with the bioconda recipe to see if we can make it changed by an environmental variable (${KLEBORATE_DB})

@rpetit3
Copy link
Contributor

rpetit3 commented Sep 17, 2021

@abhi18av I submitted a PR to kleborate klebgenomics/Kleborate#59

I might be able to rebuild the bioconda recipe to include the blast indexes as a temporary patch

@rpetit3
Copy link
Contributor

rpetit3 commented Sep 17, 2021

Also submitted recipe update to bioconda to build that blast databases bioconda/bioconda-recipes#30582

@rpetit3
Copy link
Contributor

rpetit3 commented Sep 17, 2021

Ok, I have a recipe update that pre-builds the BLAST databases. Hopefully this should allow you to continue

@abhi18av
Copy link
Member Author

Thanks for the quick efforts on this @rpetit3!

I think before giving finishing touches to this PR, it's best to await the merge for bioconda/bioconda-recipes#30582 , right?

@rpetit3
Copy link
Contributor

rpetit3 commented Sep 21, 2021

The new build was merged recently so it might take a little while for the containers to be built

@abhi18av abhi18av self-assigned this Sep 21, 2021
@abhi18av
Copy link
Member Author

@rpetit3 , thanks to the new build for kleborate I was able to get it moving fowards with the docker tests, however the singularity one is still failing. I don't have much experience with singularity (nor do I have it setup locally) - in case you have it handy, could you please give it a look?

If not, no worries, I'll give it a read and be up and running with singularity on a linux machine by tomorrow.

@abhi18av abhi18av added the new module Adding a new module label Sep 21, 2021
@rpetit3
Copy link
Contributor

rpetit3 commented Sep 21, 2021

@abhi18av You are all set, now we just wait for the Galaxy team to biuld the singularity image.

Here's the process, for Bioconda recipes.

  1. Recipe is merged into Bioconda
  2. Docker contianer is built for merged recipe (quay.io/biocontianers, usually prety quick)
  3. Singularity image is built from quay.io/biocontainer (hosted by Galaxy, takes a bit longer to become available)

We're on step 3 at the moment

modules/kleborate/main.nf Outdated Show resolved Hide resolved
modules/kleborate/main.nf Outdated Show resolved Hide resolved
modules/kleborate/main.nf Outdated Show resolved Hide resolved
@drpatelh drpatelh marked this pull request as ready for review September 21, 2021 19:43
@drpatelh
Copy link
Member

Thanks @abhi18av @rpetit3 ! Will merge this in and hopefully the Singularity container will be pushed soon too. If not, we can raise an issue. They are generally very good.

@drpatelh drpatelh merged commit 77a2895 into nf-core:master Sep 21, 2021
@rpetit3
Copy link
Contributor

rpetit3 commented Sep 21, 2021

Thank you @drpatelh ! many more in bound, looking forward to taking full advantage of nf-core/modules

@drpatelh
Copy link
Member

Awesome! Thank you for reviewing too 😎 You know the ropes now.

@abhi18av abhi18av deleted the abhinav/kleborate branch September 22, 2021 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants