Skip to content

[CMake][tmva] Fix SOFIE tests exclusion with builtin GSL#21582

Open
AdityaDRathore wants to merge 2 commits intoroot-project:masterfrom
AdityaDRathore:fix/tmva-cmake-gsl
Open

[CMake][tmva] Fix SOFIE tests exclusion with builtin GSL#21582
AdityaDRathore wants to merge 2 commits intoroot-project:masterfrom
AdityaDRathore:fix/tmva-cmake-gsl

Conversation

@AdityaDRathore
Copy link

This Pull request:

The TMVA-SOFIE test configurations in tmva/sofie/test/CMakeLists.txt currently gate the GTest targets strictly on if(BLAS_FOUND).

However, when ROOT is configured to fall back to the builtin GSL library to fulfill its BLAS interface requirement (-Dbuiltin_gsl=ON), the use_gsl_cblas CMake flag is correctly set to ON, but BLAS_FOUND remains OFF.

Consequently, TMVA successfully links against the internal ROOT::BLAS interface, 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.txt need to account for both standalone BLAS discovery and GSL CBLAS fallback.

For instance, surrounding the GTest targets, the conditional logic:

if (BLAS_FOUND)

And

if (tpython AND ROOT_KERAS_FOUND AND BLAS_FOUND)

Should be updated to:

if (BLAS_FOUND OR use_gsl_cblas)

And

if (tpython AND ROOT_KERAS_FOUND AND (BLAS_FOUND OR use_gsl_cblas))

(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:

  • tested changes locally
  • updated the docs (if necessary)

@bellenot @lmoneta @sanjibansg @guitargeek

@AdityaDRathore AdityaDRathore changed the title [CMake][tmva] Fix SOFIE tests exclusion with builtin GSL (use_gsl_cblas=ON) [CMake][tmva] Fix SOFIE tests exclusion with builtin GSL Mar 12, 2026
@guitargeek guitargeek self-assigned this Mar 13, 2026
@guitargeek guitargeek self-requested a review March 13, 2026 10:35
@github-actions
Copy link

github-actions bot commented Mar 13, 2026

Test Results

    21 files      21 suites   2d 23h 4m 17s ⏱️
 3 833 tests  3 832 ✅ 1 💤 0 ❌
72 872 runs  72 863 ✅ 9 💤 0 ❌

Results for commit f577714.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

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.

@AdityaDRathore
Copy link
Author

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,
Aditya

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