Skip to content

Conversation

KADichev
Copy link
Collaborator

@KADichev KADichev commented Sep 23, 2024

All functional tests are now GoogleTest tests.

The main changes for CMake are:

  • reorganize CMake to use GTest CMake package for compilation, and gtest_add_tests clause to define Gtests;
  • remove all use of the run.sh script;
  • make explicit the list of tests and do not use file(GLOB, ...) clauses to look for all files in test directories, as recommended by CMakesource files:

The main changes for source files are:

  • every single test file needed to be modified to not include internal Test.h but gtest.h, and to use the assert/equal clauses of GoogleTest
  • all death tests are slightly modified now to expect FAIL() after the expected fail condition
  • some modernization of C to C++ was needed for a number of files, incl. use of new/delete instead of malloc/free, or string manipulation instead of C char * manipulation

A new Python wrapper script test_launcher.py is being employed now for more flexible checking of return codes, which is needed for death tests and normal tests alike. The expected return codes are still parsed from scanning the source files as before.

The use of the variable TEST_LAUNCHER is quite important, as it sets our wrapper test_launcher.py and its parameters for each single test individually, which is needed, as each test has its own engine, parameteres, expected return code.

…quests in a queue pair), not just relying on device information about max_qp_wr, but actually trying to create QPs via ibv_create_qp with different max_send_wr until we find the largest still working number (via binary search). This becomes the updated m_maxSrs. Independently, the 100K element test manyPuts needs to be downgraded to 5K for our cluster, as our count is just over 10K, but actually 10K does not work as well (not sure why?)
…e in manyPuts -- the device does not support having too many messages in the send WR QP
…it is very complicated to fix these tests - they seem all over the place, not working, but commiting it
… (only the minimum) but still have many failing tests without explanation, and not tested at all properly
…in the execute command. Also, reduce some example message count as it does not work with IB Verbs with very large tests on the ARM machine
…ent logic if the bloody Gtest wants to just list the tests or run them
…. Also, using Setup and TearDown for entire test suite now, pretty neat, no code duplication each time
…d remove c99 requirement and turn them into C++ tests
… I need to make MPI engines in debug layer call MPI_Abort, and pthread engine in debug layer call std::abort
…y, while it works, this didn't solve the problem. Mpirun still is used with pthreads, so it changes the std::abort signal to 134. This is why now I changed the launcher. Still having issues with some hybrid tests though.
…ap script from pre-existing googletest messages
… which internally is non-portably converted to 134. This also simplifies the launcher script. Also fix some incorrect delete's for arrays in the collectives
…y, we get all tests at the moment via gtest_add_tests. It would be good to replace gtest_add_tests with gtest_discover_tests in the future though, because the current one takes 60-90 seconds to configure. Also, there is a horrible bug now where if I specify a high CMake version (e.g. the needed 3.29.0), the GoogleTests would simply not compile at all
@anyzelman
Copy link
Member

At this point, all tests pass on:

  • x86_64, CentOS Stream 9, GCC 11.5, MPICH 4.1.1
  • x86_64, CentOS Stream 9, GCC 11.5, OpenMPI 4.1.1
  • ARM, Ubuntu 24.01 LTS, GCC 11.4, MPICH 4.3.0b1

With OpenMPI, the only exception is that the following two tests could fail due to not enough slots available to OpenMPI:

The following tests FAILED:
        285 - mpimsg_API.func_lpf_register_local_parallel_multiple (Failed)
        378 - mpirma_API.func_lpf_register_local_parallel_multiple (Failed)

The fix would be to pass --oversubscribe to the test launcher, or to configure OpenMPI to have hardware threads dub as slots. Issue #37 has been raised to make it easier; I suggest to not change anything related to this in this MR.

…use mpirma and mpimsg as fallback in such cases
…ible (even if no IB card is present). With GTest integration now, this is only possible when tests are disabled
…d ensure that the variables are populated by dependent engines"

This reverts commit faa3b33.
… which were declared in the MPI directory for the MPI engine, while engines in general also define unit tests that need call add_gtest. We break the cycle by pulling engine-specific cflags into the main CMakeLists.txt
…ll have no test on OS X, but at least we will not break something else for that target
Copy link
Member

@anyzelman anyzelman left a comment

Choose a reason for hiding this comment

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

This MR has no regressions in functionality and adds all benefits that come with full Google Test integration. In retaining past functionality, as a side-effect also a new feature was added: hybrid tests now also build if libibverbs is not available (it will then rely on the mpirma engine if available, and the mpimsg engine if not).

This MR also resolves bugs that have appeared on more modern cluster deployments compared to the previous main tag of LPF. This is hence a priority MR to base all further extensions and evolutions of LPF on.

At this point, this MR has tested successfully for:

  • x86_64, CentOS Stream 9, GCC 11.5.0, MPICH 4.1.1 (caveat 2)
  • x86_64, CentOS Stream 9, GCC 11.5.0, OpenMPI 4.1.1 (caveats 1, 2, and 5)
  • x86_64, Fedora, GCC 9.3.1, MPICH 3.3.2 (caveat 2)
  • x86_64, Ubuntu 23.04, GCC 13.1.0, OpenMPI 4.1.6 (caveat 5)
  • x86_64, openEuler 20.03 LTS, GCC 7.3, MPICH 3.2.1
  • x86_64, Ubuntu 22.04 LTS, GCC 11.4.0, MPICH 4.3.0b1 (caveat 3, transient)
  • ARM, Ubuntu 22.04 LTS, GCC 11.4.0, MPICH 4.3.0b1 (caveats 3 & 4)

This are under different variations of calls to bootstrap.sh to test all the above-described changes.

The following caveats apply:

  1. with OpenMPI and default arguments to its mpirun, oversubscription to larger process counts may be limited, causing some tests to fail. Issue #37 has been raised to pass the appropriate flags to the OpenMPI mpirun during testing;
  2. make install post-install checks fail (as they should) when: 1) tests are disabled, 2) ibverbs dev libraries are present, but 3) no IB card is present. On the machines with IB we tested on, all post-install checks succeed.
  3. The hybrid backend on MPICH 4.3.0b1 does not pass the debug layer tests due to a segfault in MPICH's MPI_Abort. Issue #41 has been raised to track this. This error presently only is consistently reproducible on our cluster's ARM nodes and transient on our cluster's x86_64 nodes. They have not been observed outside our cluster.
  4. With MPICH 4.3.0b1, we furthermore hit issues #43 and #44. Also these issues are only reproducible on ARM nodes.
  5. With OpenMPI 4.1.6 and OpenMPI 4.1.1 on x86_64 we hit issue #45
  6. At present, we have no CI flow that can run all build variations relevant to this MR. Issues #40 has been raised to (partially) address as well as to track this issue.

At present, I suggest to merge first and consider the above known issues separately.

This MR fails with the following deprecated/unsupported MPI implementations:

  • OpenMPI 4.0.2 (tested on x86_64, Fedora, GCC 9.3.1)

I suggest, since these are unsupported MPI implementations, we file this under "won't fix".

@anyzelman
Copy link
Member

Concept release notes:

This MR makes all LPF functional tests use GoogleTest.

The main changes for CMake are:

  • reorganize CMake to use GTest CMake package for compilation, and gtest_add_tests clause to define Gtests;
  • remove all use of the run.sh script;
  • make explicit the list of tests and do not use file(GLOB, ...) clauses to look for all files in test directories, as recommended by CMakesource files:

The main changes for source files are:

  • every single test file needed to be modified to not include internal Test.h but gtest.h, and to use the assert/equal clauses of GoogleTest
  • all death tests are slightly modified now to expect FAIL() after the expected fail condition
  • some modernization of C to C++ was needed for a number of files, incl. use of new/delete instead of malloc/free, or string manipulation instead of C char * manipulation

A new Python wrapper script test_launcher.py is being employed now for more flexible checking of return codes, which is needed for death tests and normal tests alike. The expected return codes are still parsed from scanning the source files as before.

For the complete history, see GitHub PR #26 .

This MR also resolves bugs that have appeared on more modern cluster deployments compared to the previous main tag of LPF. This is hence a priority MR to base all further extensions and evolutions of LPF on. This MR was tested successfully for:

  • x86_64, CentOS Stream 9, GCC 11.5.0, MPICH 4.1.1 (caveat 2)
  • x86_64, CentOS Stream 9, GCC 11.5.0, OpenMPI 4.1.1 (caveats 1, 2, and 5)
  • x86_64, Fedora, GCC 9.3.1, MPICH 3.3.2 (caveat 2)
  • x86_64, Ubuntu 23.04, GCC 13.1.0, OpenMPI 4.1.6 (caveat 5)
  • x86_64, openEuler 20.03 LTS, GCC 7.3, MPICH 3.2.1
  • x86_64, Ubuntu 22.04 LTS, GCC 11.4.0, MPICH 4.3.0b1 (caveat 3, transient)
  • ARM, Ubuntu 22.04 LTS, GCC 11.4.0, MPICH 4.3.0b1 (caveats 3 & 4)

These are under different variations of calls to bootstrap.sh to test all the above-described changes.

The following caveats apply:

  1. with OpenMPI and default arguments to its mpirun, oversubscription to larger process counts may be limited, causing some tests to fail. Issue Make it easier to pass arguments for mpirun to test launcher #37 has been raised to pass the appropriate flags to the OpenMPI mpirun during testing;
  2. make install post-install checks fail (as they should) when: 1) tests are disabled, 2) ibverbs dev libraries are present, but 3) no IB card is present. On the machines with IB we tested on, all post-install checks succeed.
  3. The hybrid backend on MPICH 4.3.0b1 does not pass the debug layer tests due to a segfault in MPICH's MPI_Abort. Issue The debug tests for hybrid engine fails for MPICH 4.3.0b1 on call to MPI_Abort #41 has been raised to track this. This error presently only is consistently reproducible on our cluster's ARM nodes and transient on our cluster's x86_64 nodes. They have not been observed outside our cluster.
  4. With MPICH 4.3.0b1, we furthermore hit issues With MPICH 4.3.0b1, func_bsplib_hpsend_many_hybrid_*_debug segfaults #43 and With MPICH 4.3.0b1, func_lpf_hook_tcp_timeout.mpirma returns wrong error code #44. Also these issues are only reproducible on ARM nodes.
  5. With OpenMPI 4.1.6 and OpenMPI 4.1.1 on x86_64 we hit issue func_lpf_probe_parallel_nested tests UB? #45
  6. At present, we have no CI flow that can run all build variations relevant to this MR. Issues Bring up a basic GitHub CI  #40 has been raised to (partially) address as well as to track this issue.

@anyzelman anyzelman merged commit 14ee22a into master Dec 2, 2024
KADichev added a commit that referenced this pull request Dec 19, 2024
This MR makes all LPF functional tests use GoogleTest.

The main changes for CMake are:
- reorganize CMake to use GTest CMake package for compilation, and gtest_add_tests clause to define Gtests;
- remove all use of the run.sh script;
- make explicit the list of tests and do not use file(GLOB, ...) clauses to look for all files in test directories, as recommended by CMakesource files:

The main changes for source files are:
- every single test file needed to be modified to not include internal Test.h but gtest.h, and to use the assert/equal clauses of GoogleTest
- all death tests are slightly modified now to expect FAIL() after the expected fail condition
- some modernization of C to C++ was needed for a number of files, incl. use of new/delete instead of malloc/free, or string manipulation instead of C char * manipulation

A new Python wrapper script test_launcher.py is being employed now for more flexible checking of return codes, which is needed for death tests and normal tests alike. The expected return codes are still parsed from scanning the source files as before.

For the complete history, see GitHub PR #26 .

This MR also resolves bugs that have appeared on more modern cluster deployments compared to the previous main tag of LPF. This is hence a priority MR to base all further extensions and evolutions of LPF on. This MR was tested successfully for:
 - x86_64, CentOS Stream 9, GCC 11.5.0, MPICH 4.1.1 (caveat 2)
 - x86_64, CentOS Stream 9, GCC 11.5.0, OpenMPI 4.1.1 (caveats 1, 2, and 5)
 - x86_64, Fedora, GCC 9.3.1, MPICH 3.3.2 (caveat 2)
 - x86_64, Ubuntu 23.04, GCC 13.1.0, OpenMPI 4.1.6 (caveat 5)
 - x86_64, openEuler 20.03 LTS, GCC 7.3, MPICH 3.2.1
 - x86_64, Ubuntu 22.04 LTS, GCC 11.4.0, MPICH 4.3.0b1 (caveat 3, transient)
 - ARM, Ubuntu 22.04 LTS, GCC 11.4.0, MPICH 4.3.0b1 (caveats 3 & 4)

These are under different variations of calls to `bootstrap.sh` to test all the above-described changes.

The following caveats apply:
1. with OpenMPI and default arguments to its `mpirun`, oversubscription to larger process counts may be limited, causing some tests to fail. Issue #37 has been raised to pass the appropriate flags to the OpenMPI `mpirun` during testing;
2. `make install` post-install checks fail (as they should) when: 1) tests are disabled, 2) ibverbs dev libraries are present, but  3) no IB card is present. On the machines with IB we tested on, all post-install checks succeed.
3. The hybrid backend on MPICH 4.3.0b1 does not pass the debug layer tests due to a segfault in MPICH's MPI_Abort. Issue #41 has been raised to track this. This error presently only is consistently reproducible on our cluster's ARM nodes and transient on our cluster's x86_64 nodes. They have not been observed outside our cluster.
4. With MPICH 4.3.0b1, we furthermore hit issues #43 and #44. Also these issues are only reproducible on ARM nodes.
5. With OpenMPI 4.1.6 and OpenMPI 4.1.1 on x86_64 we hit issue #45
6. At present, we have no CI flow that can run all build variations relevant to this MR. Issues #40 has been raised to (partially) address as well as to track this issue.
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.

3 participants