8000 Refactor by ramprasadn · Pull Request #574 · nf-core/raredisease · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Refactor #574

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
Merged
merged 7 commits into from
Jun 26, 2024
Merged

Refactor #574

merged 7 commits into from
Jun 26, 2024

Conversation

ramprasadn
Copy link
Collaborator
@ramprasadn ramprasadn commented Jun 24, 2024 •
edited
Loading
  • Removed several skip parameters originally included in the pipeline to avoid failed CI tests, as we have a cleanup step in the CI now so the tests should run fine.
  • Removed the stub run section in the download pipeline workflow because stub tests are run as a part of routine CI tests, and having this step there often resulted in runs crashing due to lack of space.
  • Add a new parameter to skip smncopynumbercaller module.

PR checklist

  • 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
github-actions bot commented Jun 24, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 8101033

+| ✅ 185 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-06-26 10:52:40

@ramprasadn ramprasadn requested a review from jemten June 24, 2024 15:06
@ramprasadn ramprasadn marked this pull request as ready for review June 24, 2024 15:24
fellen31
fellen31 approved these changes Jun 26, 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.

Looks good, just two small comments.

.github/workflows/download_pipeline.yml
Comment on lines -72 to -79

- name: Run the downloaded pipeline (stub)
id: stub_run_pipeline
continue-on-error: true
env:
NXF_SINGULARITY_CACHEDIR: ./
NXF_SINGULARITY_HOME_MOUNT: true
run: nextflow run ./${{ env.REPOTITLE_LOWERCASE }}/$( sed 's/\W/_/g' <<< ${{ env.REPO_BRANCH }}) -stub -profile test,singularity --outdir ./results
Copy link
Contributor

Choose a reason for hiding this comment

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

So now neither the stub run nor the regular run of the downloaded pipeline is tested? As a user of an offline cluster, it's very nice to know that nf-core download works smoothly. But if there's no space maybe there's nothing to do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, that's right. The downloaded pipeline is not tested anymore. I can make sure that the downloaded pipeline works locally before a release, but the github runners just don't have enough space for both the downloaded containers and the tests :(

CHANGELOG.md Outdated
| --------------- | ------------------------ |
| | mbuffer_mem |
| | samtools_sort_threads |
| | skip_repeat_analysis |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think skip_repeat_calling would be more inline with the subworkflow name, the current output and usage docs ("Variant calling - repeat expansions"), and the parameters below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmmm.. good point. The workflow includes stranger which, as you know already, is used to annotate STRs. So it does perform more than repeat calling. Perhaps I need to change the name of the subworkflow, and its references in the pipeline 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree skip_repeat_analysis is more inline with what it actually does.

What about splitting the subworkflow into call and annotate, like you do for SNVs and SVs? Then we could use the same annotation subworkflow in both raredisease and Nallo, and you can have skip_repeat_calling and skip_repeat_annotation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Great minds think like 😆
That's what I am actually doing right now :D Its much easier, and like you said, it is in line with what we are doing for other variant types.

Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice! ⭐

@ramprasadn ramprasadn merged commit 427cbef into dev Jun 26, 2024
6 checks passed
@jemten jemten deleted the change-download-ci branch June 26, 2024 11:38
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

@jemten jemten Awaiting requested review from jemten

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
0