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

Update docs #594

Merged
merged 4 commits into from
Aug 14, 2024
Merged

Update docs #594

merged 4 commits into from
Aug 14, 2024

Conversation

ramprasadn
Copy link
Collaborator

PR checklist

Address #593

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

@github-actions GitHub Actions
Copy link

This PR is against the master branch ❌

  • Do not close this PR
  • Click Edit and change the base to dev
  • This CI test will remain failed until you push a new commit

Hi @ramprasadn,

It looks like this pull-request is has been made against the nf-core/raredisease master branch.
The master branch on nf-core repositories should always contain code from the latest release.
Because of this, PRs to master are only allowed if they come from the nf-core/raredisease dev branch.

You do not need to close this PR, you can change the target branch to dev by clicking the "Edit" button at the top of this page.
Note that even after this, the test will continue to show as failing until you push a new commit.

Thanks again for your contribution!

@ramprasadn ramprasadn changed the base branch from master to dev August 13, 2024 14:42
@github-actions GitHub Actions
Copy link

github-actions bot commented Aug 13, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 01a7b42

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

❗ Test warnings:

  • pipeline_todos - TODO string in awsfulltest.yml: You can customise AWS full pipeline tests as required

❔ Tests ignored:

  • files_exist - File is ignored: conf/modules.config
  • files_unchanged - File ignored due to lint config: .github/CONTRIBUTING.md
  • files_unchanged - File ignored due to lint config: .github/PULL_REQUEST_TEMPLATE.md
  • files_unchanged - File ignored due to lint config: assets/nf-core-raredisease_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-raredisease_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-raredisease_logo_dark.png
  • modules_config - modules_config

✅ Tests passed:

Run details

  • nf-core/tools version 2.14.1
  • Run at 2024-08-14 09:48:06

fellen31
fellen31 reviewed Aug 13, 2024
View reviewed changes
Copy link
Contributor

@fellen31 fellen31 left a comment

Choose a reason for hiding this comment

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

Are all the mandatory plugin files not supplied with --vep_cache? If so, does it really need to be mandatory, or could you add?

    } else {
        ch_vep_extra_files = Channel.value([])
    }

Otherwise, should It be added to mandatoryParams as well?

@ramprasadn
Copy link
Collaborator Author

Plugin files don't have to supplied with vep_cache. It used to be set up that way because it made it easier for nextflow to stage those files, but we no longer use that approach. There are some plugin files that come bundled with cache (ex: pli), but not all. For instance, we use custom plugins for annotations, and those are not part of vep's cache.

@fellen31
Copy link
Contributor

There are some plugin files that come bundled with cache (ex: pli), but not all. For instance, we use custom plugins for annotations, and those are not part of vep's cache.

Ah, I did not realise pli was bundled with the cache, I thought that was downloaded separately.

@ramprasadn
Copy link
Collaborator Author

You are right, that was my memory failing me. We get that file from https://github.com/Ensembl/VEP_plugins 😅 Then I suppose at least for all of the plugins we use, the data doesn't come bundled with cache.

@fellen31
Copy link
Contributor

Yes, asking because I need to add the --vep_plugin_files parameter as well: genomic-medicine-sweden/nallo#314 😃

@ramprasadn ramprasadn marked this pull request as ready for review August 14, 2024 09:14
@ramprasadn
Copy link
Collaborator Author

aah.. well timed PR 😄

fellen31
fellen31 approved these changes Aug 14, 2024
View reviewed changes
Copy link
Contributor

@fellen31 fellen31 left a comment

Choose a reason for hiding this comment

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

I think, in the test profile these:

LoFtool_scores.txt
pLI_values_107.txt
spliceai_21_scores_raw_indel_-v1.3-.vcf.gz
spliceai_21_scores_raw_indel_-v1.3-.vcf.gz.tbi
spliceai_21_scores_raw_snv_-v1.3-.vcf.gz
spliceai_21_scores_raw_snv_-v1.3-.vcf.gz.tbi

are currently provided to VEP both through --vep_cache with vep_cache_and_plugins.tar.gz and with --vep_plugin_files with vep_files.csv

However,

pLI.pm
SpliceAI.pm

are provided only in vep_cache_and_plugins.tar.gz. If you were to provide with with --vep_plugin_files instead, these would end up in a flat structure directly in the base of the work directory, i.e. as

./pLI.pm
./SpliceAI.pm

and not

./vep_cache/Plugins/pLI.pm
./vep_cache/Plugins/SpliceAI.pm

If you think it's necessary, could you double check that VEP picks up ./pLI.pm and ./SpliceAI.pm even though --dir_plugins vep_cache/Plugins is specified?

Or are they not needed at all because VEP picks up /usr/local/share/ensembl-vep-110.0-0/pLI.pm from within the docker image, and not the the one supplied by the pipeline?

I'm not 100% sure how to tell, or if it's important. Bit of a rabbit hole trying to understand what is needed and not for nallo 😄

I'll approve, but maybe consider updating the docs and/or testdata to reflect this?

@fellen31 fellen31 mentioned this pull request Aug 14, 2024
Merged
10 tasks
@ramprasadn
Copy link
Collaborator Author

I think, in the test profile these:

LoFtool_scores.txt
pLI_values_107.txt
spliceai_21_scores_raw_indel_-v1.3-.vcf.gz
spliceai_21_scores_raw_indel_-v1.3-.vcf.gz.tbi
spliceai_21_scores_raw_snv_-v1.3-.vcf.gz
spliceai_21_scores_raw_snv_-v1.3-.vcf.gz.tbi

are currently provided to VEP both through --vep_cache with vep_cache_and_plugins.tar.gz and with --vep_plugin_files with vep_files.csv

vep_cache_and_plugins.tar.gz file was generated way back when there were no option to feed plugins files seperately so as a roundabout way we packaged the plugins alongside cache. Now even though we have moved to supplying plugin files seperately, we are still using the same file hence the discrepancy. 😄

But I would guess if the files end up in the main working directory and if plugin directory is specified in the command line, it should run the ones found in plugin directory. I am not 100% sure how to tell either.. last time I looked into this, I remember coming out more confused. Need to do a proper investigation sometime soon. 😄

@ramprasadn ramprasadn merged commit ad98b97 into dev Aug 14, 2024
6 checks passed
@ramprasadn
Copy link
Collaborator Author

Looks like plugins come installed so its more likely that we don't need to provide them at all https://github.com/bioconda/bioconda-recipes/blob/master/recipes/ensembl-vep/build.sh

@ramprasadn ramprasadn mentioned this pull request Aug 14, 2024
Closed
@fellen31 fellen31 mentioned this pull request Sep 13, 2024
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fellen31 fellen31 fellen31 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.
2 participants
@ramprasadn @fellen31