Repository navigation
Conversation
newling
added
project: none
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
tony-davis
left a comment
tony-davis
left a comment
There was a problem hiding this comment.
Hi James! Approving. This is a nice, contained change, and I like getting GoogleTest in place before the bigger HostNumerics suites land. I checked that every old require maps one-to-one onto a GoogleTest assertion, so nothing got weaker in the move. The superbuild match checks out too (same 1.17.0 URL and hash as cmake/modules/shared_third_party.cmake). None of the below is blocking; take or leave as you see fit.
1. The compound assertions only partly get you the better failure messages. scalar_type_test.cpp:48 still ASSERT_TRUEs a nine-term && chain, so a failure says the whole expression was false but not which field was wrong. I think splitting these into EXPECT_EQ(half.exponentBits, 5) and friends would print expected vs. actual and keep checking the rest. The loop at :97 is probably the biggest win, since right now a failure doesn't say which type was wrong. EXPECT_EQ(scalarTypeName(type), name) would print both names.
2. Could HOSTNUMERICS_SOURCE_DIR have a default? Before this PR, cmake -S shared/hostnumerics/tests/embedded just worked. Now the package and embedded projects stop with an error unless you pass the flag (embedded/CMakeLists.txt:8). Something like set(HOSTNUMERICS_SOURCE_DIR "${CMAKE_CURRENT_LIST_DIR}/../.." CACHE PATH "...") would keep the old behavior and still allow an override. If a code owner asked for the explicit form, ignore me!
3. The "embedding doesn't pull in GoogleTest" check might only catch half the cases. embedded/CMakeLists.txt:15 checks TARGET GTest::gtest after add_subdirectory. My understanding is that imported targets are only visible in the directory that created them. So if someone later adds a find_package(GTest) inside HostNumerics, I bet the parent wouldn't see it, and only the download path gets caught. Checking DEFINED CACHE{HOSTNUMERICS_ENABLE_FETCH} would catch the helper being included at all.
4. The installed-GoogleTest path isn't in CI yet. Every CI job downloaded 1.17.0, so the find_package(GTest 1.12.1) path (HostNumericsTestDependencies.cmake:11) has only been checked locally. The runners are ubuntu-24.04, which I think ships libgtest-dev 1.14.0. A matrix entry with that package and HOSTNUMERICS_ENABLE_FETCH=OFF would cover it. Also, a quick note on why 1.12.1 is the minimum would help; the current comment ("Basic assertions work with older installed packages too") doesn't quite say.
Later (not for this PR):
- The MSVC runtime branch in the helper (
:21) hasn't run anywhere yet, since HostNumerics CI is Linux-only. Probably worth a Windows lane before hipBLASLt clients start using HostNumerics on Windows. - Once a file has more than one test case,
gtest_discover_testswould get ctest to report each case on its own.
Thanks for doing this one as a separate small PR. It made it really easy to review.
|
Thanks @tony-davis for the useful review!
I have this in #10553, my hesitation to adding it to develop immediately is that I've had a bad experience with github free runners on Windows in the past, they aren't as fast/abundant and can cause CI delays. Will reconsider at some point. And thanks @Alex-Vasile -- I've updated the readme and am addressing your early return comment now |
TheRock Submodule Bump ActivityNewest first
|
JIRA ID : AIHPBLAS-4195 Much of what HostNumerics will do revolves around the shapes of tensors/matrices. This PR introduces a class to encapsulate some core logic in an intuitive way. The class added (Shape) is exposed to python, where most testing is done. The class is not designed to squeeze out maximum performance, but rather to be usable. To this end for example, methods do bound checking and throw exceptions where appropriate. ## Technical Details `Shape` owns its extents and provides dimension access, checked element counts over half-open ranges, projection, and conversion between coordinates and logical linear indices. A rank-zero shape represents one scalar. `project([2, 0])` turns `[2, 3, 4]` into `[4, 2]`; an empty projection produces a scalar. A zero extent takes precedence over product overflow, so `[SIZE_MAX, 2, 0]` counts as empty, while a nonzero overflowing product throws. Index orders accept axes directly through `std::span<const size_t>`, listed from slowest to fastest. The default identity order matches NumPy's C order; reversing the axes gives Fortran order. For shape `[2, 3, 4]`, coordinate `[1, 0, 2]` maps to index 14 by default and index 13 with order `[2, 1, 0]`. Invalid permutations, ranks, coordinates, and arithmetic produce descriptive exceptions. Counts and index conversions perform checked, rank-dependent work; callers should reuse computed results in performance-critical element loops. Python exposes the complete logical interface, accepts lists and tuples, returns copied extents, and translates C++ errors into Python exceptions. Behavioral tests compare products with exact Python integer arithmetic and enumerate coordinate permutations independently, including NumPy comparisons. Native tests cover constructor ownership and caller-provided output spans. These checks share one executable with the existing scalar/version tests, and `gtest_discover_tests` reports each case individually in native, installed-package, and embedded builds. The PR builds on the GoogleTest setup merged in #13562. The new header is installed and exported through `roc::hostnumerics-core`. Python sanitizer coverage runs in the existing ASan/UBSan job, with NumPy added to the test dependencies. ## Validation Local GCC 13 and Clang 23 native/Python tests, Clang 18 ASan/UBSan native/Python tests, installed C++ consumers from producers with testing/Python enabled and disabled, installed Python tests, and repeated embedded configuration pass. Root formatting/lint checks, actionlint, and the component clang-tidy 18 scan pass. Windows execution and hosted CI for the rebased revision remain unverified. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests. ## Risk level Low: a standalone API without product or GPU consumers. Axis order, scalar/empty semantics, and exception behavior establish public conventions and have focused coverage.
JIRA ID : AIHPBLAS-4195
HostNumerics currently runs its C++ tests through handwritten
mainfunctions and an assertion helper that throws. This makes failures less informative and leaves the initial tests outside the framework intended for later native tests: The complete branch #10553 uses gtest for most of it's C++ testing, so I'm delivering that now in this PR and porting the custom testing to use gtest.Further information from an LLM:
Motivation
Use a consistent framework for native tests and installed-package and embedded smoke tests, with failures that identify the test and assertion location. This establishes GoogleTest for the initial tests before later native suites are added.
Technical Details
The existing version and scalar-type tests become named GoogleTest cases, retaining their coverage and separate source files. Their executables link to
GTest::gtest_main; installed-package and embedded smoke tests reuse the version test.A shared CMake helper accepts installed GoogleTest 1.12.1 or newer, or fetches SHA-256-pinned 1.17.0 to match the repository's shared superbuild.
HOSTNUMERICS_ENABLE_FETCH=OFFrequires an existing GoogleTest package. Each test project loads the helper by module name; the standalone package and embedded harnesses receive an explicitHOSTNUMERICS_SOURCE_DIR, including in CI. GoogleTest is loaded only for test builds and is absent from the installed HostNumerics package.Test Plan
Build and run the native and Python tests with Clang and GCC, then run the native tests with Clang address and undefined-behavior sanitizers. Exercise installed-package consumers from producers with testing and Python enabled and disabled, and check embedded defaults. Verify both the default GoogleTest fetch path and an installed GoogleTest package with fetching disabled, including dependency isolation when HostNumerics tests are disabled. Run clang-tidy and the root pre-commit checks.
Test Result
Recorded local validation on this PR's current revision passes with Clang 23 and GCC 13, including native and Python tests, Clang sanitizer tests, installed-package consumers, and embedded smoke tests. The fetched GoogleTest 1.17.0 archive and hash, installed GoogleTest with fetching disabled, and test-disabled dependency isolation checks pass. clang-tidy 18 and root pre-commit checks pass. Windows execution remains unverified.
Submission Checklist