Skip to content

Revive GPU tests #430

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

Merged
merged 1 commit into from
Apr 25, 2025
Merged

Revive GPU tests #430

merged 1 commit into from
Apr 25, 2025

Conversation

ErikQQY
Copy link
Collaborator

@ErikQQY ErikQQY commented Apr 24, 2025

Fix: #429

Instead of completely removing the current ReTest.jl test suite, this PR moves the GPU tests to another test environment so that we can only use Test.

@ErikQQY
Copy link
Collaborator Author

ErikQQY commented Apr 24, 2025

It occurs to me that while we no longer throw warning messages for numerical errors, we can safely use ReTest.jl for GPU tests as well?

# (HongGe) we no longer throw warning messages for numerical errors.
# @test_logs (:warn, "The current proposal will be rejected due to numerical error(s).") init_z1()
# @test_logs (:warn, "The current proposal will be rejected due to numerical error(s).") init_z2()

@yebai
Copy link
Member

yebai commented Apr 24, 2025

@sunxd3, can you review this PR and help set up buildkite for this repo?

@yebai yebai requested a review from sunxd3 April 24, 2025 10:21
@sunxd3
Copy link
Member

sunxd3 commented Apr 24, 2025

The changes looks sensible.

I messaged Tim who manages JuliaGPU on slack and I'll update when he returns.

In the meantime, I don't know AHMC code in close detail. I remember ReTest's purpose is to allow writing tests in source files. Are we using this pattern in AHMC? If not, are there other reasons we would still need ReTest?

@ErikQQY
Copy link
Collaborator Author

ErikQQY commented Apr 24, 2025

ReTest.jl is introduced in #287 with the intention of allowing a subset of testing and providing a better summary of test results, see https://github.com/TuringLang/AdvancedHMC.jl/blob/main/test/README.md

@sunxd3
Copy link
Member

sunxd3 commented Apr 24, 2025

Got it, thanks.

I wonder if we can use Pkg.test directly now, it also takes a test_args argument. (I am not sure if this was not true back then, or ReTest simply has a better interface.)

@ErikQQY
Copy link
Collaborator Author

ErikQQY commented Apr 24, 2025

I wonder if we can use Pkg.test directly now, it also takes a test_args argument.

I may not quite understand here, we can directly call Pkg.test() to test the package. And while the CUDA tests are no longer affected by ReTest.jl, I don't think it really matters if we are using Test or ReTest.

@sunxd3
Copy link
Member

sunxd3 commented Apr 25, 2025

You are right -- this current PR is not discussing the use of ReTest 👍

@yebai
Copy link
Member

yebai commented Apr 25, 2025

I'm merging this now since we need the Buildkite script to be on main to be activated

@yebai yebai merged commit e3a56ed into main Apr 25, 2025
15 of 18 checks passed
@yebai yebai deleted the qqy/cuda branch April 25, 2025 09:16
@sunxd3
Copy link
Member

sunxd3 commented Apr 28, 2025

The GPU pipeline should be set up https://buildkite.com/julialang/advancedhmc-dot-jl

@ErikQQY ErikQQY mentioned this pull request Apr 28, 2025
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.

GPU tests via buildkite
3 participants