Tpetra: unexercised test; missing ROCm test; change in sub package test behavior

[maartenarnst]

Enhancement

@trilinos/tpetra

In our Trilinos builds, we double check certain properties of the build process, just to make sure that our building tool works as expected. Among other properties, we track the number of Tpetra tests. We noticed a few issues/questions:

• Following recent changes relevant to the switch to modern CMake, it seems the sub package behavior has changed. It seems it is now no longer sufficient to set Tpetra_ENABLE_TESTS to ON. In our builds, setting only this CMake variable to ON does no longer exercise the Core and TSQR tests. It seems it is now necessary to set explicitly TpetraCore_ENABLE_TESTS and TpetraTSQR_ENABLE_TESTS to ON to exercise the Core and TSQR tests.

• PR #10552 deleted in core/test/CrsMatrix/CrsMatrix_createDeepCopy.cpp a number of preprocessor ifdef regions related to deprecated code. However, this PR also deleted in core/test/CrsMatrix/CMakeLists.txt an IF region concerning this test. Following this change, it appears the test is no longer exercised, but the file is still left in the source code. It is unclear whether the source code file should be deleted or the test should be reintegrated in the CMakeLists.txt .

• The test related to core/test/CrsMatrix/Tpetra_Test_CrsMatrix_WithGraph.hpp seems to follow an instantiation logic that is different from other Tpetra tests. In the folder core/test/CrsMatrix, there are separate files that each exercise this test for a different exe space. However, the ROCm case is missing. It seems to be an important test, and it is unclear why the ROCm case is missing. One way of solving this issue might perhaps be to use Tpetra ETI logic to instantiate the test for all relevant exe spaces.

[rppawlo]

@bartlettroscoe

[csiefer2]

Thanks for looking at this in detail! When we're able to merge PRs again we'll take a look!

[bartlettroscoe]

Following recent changes relevant to the switch to modern CMake, it seems the sub package behavior has changed. It seems it is now no longer sufficient to set Tpetra_ENABLE_TESTS to ON. In our builds, setting only this CMake variable to ON does no longer exercise the Core and TSQR tests. It seems it is now necessary to set explicitly TpetraCore_ENABLE_TESTS and TpetraTSQR_ENABLE_TESTS to ON to exercise the Core and TSQR tests.

@maartenarnst, I did recently update the behavior of enabling package-level and subpackage-level tests and examples and tried to add pretty detailed unit tests to make sure it behaves as it should. But this did result in tests for some subpackages to not be abled that were enabled before (but one could argue should have never been enabled in those scenarios). See TriBITSPub/TriBITS#268 and the "Changed" item at:

• https://github.com/TriBITSPub/TriBITS/blob/master/tribits/CHANGELOG.md#2022-08-18

and see:

• https://tribitspub.github.io/TriBITS/users_guide/index.html#enable-disable-of-parent-package-tests-examples-is-enable-disable-for-subpackages-tests-examples

Can you please provide the exact cmake input you are using to show this behavior and can you please attach the STDOUT/STDERR from the cmake command and the generated CMakeCache.txt file?

[maartenarnst]

Hi @bartlettroscoe. Thank you for looking into this and for the links. You will find in attachment the files that you asked for. If you should have suggestions for how we could improve the cmake settings that we set, please do not hesitate.

Small note: It seems GitHub doesn't allow .json files to be uploaded, so I'm adding .txt to the CMakePresets.json filename.

CMakePresets.json.txt stdout.txt stderr.txt CMakeCache.txt

[bartlettroscoe]

@maartenarnst, thanks for the info. The important bits of the output shown the those files are:

Preset CMake variables:

  ...
  Tpetra_ENABLE_EXAMPLES="OFF"
  Tpetra_ENABLE_TESTS="ON"
  ...
  Trilinos_ENABLE_EXAMPLES="ON"
  ...

...

Explicitly enabled packages on input (by user):  ... Tpetra ... 12

...

Enabling subpackages for hard enables of parent packages due to Trilinos_ENABLE_<PARENT_PACKAGE>=ON ...

...
-- Setting subpackage enable Trilinos_ENABLE_TpetraTSQR=ON because parent package Trilinos_ENABLE_Tpetra=ON
-- Setting subpackage enable Trilinos_ENABLE_TpetraCore=ON because parent package Trilinos_ENABLE_Tpetra=ON
...

Disabling subpackage tests/examples based on parent package tests/examples disables ...

-- Setting TpetraTSQR_ENABLE_EXAMPLES=OFF because parent package Tpetra_ENABLE_EXAMPLES=OFF
-- Setting TpetraCore_ENABLE_EXAMPLES=OFF because parent package Tpetra_ENABLE_EXAMPLES=OFF

Enabling all tests and/or examples that have not been explicitly disabled because Trilinos_ENABLE_[TESTS,EXAMPLES]=ON ...

...
-- Setting TpetraTSQR_ENABLE_TESTS=OFF
-- Setting TpetraCore_ENABLE_TESTS=OFF
...

This definingly looks like a defect (and a missing test case in TriBITS). I will take a closer look.

[maartenarnst]

@bartlettroscoe, thanks!

[bartlettroscoe]

FYI: With the merge of PR #11099, the subpackage test enable behavior bug described above should be fixed.

[csiefer2]

@maartenarnst How much of this issue still needs to be fixed?

[maartenarnst]

Hi @bartlettroscoe and @csiefer2. Thanks for following up on this issue. And thanks for the fix in TriBits!

There were three items in this issue.

The item of the subpackage test enable behavior was solved in Roscoe's PR. I also confirmed in our build that the fix works!

The other two items are still open:

• the file core/test/CrsMatrix/CrsMatrix_createDeepCopy.cpp is not used: I am unsure what to do; the Tpetra team is probably best placed to decide whether this file can be deleted.
• the test related to core/test/CrsMatrix/Tpetra_Test_CrsMatrix_WithGraph.hpp is missing the ROCm case: this can easily be solved by adding a new file Tpetra_Test_CrsMatrix_WithGraph_HIP.cpp along the lines of the existing file Tpetra_Test_CrsMatrix_WithGraph_Cuda.cpp and then adding the new file in the CMakeLists.txt. I have made such a modification in our build, compiled with ROCm 5.2, and confirmed that all goes well. I submit this as a PR to close this?

[csiefer2]

@maartenarnst PR posted.

[github-actions[bot]]

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity. If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label. If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE. If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.

[github-actions[bot]]

This issue was closed due to inactivity for 395 days.