[Build][Docs][CI] BUILD_FUNCTIONAL_TESTS=False by default #557
Conversation
* Currently tests build by default * This adds additional dependencies, tripping users up. * This slows down builds where the tests are not used. * This PR disabled tests by default and * Updates the documentation * Updates the Github CI
Co-authored-by: Romain Biessy <[email protected]>
| @@ -64,7 +64,7 @@ set(ONEMKL_SYCL_IMPLEMENTATION "dpc++" CACHE STRING "Name of the SYCL compiler") | |||
| set(HIP_TARGETS "" CACHE STRING "Target HIP architectures") | |||
|
|
|||
| ## Testing | |||
| option(BUILD_FUNCTIONAL_TESTS "" ON) | |||
| option(BUILD_FUNCTIONAL_TESTS "" OFF) | |||
There was a problem hiding this comment.
I agree that this extra Netlib dependency causes problems for unexperienced users, so it makes sense to disable it, but I think we still need some default validation to make sure the build is fine. As far as I know extra dependency is a problem only for BLAS and LAPACK, does it make sense to keep testing for other domains since they don't have extra dependencies? And for BLAS/LAPACK we can come up with more light-weight testing in the future.
There was a problem hiding this comment.
Are you talking about a default validation for unexperienced users? The examples are still built by default and don't require any external dependencies. This should be enough to make sure everything built fine.
There was a problem hiding this comment.
I'm not sure I can agree that passed examples are enough to confirm that built oneMKL Interfaces functionality is fine, we cover just 1-2 functions per domain, only USM API and mostly Intel oneMKL CPU backend for compiler time dispatcher from the build and we do not check correctness which could be a tricky problem to catch in the application where this oneMKL Interfaces build will be used.
There was a problem hiding this comment.
I'm not saying the examples are enough to test all the functionalities, just that they are enough to show oneMKL built. The point of this PR is that we have seen users getting confused why they couldn't build oneMKL and the issue was due to the additional dependencies needed by the tests. This PR aims to clarify this and force the users to manually enable the tests if they want them.
I think this makes it easier for first time users.
There was a problem hiding this comment.
Gentle ping @mkrainiuk. What are your thoughts on this?
There was a problem hiding this comment.
I support the idea of disabling testing for BLAS and LAPACK for the first time users and come up with some light testing that won't require Netlib package in the future, but I don't understand why we need to disable testing for DFT, RNG, and spBLAS that have proper testing coverage by default and don't have any extra dependencies that might confuse users. Could you please clarify why it's required to change testing mode for these domains as part of Netlib dependency problem?
There was a problem hiding this comment.
It is much simpler for users and developers if a CMake option always has the same default value. Some people use CMake GUI tools which show all the options with the default value but this causes confusions when the value change due to another option. We would need to add some documentation to explain when the tests are enabled or disabled and hope users find it.
There was a problem hiding this comment.
That's true, in this case I don't think modifying common option will be the right solution, instead it will be better to change requirements for Netlib dependency in two specific domains, where it's used, we can make it optional and based on the Netlib availability enable/disable unit tests where Netlib is used (later on adding some tests where it's not used so they can be built without Netlib).
There was a problem hiding this comment.
To be more specific here is a draft with some quick changes for what I've proposed before as potential solution for Netlib dependency problem #577
Please let me know what do you think about this approach.
There was a problem hiding this comment.
That's a good idea. Your solution looks fine to me!
|
Closing in favor of #577 |
Description
Fixes #443
Checklist
All Submissions