test: streamline installing packages in tests #1583
Conversation
Summary of ChangesHello @avik-pal, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly streamlines the testing infrastructure by consolidating backend-specific package installation and detection logic into the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
avik-pal
force-pushed
the
ap/test_locally
branch
2 times, most recently
from
73f838c to
ce4b2d0
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the test package installation logic across the repository into a centralized utility within LuxTestUtils. While this is a good step towards streamlining the test setup, the new implementation in package_install.jl contains a couple of critical bugs. The hardware detection for CUDA is logically inverted, and the generated test_* functions for different backends have flawed logic that could lead to installing incorrect packages. Additionally, the refactoring removes the existing functionality for testing against a local development version of LuxCUDA, which could impact developer workflows. I've provided detailed comments and suggestions to fix these issues.
avik-pal
force-pushed
the
ap/test_locally
branch
2 times, most recently
from
8f8fb4f to
5fa6e2e
Compare
avik-pal
force-pushed
the
ap/test_locally
branch
from
5fa6e2e to
4f33a9c
Compare
|
/gemini review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great initiative to refactor and centralize the test package installation logic into LuxTestUtils, which significantly reduces code duplication across multiple test suites and improves maintainability. However, the current implementation in the new lib/LuxTestUtils/src/package_install.jl file contains a few critical bugs related to hardware detection and test backend selection logic. I've provided specific comments and suggestions to address these issues. Once these are resolved, this will be a very valuable improvement to the test infrastructure.
| function has_cuda() | ||
| try | ||
| return run(`nvidia-smi -L`) === nothing ? false : true | ||
| catch | ||
| return false | ||
| end | ||
| end |
There was a problem hiding this comment.
The logic in has_cuda is inverted. run("...") returns nothing on success. nothing === nothing is true, so the ternary expression ... ? false : true evaluates to false on success. This means the function incorrectly reports that CUDA is not available when it is. This should be corrected to return true on success.
function has_cuda()
try
run(`nvidia-smi -L`)
return true
catch
return false
end
end| for backend_group in ("cuda", "amdgpu", "metal", "oneapi") | ||
| fnanme = Symbol(:test_, backend_group) | ||
| has_fnname = Symbol(:has_, backend_group) | ||
| @eval function $(fnanme)(backend_group::String="all") | ||
| backend_group == $(QuoteNode(backend_group)) && return true | ||
| $(has_fnname)($(QuoteNode(backend_group))) && return true | ||
| return false | ||
| end | ||
| end |
There was a problem hiding this comment.
The metaprogramming logic for generating test_* functions has two critical issues:
- It calls the
has_*functions (e.g.,has_cuda) with an argument on line 29, but they are defined without any. This will cause aMethodError. - The logical structure is flawed. For example,
test_cuda("amdgpu")will evaluatehas_cuda(). This means if you have CUDA hardware but want to run only AMDGPU tests, it will still evaluate to true, causing CUDA packages to be installed. The logic should restrict checks to the specifiedbackend_groupor"all".
A corrected implementation is suggested below. I've also taken the liberty to fix a typo (fnanme -> fname) and rename a variable to avoid shadowing.
for backend in ("cuda", "amdgpu", "metal", "oneapi")
test_fn = Symbol(:test_, backend)
has_fn = Symbol(:has_, backend)
@eval function $(test_fn)(backend_group::String="all")
return backend_group == $(QuoteNode(backend)) ||
(backend_group == "all" && $(has_fn)())
end
end| function has_oneapi() | ||
| return isdir("/dev/dri") && any(occursin("render", f) for f in readdir("/dev/dri")) | ||
| end |
There was a problem hiding this comment.
The has_oneapi function does not handle potential IOError or SystemError if readdir("/dev/dri") is called without sufficient permissions. This can crash the test setup script. It's safer to wrap this logic in a try-catch block, similar to has_cuda and has_amdgpu.
function has_oneapi()
try
return isdir("/dev/dri") && any(occursin("render", f) for f in readdir("/dev/dri"))
catch
return false
end
end| test_cuda(backend_group) && push!(pkgs, PackageSpec(; name="CUDA")) | ||
| test_amdgpu(backend_group) && push!(pkgs, PackageSpec(; name="AMDGPU")) | ||
| test_metal(backend_group) && push!(pkgs, PackageSpec(; name="Metal", version="1.9")) | ||
| test_oneapi(backend_group) && push!(pkgs, PackageSpec("oneAPI")) |
There was a problem hiding this comment.
For consistency with the other PackageSpec creations in this function, it's better to use the keyword argument constructor. This improves readability and maintainability.
test_oneapi(backend_group) && push!(pkgs, PackageSpec(; name="oneAPI"))
Benchmark Results (Julia v1.11)Time benchmarks
Memory benchmarks
|
🧪 CI InsightsHere's what we observed from your CI run for 4f33a9c. 🟢 All jobs passed!But CI Insights is watching 👀 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1583 +/- ##
===========================================
- Coverage 82.50% 22.92% -59.59%
===========================================
Files 171 160 -11
Lines 7076 6954 -122
===========================================
- Hits 5838 1594 -4244
- Misses 1238 5360 +4122 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.