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

fix tests if AMDGPU is not available #285

Merged
merged 5 commits into from
Dec 5, 2023
Merged

fix tests if AMDGPU is not available #285

merged 5 commits into from
Dec 5, 2023

Conversation

frapac
Copy link
Member

@frapac frapac commented Dec 4, 2023

No description provided.

@frapac frapac requested a review from michel2323 December 4, 2023 15:11
Copy link

codecov bot commented Dec 4, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c0d69a6) 63.33% compared to head (3cbe3bf) 69.45%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #285      +/-   ##
==========================================
+ Coverage   63.33%   69.45%   +6.11%     
==========================================
  Files          34       35       +1     
  Lines        4484     4488       +4     
==========================================
+ Hits         2840     3117     +277     
+ Misses       1644     1371     -273     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@michel2323
Copy link
Member

michel2323 commented Dec 4, 2023

The test failing with ERROR: LoadError: ArgumentError: Unknown component :hsa is an issue JuliaGPU/AMDGPU.jl#564 with AMDGPU.jl. AMDGPU.jl should be loadable without an AMD GPU present, just like CUDA.jl. I'd prefer to have the tests being dependent on both packages. In particular, this does not resolve the issue if a user wants to use ExaPF and loads AMDGPU for some reason on a system without AMD GPUs.

@michel2323
Copy link
Member

AMDGPU.functional() works as expected. Using that now to guard has_rocm_gpu().

Copy link
Member Author

@frapac frapac left a comment

Choose a reason for hiding this comment

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

Ok to keep both AMDGPU and CUDA in the tests. But I feel somewhat uncomfortable with the current status: it looks to heavy in term of dependencies.

@@ -2,6 +2,8 @@ module TestPolarFormulation

using Test

using AMDGPU
Copy link
Member Author

Choose a reason for hiding this comment

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

I think the subpackage TestPolarForm should not depend on any particular AD backend. I would prefer testing it using the previous fix:

if startswith(string(device), "ROCBackend")
    ....

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok. Makes sense. Ah yes, that's for marking the broken second-order stuff with AMD.

@michel2323
Copy link
Member

Ok to keep both AMDGPU and CUDA in the tests. But I feel somewhat uncomfortable with the current status: it looks to heavy in term of dependencies.

AMGPU and CUDA are not dependencies of ExaPF. I am not sure I understand.

@michel2323
Copy link
Member

@frapac So keeping CUDA.jl and AMDGPU.jl together in the tests and removing GPU packages from TestPolarForm.jl.

@michel2323 michel2323 merged commit 7e3b639 into main Dec 5, 2023
5 checks passed
@amontoison amontoison deleted the fp/fix_tests branch December 5, 2023 20:47
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.

2 participants