Skip to content

Conversation

fabianliebig
Copy link
Collaborator

This PR adds a dedicated Tox environment for GPU-related tests and one manual workflow as well as additional steps to regular and CI for installing and starting those tests. At the moment, they are only executed if triggered manually and if core tests have passed. Support for other triggers is WIP. Additionally, I've changed the Benchmark installation to UV and added terminal outputs of standard resource commands for CPU, RAM and so on.

@fabianliebig fabianliebig self-assigned this Sep 8, 2025
@fabianliebig fabianliebig marked this pull request as ready for review September 8, 2025 05:47
Copy link
Collaborator

@AVHopp AVHopp left a comment

Choose a reason for hiding this comment

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

First round of comments.

Update the concurrency group name for the GPU Test Workflow by adding an extension since the calling workflow, which is in the same concurrency group, will block this one, which leads to a deadlock, and the pipeline will skip this call.
@fabianliebig fabianliebig deleted the fix/change-benchmark-env-creation-and-add-gpu-tox branch September 10, 2025 09:24
@fabianliebig fabianliebig restored the fix/change-benchmark-env-creation-and-add-gpu-tox branch September 10, 2025 09:24
@fabianliebig fabianliebig reopened this Sep 10, 2025
@Copilot Copilot AI review requested due to automatic review settings September 11, 2025 10:56
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds GPU testing capabilities to the project by introducing a dedicated Tox environment for GPU tests and workflow configurations. The changes include manual workflow triggers for GPU testing, system information outputs for benchmarking, and modernization of the benchmark installation process.

  • Adds a new gputest Tox environment for GPU-specific testing
  • Introduces manual GPU testing workflows that require core tests to pass first
  • Modernizes benchmark installation by switching from pip to uv package manager

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tox.ini Adds gputest environment configuration with GPU availability check
.github/workflows/regular.yml Adds GPU testing job triggered by manual workflow dispatch
.github/workflows/gpu_tests.yml New dedicated workflow for GPU tests with AWS Lambda runner provisioning
.github/workflows/ci.yml Adds GPU testing job to CI workflow with manual trigger
.github/workflows/benchmark.yml Adds system information output and migrates to uv package manager

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

extras = test
passenv =
CI
BAYBE_NUMPY_USE_SINGLE_PRECISION
Copy link
Collaborator

Choose a reason for hiding this comment

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

whats the intention of these flags?
if it is to enforce single precision I think they should rather be part of setenv

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I misunderstood that. I thought that would configure which ENV variables will be parsed when set on the computer where Tox is executed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

no thats right, so this was the intention?
Ie one could run GPU tests in single and double precision? Or do GPU tests need to be run in single precision anyway?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought that might become important because if a remember correctly, @AVHopp mentioned that there was an issue regrading the precision when he started investigation GPUs. Maybe he can comment on that before I confuse something here. But it is probably better to add those flag when GPU support is tackled, so we can also remove it. Thanks for pointing it out :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

no please dont change it yet, I think this line is reasonable if GPUs can indeed be run with double and single precision, if not possible generally then we should probably change it

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