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

Feat/pb-fq2bammeth #451

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

Feat/pb-fq2bammeth #451

wants to merge 8 commits into from

Conversation

sateeshperi
Copy link
Contributor

No description provided.

@sateeshperi sateeshperi requested a review from a team as a code owner October 29, 2024 08:20
@sateeshperi sateeshperi self-assigned this Oct 29, 2024
@sateeshperi sateeshperi added this to the 2.8.0 milestone Oct 29, 2024
@sateeshperi sateeshperi linked an issue Oct 29, 2024 that may be closed by this pull request
integrate parabricks/fq2bammeth module #447
Open
@sateeshperi sateeshperi changed the base branch from master to dev October 29, 2024 08:20
@nf-core nf-core deleted a comment from github-actions bot Oct 29, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 29, 2024 •
edited
Loading

nf-core pipelines lint overall result: Passed ✅

Posted for pipeline commit 9be68d9

+| ✅ 199 tests passed       |+
#| ❔   4 tests were ignored |#

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 3.0.2
  • Run at 2024-11-04 11:12:39

GallVp
GallVp reviewed Nov 4, 2024
View reviewed changes
nextflow.config
@@ -216,6 +217,12 @@ profiles {
executor.cpus = 4
executor.memory = 8.GB
}
gpu {
use_gpu = true
docker.runOptions = '-u $(id -u):$(id -g) --gpus device=0'
Copy link
Member

Choose a reason for hiding this comment

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

Why not --gpus all?

GallVp
GallVp reviewed Nov 4, 2024
View reviewed changes
tests/gpu.config
input = "${projectDir}/assets/samplesheet_gpu.csv"
fasta = "${params.pipelines_testdata_base_path}/reference/genome.fa"
fasta_index = "${params.pipelines_testdata_base_path}/reference/genome.fa.fai"
outdir = 'results'
Copy link
Member

Choose a reason for hiding this comment

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

use_gpu = true?

Without that, this is just a non-gpu test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the use_gpu param is set in the when.param block

GallVp
GallVp reviewed Nov 4, 2024
View reviewed changes
conf/base.config
@@ -52,6 +52,9 @@ process {
withLabel:process_high_memory {
memory = { 200.GB * task.attempt }
}
withLabel:process_gpu {
ext.use_gpu = { use_gpu }
Copy link
Member

Choose a reason for hiding this comment

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

What does this line do?

GallVp
GallVp reviewed Nov 4, 2024
View reviewed changes
nextflow.config
@@ -216,6 +217,12 @@ profiles {
executor.cpus = 4
executor.memory = 8.GB
}
gpu {
Copy link
Member

Choose a reason for hiding this comment

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

If the pipeline is started with this profile, the specified settings will be applied to every process.

I think we should apply these settings only to PARABRICKS_FQ2BAMMETH. Or am I missing something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GallVp GallVp GallVp left review comments

@ewels ewels Awaiting requested review from ewels ewels is a code owner automatically assigned from nf-core/methylseq

At least 1 approving review is required to merge this pull request.

Assignees

@sateeshperi sateeshperi

Labels
None yet
Projects
None yet
Milestone
2.8.0
Development

Successfully merging this pull request may close these issues.

integrate parabricks/fq2bammeth module
2 participants
@sateeshperi @GallVp