[CMake][tmva] Fix SOFIE tests exclusion with builtin GSL#21582
[CMake][tmva] Fix SOFIE tests exclusion with builtin GSL#21582AdityaDRathore wants to merge 2 commits intoroot-project:masterfrom
Conversation
a3fe28b to
a088275
Compare
Test Results 21 files 21 suites 2d 23h 4m 17s ⏱️ Results for commit f577714. ♻️ This comment has been updated with latest results. |
guitargeek
left a comment
There was a problem hiding this comment.
Thanks for the initiative!
The PR doesn't work as-is yet, because in the tmva/sofie/test/CMakeLists.txt file, we are still using a target that is only available when BLAS is found: BLAS::BLAS.
The compatibility target for using either the regular BLAS or the GSL BLAS is called ROOT::BLAS, so you also have to use that:
https://github.com/root-project/root/blob/master/cmake/modules/SearchInstalledSoftware.cmake#L1449
Also, how did you test this PR? As you can see in our CI run, we don't have any test platform where a regular BLAS library is not available, which is why the obvious CMake targets problem in your code was not caught. Given that you probably also thought that it works for you locally, I was wondering if you maybe also have a BLAS library installed after all, and the fallback to GSL is also not required for you? That would weaken the motivation for this change, because any fallback to a different library is a potential source of fragility, and if it's not strictly required, we should avoid it.
|
Thanks for the review. You're right on the BLAS::BLAS point , I've updated the PR to replace all four occurrences (TestGemmDerivative, TestSofieModels, TestRModelParserPyTorch, TestRModelParserKeras) with ROOT::BLAS. On your testing question: I have to be honest,my original PR submission was based on code inspection rather than a properly reproduced build. My local machine has external BLAS installed, so BLAS_FOUND was ON and the GSL fallback path never actually triggered. I should have been explicit about that upfront. After your feedback, I applied the ROOT::BLAS change alongside my original fix and did a proper reproduction on a machine with no external BLAS (BLAS_FOUND=OFF, use_gsl_cblas=ON). Without the condition fix the target is absent from the build manifest; with it, TestRModelParserKeras builds cleanly. Please let me know if there is anything further to address. Thanks, |
This Pull request:
The TMVA-SOFIE test configurations in
tmva/sofie/test/CMakeLists.txtcurrently gate the GTest targets strictly onif(BLAS_FOUND).However, when ROOT is configured to fall back to the builtin GSL library to fulfill its BLAS interface requirement (
-Dbuiltin_gsl=ON), theuse_gsl_cblasCMake flag is correctly set toON, butBLAS_FOUNDremainsOFF.Consequently, TMVA successfully links against the internal
ROOT::BLASinterface, but the SOFIE execution tests (e.g.,TestRModelParserKeras,TestRModelParserPyTorch) are silently excluded from the build manifest.This patch updates the capability checks to account for both standalone BLAS discovery and GSL CBLAS fallback by verifying
if (BLAS_FOUND OR use_gsl_cblas).Reproducer
cmake -Dbuiltin_gsl=ON -Dtpython=ON -DROOT_TEST_KERAS=ON ../src cmake --build . --target TestRModelParserKeras(Fails: Target does not exist prior to this patch).
Changes or fixes:
The capability checks in
tmva/sofie/test/CMakeLists.txtneed to account for both standalone BLAS discovery and GSL CBLAS fallback.For instance, surrounding the GTest targets, the conditional logic:
if (BLAS_FOUND)And
Should be updated to:
And
(I have tested this patch locally and it successfully re-enables the test suite compilation while correctly linking TestRModelParserKeras against the GSL CBLAS interface).
Checklist:
@bellenot @lmoneta @sanjibansg @guitargeek