Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Build][Docs][CI] BUILD_FUNCTIONAL_TESTS=False by default #557

Closed
wants to merge 2 commits into from
Closed

[Build][Docs][CI] BUILD_FUNCTIONAL_TESTS=False by default #557

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5cba9ef
[Build][Docs][CI] BUILD_FUNCTIONAL_TESTS=False by default
hjabird Aug 16, 2024
b92512f
on -> ON for consistency (Rbiessy)
hjabird Aug 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/pr.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ jobs:
if: steps.domain_check.outputs.result == 'true'
run: |
source /opt/intel/oneapi/setvars.sh
cmake -DTARGET_DOMAINS=${{ matrix.domain }} -DENABLE_MKLGPU_BACKEND=off -DCMAKE_VERBOSE_MAKEFILE=on ${{ matrix.build_options }} -B build
cmake -DTARGET_DOMAINS=${{ matrix.domain }} -DENABLE_MKLGPU_BACKEND=OFF -DBUILD_FUNCTIONAL_TESTS=ON -DCMAKE_VERBOSE_MAKEFILE=on ${{ matrix.build_options }} -B build
hjabird marked this conversation as resolved.
Show resolved Hide resolved
cmake --build build ${PARALLEL}
- name: Run tests
if: steps.domain_check.outputs.result == 'true'
Expand Down
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gentle ping @mkrainiuk. What are your thoughts on this?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good idea. Your solution looks fine to me!


## Examples
option(BUILD_EXAMPLES "" ON)
Expand Down
2 changes: 1 addition & 1 deletion docs/building_and_running_tests.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
Building and Running Tests
==========================

The functional tests are enabled by default, and can be enabled/disabled
The functional tests are disabled by default, and can be enabled/disabled
with the CMake build parameter ``-DBUILD_FUNCTIONAL_TESTS=True/False``. Only
the tests relevant to the enabled backends and target domains will be built.

Expand Down
14 changes: 7 additions & 7 deletions docs/building_the_project_with_adaptivecpp.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ On Linux (other OSes are not supported with the AdaptiveCpp compiler):
-DENABLE_<BACKEND_NAME>_BACKEND=True \ # Enable backend(s) (optional)
-DENABLE_<BACKEND_NAME_2>_BACKEND=True \ # Multiple backends can be enabled at once.
-DHIPSYCL_TARGETS=omp/;hip:gfx90a,gfx906 \ # Set target architectures depending on supported devices.
-DBUILD_FUNCTIONAL_TESTS=False \ # See section *Building the tests* for more on building tests. True by default.
-DBUILD_FUNCTIONAL_TESTS=False \ # See page *Building and Running Tests* for more on building tests. False by default.
-DBUILD_EXAMPLES=False # Optional: True by default.
cmake --build .
cmake --install . --prefix <path_to_install_dir> # required to have full package structure
Expand All @@ -61,11 +61,11 @@ If a backend library supports multiple domains (i.e. BLAS, RNG), it may be
desirable to only enable selected domains. For this, the ``TARGET_DOMAINS``
variable should be set. For further details, see :ref:`_build_target_domains`.

By default, the library also additionally builds examples and tests. These can
be disabled by setting the parameters ``BUILD_FUNCTIONAL_TESTS`` and
``BUILD_EXAMPLES`` to False. Building the functional tests may require additional
external libraries. See the section :ref:`building_and_running_tests` for more
information.
By default, the library builds examples. These can be disabled by setting the
parameter ``BUILD_EXAMPLES`` to ``False``. Tests are disabled by default, but
can be enabled by setting ``BUILD_FUNCTIONAL_TESTS`` to ``True``. Building the
functional tests requires additional external libraries for the BLAS and LAPACK
domains. See the section :ref:`building_and_running_tests` for more information.

The most important supported build options are:

Expand Down Expand Up @@ -98,7 +98,7 @@ The most important supported build options are:
- True
* - BUILD_FUNCTIONAL_TESTS
- True, False
- True
- False
* - BUILD_EXAMPLES
- True, False
- True
Expand Down
21 changes: 10 additions & 11 deletions docs/building_the_project_with_dpcpp.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ for Windows`_ for building on Windows):
-DENABLE_MKLGPU_BACKEND=False \ # Optional: The MKLGPU backend is True by default.
-DENABLE_<BACKEND_NAME>_BACKEND=True \ # Enable any other backend(s) (optional)
-DENABLE_<BACKEND_NAME_2>_BACKEND=True \ # Multiple backends can be enabled at once.
-DBUILD_FUNCTIONAL_TESTS=False \ # See page *Building and Running Tests* for more on building tests. True by default.
-DBUILD_FUNCTIONAL_TESTS=False \ # See page *Building and Running Tests* for more on building tests. False by default.
-DBUILD_EXAMPLES=False # Optional: True by default.
cmake --build .
cmake --install . --prefix <path_to_install_dir> # required to have full package structure
Expand All @@ -72,11 +72,11 @@ If a backend library supports multiple domains (i.e., BLAS, LAPACK, DFT, RNG,
sparse BLAS), it may be desirable to only enable selected domains. For this, the
``TARGET_DOMAINS`` variable should be set. See the section `TARGET_DOMAINS`_.

By default, the library also additionally builds examples and tests. These can
be disabled by setting the parameters ``BUILD_FUNCTIONAL_TESTS`` and
``BUILD_EXAMPLES`` to ``False``. Building the functional tests requires
additional external libraries for the BLAS and LAPACK domains. See the section
:ref:`building_and_running_tests` for more information.
By default, the library builds examples. These can be disabled by setting the
parameter ``BUILD_EXAMPLES`` to ``False``. Tests are disabled by default, but
can be enabled by setting ``BUILD_FUNCTIONAL_TESTS`` to ``True``. Building the
functional tests requires additional external libraries for the BLAS and LAPACK
domains. See the section :ref:`building_and_running_tests` for more information.

The most important supported build options are:

Expand Down Expand Up @@ -130,7 +130,7 @@ The most important supported build options are:
- False
* - BUILD_FUNCTIONAL_TESTS
- True, False
- True
- False
* - BUILD_EXAMPLES
- True, False
- True
Expand Down Expand Up @@ -355,8 +355,7 @@ disabled using the Ninja build system:
-DENABLE_CUFFT_BACKEND=True \
-DENABLE_CUBLAS_BACKEND=True \
-DENABLE_CUSOLVER_BACKEND=True \
-DENABLE_CURAND_BACKEND=True \
-DBUILD_FUNCTIONAL_TESTS=False
-DENABLE_CURAND_BACKEND=True

``$ONEMKL_DIR`` points at the oneMKL source directly. The x86 CPU (``MKLCPU``)
and Intel GPU (``MKLGPU``) backends are enabled by default, but are disabled
Expand All @@ -376,8 +375,7 @@ disabled:
-DENABLE_ROCFFT_BACKEND=True \
-DENABLE_ROCBLAS_BACKEND=True \
-DENABLE_ROCSOLVER_BACKEND=True \
-DHIP_TARGETS=gfx90a \
-DBUILD_FUNCTIONAL_TESTS=False
-DHIP_TARGETS=gfx90a

``$ONEMKL_DIR`` points at the oneMKL source directly. The x86 CPU (``MKLCPU``)
and Intel GPU (``MKLGPU``) backends are enabled by default, but are disabled
Expand All @@ -396,6 +394,7 @@ GPU and Nvidia GPU with testing enabled:
-DENABLE_ROCFFT_BACKEND=True \
-DENABLE_CUFFT_BACKEND=True \
-DTARGET_DOMAINS=dft \
-DBUILD_FUNCTIONAL_TESTS=True \
-DBUILD_EXAMPLES=False

Note that this is not a supported configuration, and requires Codeplay's oneAPI
Expand Down
Loading