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

CI: add gpu tag test job #478

Open
wants to merge 48 commits into
base: fasta-align-dedup-bwameth
Choose a base branch
from

Conversation

sateeshperi
Copy link
Contributor

No description provided.

@sateeshperi sateeshperi requested a review from a team as a code owner December 2, 2024 05:58
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@nf-core nf-core deleted a comment from github-actions bot Dec 3, 2024
Copy link

github-actions bot commented Dec 3, 2024

nf-core pipelines lint overall result: Passed ✅

Posted for pipeline commit 60947ab

+| ✅ 201 tests passed       |+
#| ❔   5 tests were ignored |#

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 3.0.2
  • Run at 2024-12-03 22:26:07

nf-test test \
--ci \
--shard ${{ inputs.shard }}/${{ inputs.total_shards }} \
--changed-since HEAD^ \
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
--changed-since HEAD^ \

we already get the tests to run in the nf-test-shard action (just need to export it there), simpler than running it in two places.

Additionally, this changed since will also include deleted files, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have to check about deleted files

conda-remove-defaults: true

- name: Clean up Disk space
uses: jlumbroso/free-disk-space@54081f138730dfa15788a46383842cd2f914a1be # v1.3.1
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way to make this action not spam the whole log

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I ma not convinced that this action is always required. I only include it for specific scenarios such as when 8GB containers have to be downloaded. Will leave it to @sateeshperi

Just an example of what I have done elsewhere: https://github.com/Plant-Food-Research-Open/genepal/blob/ee702d71a9ea4f422c92729533ff0564c231b5e8/.github/workflows/ci.yml#L77

nextflow.config Outdated
@@ -221,6 +221,17 @@ profiles {
executor.cpus = 4
executor.memory = 8.GB
}
docker_self_hosted {
Copy link
Member

Choose a reason for hiding this comment

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

Should we place it in nf-test.config so as to not cause confusion for the users?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am actually curious and think we don't need a special docker profile for GPU runners, because they are not running in rootless docker mode

Copy link
Member

Choose a reason for hiding this comment

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

I am working on this PR. Will look into it as well. Thank you!

NFT_VER: ${{ env.NFT_VER }}
with:
tags: "cpu"
max_shards: 5
Copy link
Member

Choose a reason for hiding this comment

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

I have capped the number of shards to 5. This might be a bit too aggressive. @sateeshperi , please feel free to bump it up.

echo "No related tests found."
else
# Extract the number of related tests
number_of_shards=$(echo "$nftest_output" | sed -n 's|.*Executed \([0-9]*\) tests.*|\1|p')
Copy link
Member

Choose a reason for hiding this comment

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

Here we use shards = tests strategy. This is good for long running pipeline level tests. For modules, we might want to switch to shards = tests / n as the runtime of a single module is generally comparable to action setup time.

@GallVp

This comment was marked as resolved.

@GallVp
Copy link
Member

GallVp commented Dec 3, 2024

Some of the CPU tests are failing due to permission issues. It might be related to docker_self_hosted.

@mashehu
Copy link
Contributor

mashehu commented Dec 4, 2024

Ah sorry, forgot about the cpu tests. They indeed still run on rootless docker, so need docker_self_hosted but I will try to fix that soon ™

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.

4 participants