[BUG] csrot and zdrot tests fail silently

[Sholde]

Summary

I tried to build the oneMKL tests. But some rot/rotg tests failed because they don't find (and therefore don't load) libblas.so, and therefore the functions are not executed and the reference vectors remain unchanged. Also, we don't have any error message (exception for example) that could help us to fix the bug.

I think you should just need to load the library specified in a new cmake variable, for example REF_BLAS_LIBNAME instead of the hardcoded library libblas.so here.

You can also add an exception if the library is not found here.

And if the function was not found in the library, here

Version

oneMKL version: v0.2 githash: f516ad1

Environment

OS: Debian 10 Compiler: clang version 15.0.0 (intel/llvm 7b2fb02)

Steps to reproduce

Build oneMKL with the mklcpu backend and use a blas library whose name does not match with libblas.so.

Observed behavior

The tests fail not because the result is different , but because the reference functions were not executed because the library libblas.so does not exist. (the reference vectors remain unchanged because the reference functions have not been executed)

Expected behavior

Tests pass if they find the csrot and zdrot functions. Fails with an error and/or an exception if the library or functions were not found.

[andrewtbarker]

Thanks very much for the detailed bug report. I have put this on my todo list, but it looks like you've already identified all the necessary changes. If you want to submit a pull request fixing this that would be appreciated.

[mmeterel]

@Sholde Thanks for the issue and the PR. Terribly sorry for getting back to you so late. :( After reviewing the issue and PR, I am not in favor of skipping the tests if the library name is different than libblas.so. I think, your suggestion about adding a CMAKE variable for the library name makes more sense. To me, the problem is failing to find the right symbols and we should fix it there. Then, no chance needs to be done in the tests and we will not lose functional coverage. Please let us know if that makes sense and willing to update the PR. @andrewtbarker Would you agree with my approach?

[Sholde]

Hello,

I agree with you that creating a new CMAKE variable will be more sensible, but I don't know how to reflect this cmake variable in the C file. Maybe with a define?

So I proposed a simple PR that warns the user when the library was not found.

I'll let you fix that if you want to use a new CMAKE variable.

Regards,

[andrewtbarker]

I agree @mmeterel that actually finding the correct library with cmake is the best approach. Regarding the tests, it may make sense to keep the added throw statements but then not to catch them so we get an error when the library is not found, rather than the current behavior of failing silently.

[mmeterel]

I agree @mmeterel that actually finding the correct library with cmake is the best approach. Regarding the tests, it may make sense to keep the added throw statements but then not to catch them so we get an error when the library is not found, rather than the current behavior of failing silently.

My preference would be to fail the build if the library is not found. IMHO, running the tests does not make sense if the reference library/API is not found. So, I would not change the tests at all.

[mmeterel]

Hello,

I agree with you that creating a new CMAKE variable will be more sensible, but I don't know how to reflect this cmake variable in the C file. Maybe with a define?

So I proposed a simple PR that warns the user when the library was not found.

I'll let you fix that if you want to use a new CMAKE variable.

Regards,

@Sholde Thanks for the feedback. We will work with library engineering team to find a more stable solution. Tagging @vmalia and @mkrainiuk to initiate the discussion.

[vmalia]

Thank you @mmeterel and @Sholde . We can add a find_library call in our CMake to search for libblas.so or blas.dll, and if they are not found, CMake will throw the required error message. In order to get the library reference from CMake, the recommendation is to configure/generate a file using CMake. https://cmake.org/cmake/help/latest/command/configure_file.html We can use a special header file that contains all CMake reference definitions or use existing headers. My recommendation would be to have a separate header and #define all library names there. Those can then be used in other headers or source files. Please share your thoughts and preference.

[mmeterel]

@vmalia Thanks for investigating the issue. Having a separate header file sounds good to me.

[Sholde]

@vmalia Thank you, and I agree with @mmeterel

[vmalia]

Great, I will get this to our team's attention. We will post updates here, as and when they happen. Thanks, folks!

[mkrainiuk]

We already search for required library in our cmake/FindCBLAS and fail if it's not found https://github.com/oneapi-src/oneMKL/blob/722f6c51b528a2bc09bf549b1097f7d8fdc37826/cmake/FindCBLAS.cmake#L25

So we really can improve this function implementation with new variable: https://github.com/oneapi-src/oneMKL/blob/f43e4e5943d0ef3a85ff3331d567eadb09c8fe71/tests/unit_tests/blas/include/reference_blas_templates.hpp#L49

But I think it also will be useful to generate an error if something goes wrong at runtime, e.g., the location of the library was changed after CMake configuration step.

As for config header what do you think about extending https://github.com/oneapi-src/oneMKL/blob/develop/src/config.hpp.in? We already have it included to BLAS testing, so no need to generate and propagate new header file.