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 GPU-based tests #6953

Merged
merged 24 commits into from
Nov 16, 2024
Merged

Add GPU-based tests #6953

merged 24 commits into from
Nov 16, 2024

Conversation

mashehu
Copy link
Contributor

@mashehu mashehu commented Nov 7, 2024

moved the nf-test logic into a separate actions.yml so I can reuse it between for non-gpu and gpu cases. Did some clean-up in the action while at it.

@edmundmiller
Copy link
Contributor

I love that we can do that. I'm sad because it'll mean I have to redo #6286

tests/config/nextflow.config Outdated Show resolved Hide resolved
path: ["${{ fromJson(needs.nf-test-changes.outputs.paths) }}"]
profile: [conda, docker_self_hosted, singularity]
include:
- path: modules/nf-core/parabricks/applybqsr
Copy link
Member

Choose a reason for hiding this comment

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

Will this trigger tests for these three modules whenever if: ( needs.nf-test-changes.outputs.paths != '[]' ) is true.

@GallVp
Copy link
Member

GallVp commented Nov 8, 2024

Let's touch,

  1. A non-gpu module
  2. A gpu module
  3. A non-gpu and a gpu module

to see how it goes in each case.

Cheers!

.github/workflows/test.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@adamrtalbot adamrtalbot 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 we should enable the GPU container flags a slightly different way.

Comment on lines 27 to 31
podman.runOptions = "--runtime crun --platform linux/x86_64 --systemd=always"
} else if ("$PROFILE" == "gpu") {
docker.runOptions = '-u $(id -u):$(id -g) --gpus all'
apptainer.runOptions = '--nv'
singularity.runOptions = '--nv'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer this stuff to go in the configuration of the individual tools, so it can function as a reference implementation for users. Adding it here feels too "secret sauce" that will be hard for other people to follow.

Plus we should use process.containerOptions for process specific stuff.

Or we could add label 'gpu' to achieve a similar thing.

process {
    withLabel "gpu" {
        process.containerOptions = "--gpus all"
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 for label

Copy link
Member

Choose a reason for hiding this comment

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

How do we handle the singularity/apptainer option --nv then?

tests/config/nextflow.config Outdated Show resolved Hide resolved
@mashehu
Copy link
Contributor Author

mashehu commented Nov 8, 2024

For reference, the gpu profile worked for the parabricks tests in #6881

@mashehu
Copy link
Contributor Author

mashehu commented Nov 15, 2024

waiting to get again some modules updated on main, to see if they pick up things correctly.
Also at the moment, I need to work on the logic, to actually only get gpu tests in the the nf-test-gpu test, without adding them manually to the include step...

@mashehu
Copy link
Contributor Author

mashehu commented Nov 15, 2024

happy for now. need to refine the gpu test selection logic, but good to have it in for now

@mashehu mashehu added this pull request to the merge queue Nov 15, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 15, 2024
@sateeshperi sateeshperi added this pull request to the merge queue Nov 16, 2024
Merged via the queue into nf-core:master with commit 21f230b Nov 16, 2024
20 checks passed
Copy link
Contributor

Choose a reason for hiding this comment

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

@mashehu Which one is the right gpu profile? 🫠

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

There's two gpu profiles in this config now

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.

5 participants