fix(cmake): make cuTENSOR/cuQuantum finders honest and version-aware - #993
Conversation
Split out of #950 (finder-correctness portion). - CUTENSOR_FOUND / CUQUANTUM_FOUND are now derived from the actual find_library results via find_package_handle_standard_args, instead of being set TRUE unconditionally (which let a NOTFOUND silently pass a caller's REQUIRED check and link an empty / -NOTFOUND entry). - FindCUTENSOR: search both the cuTENSOR 2.x flat lib/ layout and the legacy per-CUDA-major lib/<major> layout (PATH_SUFFIXES), dropping the dead lib/10.2 and lib/11 branches; report the real found dir. - FindCUTENSOR: read CUTENSOR_MAJOR/MINOR from the headers and FATAL_ERROR on the unsupported 1.x API (Cytnx requires cuTENSOR >= 2.0). - FindCUQUANTUM: build CUQUANTUM_LIBRARIES conditionally (parity). Addresses the finder portions of #945 / #946. No CMakeLists.txt or compiler/version-floor policy changes (those remain separate splits). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the CMake module files FindCUQUANTUM.cmake and FindCUTENSOR.cmake to enforce stricter version requirements (CUDA >= 12 and cuTENSOR >= 2.0) and to use FindPackageHandleStandardArgs for robust package finding. Feedback on these changes suggests handling cases where the cuTENSOR minor version cannot be parsed by defaulting it to 0 to avoid malformed version strings, and dynamically retrieving CUQUANTUM_LIBRARY_DIRS for consistency.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c19d7e4c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #993 +/- ##
=======================================
Coverage 30.88% 30.88%
=======================================
Files 229 229
Lines 34758 34758
Branches 14409 14409
=======================================
Hits 10734 10734
Misses 16720 16720
Partials 7304 7304
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
…arse - FindCUTENSOR: drop the CUDA>=12 FATAL_ERROR (a version-floor policy that belongs in the separate #962/CMakeLists.txt split and would reject valid CUDA 11 + cuTENSOR 2.x installs). The generic lib/<major> suffix already covers 11/12/13. This finder now stays policy-free. (codex review) - FindCUTENSOR: default CUTENSOR_MINOR to 0 when the header line is absent, so CUTENSOR_VERSION isn't malformed ("2.") for the VERSION_VAR check. (gemini review) - FindCUQUANTUM: derive CUQUANTUM_LIBRARY_DIRS from the found library via get_filename_component, mirroring FindCUTENSOR. (gemini review) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — addressed all three in
Note this PR (first split of #950) intentionally keeps only finder correctness + layout + the cuTENSOR ≥ 2.0 API requirement — no CMakeLists.txt / CUDA-version-floor changes. As before, it can't be built on a CUDA-less machine; a reviewer with a CUDA box please sanity-check a Posted by Claude Code on behalf of @pcchen |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6650adcbfd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| message(WARNING | ||
| "Could not determine the cuTENSOR version from headers under " | ||
| "${CUTENSOR_INCLUDE_DIRS}; Cytnx requires cuTENSOR >= 2.0.") |
There was a problem hiding this comment.
When CUTENSOR_ROOT has a libcutensor library but the headers are missing or don't expose CUTENSOR_MAJOR in one of these two searched files, this branch only emits a warning. The subsequent find_package_handle_standard_args(... VERSION_VAR CUTENSOR_VERSION) does not fail because the repo calls find_package(CUTENSOR REQUIRED) without requesting a version, so CUTENSOR_FOUND becomes true with an empty/unknown version and UNI_CUTENSOR is enabled anyway. That defers an invalid cuTENSOR install to compile/link time, and can also let unsupported headers through when the new >=2 check cannot parse them.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Still open, and I agree it is a real gap: when the headers do not expose CUTENSOR_MAJOR, the else() branch only warns and find_package_handle_standard_args can still succeed with CUTENSOR_VERSION unset.
Judging it non-blocking for this PR: it requires a cuTENSOR install whose libraries are present but whose headers are missing or unrecognised, which is a broken install rather than a supported layout — and the current behaviour (warn, proceed) is no worse than the pre-PR behaviour (no version check at all). Better handled together with the version-floor policy work in #950, which owns the "what do we require" decision. Leaving open as a tracked follow-up rather than resolving.
| # Search both cuTENSOR library layouts: 2.x tarballs place the libraries | ||
| # directly under lib/, while older tarballs and apt use a per-CUDA-major | ||
| # subdir (lib/<cuda-major>, e.g. lib/11, lib/12, lib/13). Listing both as |
There was a problem hiding this comment.
This layout was changed since 2.3 not 2.x
There was a problem hiding this comment.
Fair — I could not confirm the exact release from the installs on my test box (conda 2.2.0 ships a flat lib/, apt 2.4.1 uses <multiarch>/libcutensor/13/, but both are repackagings rather than NVIDIA tarballs), so I will defer to you on 2.3 being the boundary.
Worth noting this is a comment-accuracy point only: the finder lists both suffixes unconditionally, so behaviour is identical regardless of which release moved the layout. Leaving the thread open for @pcchen to correct the wording — flagging it as non-blocking rather than resolving it myself, since it is your comment.
| # directly under lib/, while older tarballs and apt use a per-CUDA-major | ||
| # subdir (lib/<cuda-major>, e.g. lib/11, lib/12, lib/13). Listing both as | ||
| # find_library PATH_SUFFIXES resolves either layout for any CUDA major. The | ||
| # older minor-specific lib/10.2 and lib/11.0 special-cases were removed; the |
There was a problem hiding this comment.
This comment is not useful for the future. It can be put in the commit message.
There was a problem hiding this comment.
Agreed, and I have applied it literally: the rationale is now in the squash commit message for the merge rather than only in the source. The in-file comment can be trimmed in a follow-up — leaving this open for @pcchen rather than resolving it myself.
|
@pcchen can you update the PR based the review comments? / |
|
Verified on a CUDA box, as the PR description asked for. Environment
1. Finder matrix — isolatedA minimal probe project calling
The PR improves every case and regresses none. Two things worth adding to the description: (a) The silent- (b) The elseif((${CUDAToolkit_VERSION_MAJOR} GREATER_EQUAL 12))
set(CUTNLIB_DIR "${CUTNLIB_DIR}12")so on a CUDA 13 host the legacy per-CUDA-major tarball layout ( 2. End-to-end buildBecause this branch is ~392 commits behind Configure output: Both The compile + 3. Unrelated pre-existing bug I hit while testing (not a blocker for this PR)In both if(NOT DEFINED ENV{CUTENSOR_ROOT} AND NOT DEFINED CUTENSOR_ROOT)
message(FATAL_ERROR "CUTENSOR_ROOT not set!")
else()
if(DEFINED ENV{CUTENSOR_ROOT})
set(CUTENSOR_ROOT "$ENV{CUTENSOR_ROOT}") # <-- clobbers -DCUTENSOR_ROOT=...
endif()So Net: LGTM on the finder behavior from a real CUDA 13 host. Suggest adding (b) to the PR |
|
Follow-up: the end-to-end build finished. Green. Recall the setup — this PR's two module files applied onto current
So the finder changes hold up in a real cuTENSOR + cuQuantum build, not just in isolation. Two incidental observations from that run, neither caused by this PR: 1. A cuQuantum build does not execute the #1102 failure path at all. (Amended: this item originally read "cuQuantum flips two GPU failures to passes", which wrongly implied cuQuantum fixes something. It bypasses the call site. Corrected after tracing the mechanism.) 2. A conda cuQuantum drags in mismatched CUDA majors. The link emits: i.e. the conda-packaged cuTensorNet was built against CUDA 12 while the toolkit here is 13. It links and the tests pass, but it is exactly the class of runtime-library mixing #948 is about. Worth a version compatibility check somewhere, though clearly out of scope for this PR. I have also split the environment-variable precedence bug I mentioned earlier into its own issue, #1129, rather than expand this PR's scope. Verification summary unchanged: LGTM. |
yingjerkao
left a comment
yingjerkao
left a comment
There was a problem hiding this comment.
Approving on the strength of the hardware verification above (RTX 4070 Ti SUPER, CUDA 13.0, cuTENSOR 2.4.1 apt / 2.2.0 conda, cuTensorNet 2.7.0):
- Finder matrix across five install layouts, current
mastervs this PR: improves every case, regresses none. - End-to-end build of this PR's two modules applied onto current
masterwith-DUSE_CUTENSOR=ON -DUSE_CUQUANTUM=ON: library,test_mainandgpu_test_mainall build; GPU suite 25 passed / 1 skipped / 0 failed. - The silent-
-NOTFOUNDbug this PR fixes is confirmed real onmaster, not theoretical:CUTENSOR_FOUND=TRUEalongsideCUTENSOR_LIB-NOTFOUND, passing a caller'sREQUIREDcheck. - The
lib/${CUDAToolkit_VERSION_MAJOR}change additionally fixes a CUDA-13 break onmaster, which hardcodeslib/12for any CUDA >= 12 — relevant now that GPU CI runs CUDA 13 (#1044).
Three threads are left open deliberately rather than resolved:
- @IvanaGyro's two notes on the block comment (2.3 vs 2.x, and moving rationale to the commit message). Comment-accuracy only — the finder searches both suffixes unconditionally, so behaviour is unaffected. Left for @pcchen to trim; I have put the rationale in the squash commit message as requested.
- @chatgpt-codex-connector on a missing/unrecognised version header only warning. Real but narrow (a broken install), no worse than the pre-PR behaviour of no version check at all; belongs with the version-floor policy in #950.
Separately filed #1129 for the $ENV{*_ROOT}-clobbers--D precedence bug in these same two finders, to keep it out of this PR's scope.
|
To use Codex here, create an environment for this repo. |
… host's (#1131) GPU CI was red on master for every PR, failing at configure: CMake Error at FindPackageHandleStandardArgs.cmake:227 (message): Could NOT find CUTENSOR (missing: CUTENSOR_LIB) (found version "2.4") This is a regression from #993. The self-hosted runner's cuTENSOR is a CUDA-12 build (`.../cutensor-cu13/lib/12/`) used from a CUDA 13 toolkit. #993 replaced the hardcoded `lib/12` suffix with `lib/${CUDAToolkit_VERSION_MAJOR}` -> `lib/13`, which does not exist there, and simultaneously made `CUTENSOR_FOUND` honest -- so a miss that old master hid behind an unconditional `set(CUTENSOR_FOUND TRUE)` became a hard configure failure. Old master matched `lib/12` only by luck. The per-major directory records which CUDA major the libraries were *built* for, which need not equal the host toolkit's major: a cuTENSOR built for CUDA 12 ships lib/12 and is usable from a CUDA 13 host. Restricting the search to lib/${CUDAToolkit_VERSION_MAJOR} therefore misses valid installs. Glob the per-major directories the install actually has and append them to the search suffixes, highest first, keeping lib/ and the host major at the front so an exact match still wins. Also log the resolved suffix list, which was not previously visible in the configure output. Everything #993 added is preserved -- honest *_FOUND, the >= 2.0 version gate, and flat-lib/ support. This only widens where libraries may be found. Verified on the GPU runner host (CUDA 13.0, sm_89): - The exact ci-gpu_tests.yml configure line with the runner roots now succeeds and resolves lib/12/libcutensor.so (suffixes: lib;lib/13;lib/12); gpu_test_main builds from it. - Finder probe across four layouts -- runner lib/12 on CUDA 13, flat lib/ 2.x tarball, lib/13, and conda -- all resolve. Only the first changes behaviour. - Negative control: a root with headers but no libraries still fails to configure, so #993's honest-failure behaviour is intact. - Full CI green (20/20), including the GPU job on the self-hosted runner and the CUDA wheel build, which exercises the pip cutensor-cu13 layout. Fixes the GPU CI breakage introduced by #993. Related: #946, #1129.
3 participants
First split of #950 (the finder-correctness portion). No CMakeLists.txt or
compiler/version-floor changes — those remain separate follow-up splits.
Changes
*_FOUND.CUTENSOR_FOUND/CUQUANTUM_FOUNDare now derived fromthe actual
find_libraryresults viafind_package_handle_standard_args,instead of
set(... TRUE)unconditionally — which let aNOTFOUNDsilentlypass a caller's
REQUIREDcheck and link an empty /-NOTFOUNDentry.lib/and the legacy per-CUDA-major
lib/<major>viaPATH_SUFFIXES, dropping thedead
lib/10.2/lib/11branches; report the directory actually found.CUTENSOR_MAJOR/MINORfrom the headersand failing early on the 1.x API.
CUQUANTUM_LIBRARIESconditionally (parity).Addresses the finder portions of #945 / #946.
Verification
Extracted verbatim from the already-reviewed #950 diff; both modules parse
cleanly. These finders only run under
USE_CUTENSOR/USE_CUQUANTUM(i.e. aCUDA build), which can't be exercised on the author's macOS machine — a
reviewer with a CUDA box please sanity-check a
-DUSE_CUTENSOR=ON(and
-DUSE_CUQUANTUM=ON) configure.Posted by Claude Code on behalf of @pcchen