docs: add build-test-workflow and cross-revision-benchmark skills#1039
Merged
Conversation
Add an agent skill covering fast edit-compile-test iteration: accumulated incremental builds (rebuild one target in an existing build dir, never delete build/), running ctest and pytest against already-built binaries, per-suite --gtest_filter iteration with the ASAN_OPTIONS workaround, how much of the suite to run while iterating vs before push, the requirement that a regression test be shown to fail on the pre-fix code, and the link line for compiling a standalone verification harness directly against libcytnx.a. Co-Authored-By: Claude <noreply@anthropic.com>
Add an agent skill for fair performance comparisons across git revisions: keep one fixed benchmark source that calls the library entry point directly, check out only the differing src/include files per revision and rebuild just the release library, run Google Benchmark with repetitions and aggregate reporting, and restore the working tree afterwards. Reporting rules: default to merge-base vs tip, name the exact commit for every column, run all columns in the same session on the same machine, state the aggregation, and call out baselines whose bugs change how much work they do. Co-Authored-By: Claude <noreply@anthropic.com>
…skills Point CLAUDE.md's Test section at the two agent skills that carry the day-to-day mechanics, so agents invoke them instead of re-deriving build, test, and benchmarking commands. Co-Authored-By: Claude <noreply@anthropic.com>
Add a guardrail to CLAUDE.md: changing which of several equal-cost candidates a greedy/heuristic planner picks (e.g. the contraction order chosen by OptimalTreeSolver) changes user-visible results even though each choice is individually valid, so such changes must be flagged for sign-off like physics/numerical changes, and a reviewer's counterexample must be reproduced on both revisions before it is accepted or disputed. Mirror the point in GEMINI.md's review priorities so reviews call out tie-break/selection-order changes instead of silently accepting them, keeping the two files in sync per CLAUDE.md's own instruction. Co-Authored-By: Claude <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces two new agent skills, build-test-workflow and cross-revision-benchmark, to document fast iteration and performance comparison procedures. It also updates CLAUDE.md and GEMINI.md to reference these skills and adds guidelines regarding heuristic tie-breaks in planners. The review feedback is highly constructive, pointing out a linker order issue in the standalone compilation example, a potential git checkout failure for newly introduced files, and an inconsistency in the benchmark executable name. All of these suggestions should be addressed to improve the accuracy and robustness of the documentation.
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.
Gemini review on PR #1039 flagged three inaccuracies, all confirmed: - build-test-workflow's standalone harness link line placed -lopenblas before -llapacke/-larpack. Verified empirically that both orders link on this machine (all three are shared libraries here), but the correct and portable convention -- valid for static archives too, where order is not optional -- is dependents before the library they call into. Reorder to -larpack -llapacke -lopenblas -lgomp and say why. - cross-revision-benchmark's step 1 collected "files that differ" between two revisions and step 2 checked them out at each revision. Verified that `git checkout <rev> -- <path>` fails with a pathspec error when <path> was added by <tip> and does not exist at <rev>, and that checking out the containing directory instead avoids the error but does not delete the file -- silently leaving <tip>'s version of it in <rev>'s build. Split the diff by status (M vs A/D) and restrict the checkout-in-place recipe to M files; point A/D cases at the worktree fallback the Preconditions section already names for interface changes. - The harness output name in step 2's benchmark invocation (./bench) did not match the -o harness used in build-test-workflow's link line. Renamed to ./harness for consistency between the two skills. Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1039 +/- ##
==========================================
+ Coverage 55.63% 57.71% +2.07%
==========================================
Files 228 229 +1
Lines 32896 33468 +572
Branches 71 71
==========================================
+ Hits 18303 19315 +1012
+ Misses 14573 14132 -441
- Partials 20 21 +1
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:
|
# Conflicts: # GEMINI.md
Collaborator
|
Posted by Claude Code on behalf of @pcchen ReviewGood additions — worth landing. I verified every command, target name, path, Four accuracy issues. The common thread for 1–3: each is a command or file 1.
|
… checkout @pcchen's review on PR #1039 verified every command, target, path, and preset name against the repo and found four accuracy issues, all confirmed: - cross-revision-benchmark's Preconditions example cited benchmarks/search_tree_bm.cpp, which does not exist on master (it lives on the unmerged #853/#1027 branch). Replaced with benchmarks/Tensor_bm.cpp, which does exist, and added a note that a comparison needs an existing benchmark file covering the function -- add one first rather than writing comparison code inline in a scratch harness. - Both skills presented `ninja libcytnx.a` as equivalent to `cmake --build --target cytnx`. Confirmed no preset sets `generator`, and `cmake --system-information` on this machine reports the platform default as Unix Makefiles, not Ninja, even with Ninja installed -- so a stock preset configure does not produce a build.ninja file, and `ninja libcytnx.a` (wrong target name for Make besides) only works if that specific build dir happens to have been configured with -G Ninja. Kept only the generator-agnostic `cmake --build --target cytnx` form as the one safe to assume, and qualified the direct-generator shortcut with how to check which generator a build dir actually has. - cross-revision-benchmark's step 2 offered the `benchmarks_main` CMake target as an alternative to the standalone harness with no caveats. Confirmed RUN_BENCHMARKS defaults OFF in every preset (CMakeLists.txt gates add_subdirectory(benchmarks) behind it) and also requires Google Benchmark installed via find_package(REQUIRED), so the target does not exist in a stock preset-configured build dir. Reordered to present the harness as the default path and note benchmarks_main's reconfigure precondition. - The harness link line's `-I build/<preset>` was justified as "covers generated headers". Confirmed the only configure_file in the build is the Doxyfile (docs-only) and no build dir contains a generated header outside CMakeFiles/. Rephrased as harmless-but-currently-unnecessary instead of a false justification. Also folded in the two minor suggestions: a macOS note (Homebrew paths, -lomp in place of -lgomp) on the Linux-specific harness link line. Co-Authored-By: Claude <noreply@anthropic.com>
IvanaGyro
commented
Jul 11, 2026
Member
Author
There was a problem hiding this comment.
- Consider to implement some scripts for all platform to cover these use cases. So that the bot can directly run the script to build stuff instead of composing the command. Notice that, the the build should use max CPUs, and the generator should be consistent between pip install and cmake. The script should also handle venv properly
- build preset openblas-cpu (pip editable install/gtest only)
- build preset mkl-cpu (pip editable install/gtest only)
- build preset debug-openblas-cpu (pip editable install/gtest only)
- Verify if turn RUN_BENCHMARKS on after building will trigger the full rebuild. If it will rebuild, always turn it on.
Review on PR #1039 asked for a script the bot can run directly instead of composing pip/cmake commands by hand, covering openblas-cpu, mkl-cpu, and debug-openblas-cpu in two modes: a pip editable install (enables pytest) and a direct cmake configure+build (gtest/ctest only, no wheel). Requirements: max-CPU builds, a generator consistent between the pip and cmake entry points, and proper venv handling. Add tools/build_preset.sh: - `--pytest` creates (or reuses) a venv at build/<preset>-venv and runs `pip install --editable '.[dev]'` pinned to that preset's build dir. - `--gtest-only` runs `cmake --preset <preset>` + `cmake --build --target test_main` directly, with BUILD_PYTHON=OFF on a fresh configure only (a plain `cmake --preset` needs pybind11 discoverable, which isn't available outside a venv, but --gtest-only doesn't need the Python target at all; an existing dir a --pytest run already configured with BUILD_PYTHON=ON is left untouched). - Both modes read the CPU count via nproc/sysctl and pass --parallel/-j. - The generator is never forced against an already-configured build dir (only a --gtest-only reconfigure error, from a genuinely mixed dir, would need a -G at all); a brand-new dir defaults to Ninja, matching pip/scikit-build-core's own default preference, so a --pytest and a --gtest-only build of the same fresh preset converge on the same generator without needing to agree in advance. Tested end to end: --gtest-only on a fresh mkl-cpu dir builds test_main; --pytest on an existing debug-openblas-cpu dir builds a working venv (pytest passes against it) without touching that dir's already-Ninja generator; --gtest-only against an existing --pytest-configured dir leaves BUILD_PYTHON=ON as found. Also verified empirically (fully-built build/openblas-cpu, `ninja -n` before/after each toggle): flipping RUN_BENCHMARKS OFF then back ON from a clean baseline triggers zero pending build steps in either direction. It is a metadata-only change, not a rebuild trigger, so the script always turns it (and RUN_TESTS) on rather than requiring a later reconfigure to enable them. Update build-test-workflow to match: - Point to tools/build_preset.sh as the default way to build a preset, and drop the "delete the build dir and reconfigure" recovery advice for a generator/cache mismatch -- recovery is building with --pytest first (so the dir starts on Ninja) or reconfiguring with no -G flag to keep the existing generator, never deleting a working dir. - Scope the ASAN_OPTIONS workaround to the debug+CUDA presets specifically, not debug presets generally. - Show running pytest against a specific preset's venv (source build/<preset>-venv/bin/activate) instead of a bare `pytest pytests/` with no stated venv. - Add running the GPU test suite (gpu_test_main, ctest --preset cpu-and-cuda) when a GPU is actually present, alongside the existing GPU-less compile-check guidance. Co-Authored-By: Claude <noreply@anthropic.com>
IvanaGyro
commented
Jul 11, 2026
….md Test Three more review points on PR #1039: 1. cross-revision-benchmark's Preconditions punted a changed-API comparison to "use worktrees instead" with no concrete procedure, and its checkout recipe was file-scoped (split by git diff --name-status) specifically to avoid a pathspec error on added files. Verified empirically (isolated git worktree, unrelated to any working branch): a full `git checkout <rev>` between two commits with a 5-file, 460-line diff -- including one newly-added file -- left only 7 build steps pending in an already-built dir, the same incremental scope as the file-scoped approach. Git only rewrites files whose blob content actually differs, so a whole-tree checkout is exactly as cheap and additionally handles added/removed files correctly with no pathspec error and no directory-checkout-leaves-stale-files trap. Rewrote the procedure around a git worktree per revision on that basis: two named cases (Case A, benchmark source held fixed across revisions -- overlay the tip's benchmark file *and* benchmarks/CMakeLists.txt, verified the CMakeLists.txt overlay is required or the new file silently isn't registered in the build, by building the #853/#1027 merge-base commit, which predates search_tree_bm.cpp, both with and without it; Case B, each revision's own benchmark source, for a changed interface). Worktrees also let independent revisions' builds run in parallel and never touch the current branch, removing the old restore-at-the-end step. 2. The run step now explicitly covers both benchmark forms: the committed Google Benchmark target (benchmarks_main) for any actual Cytnx function, and the standalone harness reserved for a language/stdlib question with no Cytnx dependency (e.g. std::vector vs std::unordered_set) that doesn't belong as a permanent benchmarks/*.cpp file. Builds may run in parallel across worktrees; benchmark runs may not, noted explicitly (CPU contention between simultaneous runs skews results). 3. Reporting rules now require a commit hash *and* a short readable label together for every column -- a bare hash goes stale the moment history is rewritten (rebase/squash/force-push), while the label is what the comparison is actually about and survives that. Also: CLAUDE.md's Test section duplicated the build-test-workflow skill's now-more-detailed mechanics (tools/build_preset.sh supersedes the raw preset commands it had inline). Trimmed it to match the Coding style section's existing pattern -- skill pointers plus a condensed list of the essential project facts (RUN_TESTS requirement, ASan scoped to debug+CUDA, gpu_test_main vs test_main, GPU-only-if-present, the pip/pytest invocation) -- and noted that Claude Code's .claude/skills/ auto-discovery is not necessarily available to every agent this file serves (e.g. Codex), but the files remain plain markdown any agent can open directly, so the condensed facts are a fallback rather than the file losing this information for agents that can't invoke the skill. Co-Authored-By: Claude <noreply@anthropic.com>
IvanaGyro
commented
Jul 11, 2026
IvanaGyro
left a comment
IvanaGyro
left a comment
Member
Author
There was a problem hiding this comment.
Remove standalone benchmark case. Limit the skill to the benchmark will be committed.
Compact the skills. They are too long now.
…ild+test entry point Move tools/build_preset.sh under the skill directory (.claude/skills/build-test-workflow/scripts/) and replace its four separate run modes with one interface: `build_preset.sh <preset> [--target <target>] [--test [args...]]`. - `--target` defaults to `all` and picks whether the Python venv/pip-install path is needed (only `all`/`pycytnx` do); every other target skips Python setup and goes straight to `cmake --build`. - The pip-install path now always pins `--config-settings=cmake.args= --preset=<preset>`. pyproject.toml hardcodes `cmake.args = ["--preset=openblas-cpu"]` as its own scikit-build-core default, so without this override every pip-driven build silently configured as openblas-cpu regardless of the preset the caller asked for. - `--test [args]` runs the target's tests: pytest args pass through verbatim for a Python target (a path/-k filter replaces the default `pytests/` collection rather than adding to it), and a single arg becomes `--gtest_filter=<value>` against tests/test_main otherwise. - `debug-*-cuda` presets get ASAN_OPTIONS='protect_shadow_gap=0: replace_intrin=0:detect_leaks=0' exported automatically before running test_main, instead of requiring the caller to remember the workaround. - A build dir's generator is decided once by whichever call configures it first; the script never reconfigures or deletes an existing dir to fix a generator/cache mismatch, and never touches BUILD_PYTHON on a dir that is already configured. Update build-test-workflow/SKILL.md to document this single entry point in place of the four prior standalone command blocks, and to point the standalone-harness section at cross-revision-benchmark for any benchmark of an actual Cytnx function rather than describing that case inline. Co-Authored-By: Claude <noreply@anthropic.com>
…ons, drop standalone-harness case Switch the procedure from one worktree/build dir per revision (built in parallel) to one dedicated worktree with a single shared build dir, switched sequentially between revisions via `git checkout <rev>` in place. A separate build dir per revision throws away the incremental-rebuild benefit a shared dir gets from git only rewriting files whose blob content actually changed between revisions, and running builds in parallel contends for the same CPUs a sequential benchmark run also needs undisturbed. Also remove the standalone-harness / language-feature-benchmark case: the skill now only covers benchmarking a committed `benchmarks/*.cpp` (the `benchmarks_main` target), not comparison code written in a scratch harness. Compact the reporting-rules and case-selection sections. Co-Authored-By: Claude <noreply@anthropic.com>
…enchmark target support Replace build_preset.sh's non-Python --test path (a direct tests/test_main invocation with --gtest_filter) with `ctest --test-dir <build_dir> --output-on-failure`. test_main and gpu_test_main each register their gtest cases as individual ctest tests via gtest_discover_tests, so ctest gives per-test pass/fail output and matches what CI runs, instead of dumping one binary's raw combined output. --test-dir is used rather than --preset: CMakePresets.json's testPresets are hardcoded to only debug-openblas-cpu/debug-openblas-cuda and don't generalize to every preset the script accepts. A single --test arg now becomes `-R <value>` (a ctest regex against ClassName.TestName) rather than a gtest glob/`:`-joined filter. Add a `--target benchmarks_main` case: since the Google Benchmark binary has no ctest/add_test registration, --test runs it directly with the given args passed through as its own CLI flags (e.g. --benchmark_filter=...). Let --target accept a space-separated list (e.g. "test_main gpu_test_main") so the CUDA test suite's two binaries can be built in one call; cmake --build's --target flag itself accepts multiple names. Update build-test-workflow/SKILL.md to document the ctest/benchmark behavior and route the GPU-suite instructions through the script instead of raw cmake/ctest invocations, so ASAN_OPTIONS and the other per-preset handling the script already does aren't duplicated by hand. Co-Authored-By: Claude <noreply@anthropic.com>
… commands - Point to build_preset.sh's actual location under .claude/skills/build-test-workflow/scripts/ instead of the old tools/ path. - Replace the `ctest --preset cpu-only` fallback with `ctest --test-dir build/<preset> --output-on-failure`: the named testPreset is hardcoded to debug-openblas-cpu specifically and doesn't generalize to the other presets this fallback is meant to cover. - Fix the pip install fallback: pin both --preset=<preset> and a preset-specific build-dir explicitly instead of the unqualified build-dir=build (pyproject.toml's own default silently configures openblas-cpu regardless of the intended preset), drop the CMAKE_BUILD_TYPE=Debug override since every preset already sets its own CMAKE_BUILD_TYPE, and add the venv create/activate step the command depends on. Co-Authored-By: Claude <noreply@anthropic.com>
…through the script Switch the procedure from a dedicated git worktree to checking out revisions in place in the main working tree. A fresh worktree starts with an empty build dir, so its first build is a full rebuild no matter how small the diff between revisions is -- checking out in place instead reuses whatever build/<preset> the build-test-workflow script has already produced from ordinary development, which is what actually makes the incremental-rebuild property useful. Add the stash-and-restore steps a main-tree checkout needs that a worktree didn't. Route every build and benchmark run through build_preset.sh (--target benchmarks_main, then --target benchmarks_main --test <benchmark args>) instead of raw cmake/binary invocations, so the script's existing generator-reuse and max-parallelism handling apply here too. Drop references to specific issue/PR numbers from the examples in favor of generic descriptions, since a skill file outlives any single PR. Co-Authored-By: Claude <noreply@anthropic.com>
IvanaGyro
marked this pull request as ready for review
July 11, 2026 17:41
![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: 7c91cc7a9b
ℹ️ 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".
…nchmarks_main CMakeLists.txt runs find_package(benchmark REQUIRED) whenever RUN_BENCHMARKS is on, regardless of which target is actually being built. build_preset.sh was passing -DRUN_BENCHMARKS=ON (or the pip equivalent) unconditionally, so a plain test_main or pycytnx build would fail at configure time on any machine that never installed Google Benchmark -- including CI's own native-dependency list, which does not include it. Gate RUN_BENCHMARKS on target == benchmarks_main in both the pip and raw cmake configure paths. For an already-configured, non-fresh build dir that didn't need it before, reconfigure in place with -DRUN_BENCHMARKS=ON when benchmarks_main is requested -- the same zero-cost toggle already relied on for RUN_TESTS, verified here by reusing an existing preset's build dir and rerunning the benchmark target through it. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 40c346055f
ℹ️ 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".
…not just before ctest gtest_discover_tests defaults to POST_BUILD discovery, which runs the freshly built test binary as part of `cmake --build` itself to enumerate its cases. build_preset.sh only exported the debug-*-cuda ASAN_OPTIONS workaround in the later test-run block, so a gpu_test_main binary that needs it could already abort during build-time discovery, before the script ever reached that block -- e.g. `build_preset.sh debug-openblas-cuda --target gpu_test_main --test` would fail at the build step, not the test step. Move the export to right after target/preset parsing, before any configure or build happens, and drop the now-duplicate export from the test-run block. Co-Authored-By: Claude <noreply@anthropic.com>
… invocation CI's pytest step (.github/workflows/ci-cmake_tests.yml) runs `pytest pytests/ --doctest-modules`, and CLAUDE.md documents the same command. build_preset.sh's default (no --test args) Python test path only ran `pytest pytests/`, missing --doctest-modules -- a docstring regression could pass through the script's advertised `--target pycytnx --test` entry point and only surface later in CI. Add --doctest-modules to the no-args default. An explicit filter/path still replaces the default collection verbatim, as documented, and does not get --doctest-modules added automatically. Co-Authored-By: Claude <noreply@anthropic.com>
…next checkout Case A's step 2 leaves benchmarks/<the_bm_file>.cpp and benchmarks/CMakeLists.txt checked out from <tip> and locally modified relative to the revision just benchmarked. Without restoring them, the next iteration's `git checkout <rev>` either refuses (when the local diff conflicts with <rev>'s own committed content) or, when it can 3-way-merge, silently carries <tip>'s content forward onto the wrong revision -- verified both failure modes directly: reproduced a case where the merge succeeds and leaves the wrong (tip's) benchmark file present after switching to an older revision that never had it. Add a step to restore the two overlaid files before moving to the next revision, using `git restore --source=HEAD --staged --worktree` rather than `git checkout HEAD -- <path>`: when <rev> predates the benchmark file existing at all, `checkout HEAD -- <path>` errors with "did not match any file(s) known to git" for a path HEAD doesn't have, while `git restore --source=HEAD` correctly deletes it to match HEAD's (absence) -- verified both the failing command and the working one directly in an isolated worktree. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34df70ad31
ℹ️ 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".
…E-relative path The BASH_SOURCE-relative path count breaks the moment the script is invoked from a copy living outside its normal .claude/skills/build-test-workflow/scripts/ location -- e.g. cross-revision-benchmark needs a stable copy of the script that survives checking out revisions predating the script's own existence in the tree, and the copy's repo-root computation would resolve to the wrong directory entirely. Use `git rev-parse --show-toplevel` instead: it only depends on the caller's cwd being inside the target repo, not on where the script file itself lives. Verified by running a copy from /tmp against this repo while cwd stayed at the real repo root. Co-Authored-By: Claude <noreply@anthropic.com>
…hes the target An existing (non-fresh) build dir was only ever reconfigured in place for --target benchmarks_main (to flip RUN_BENCHMARKS on); every other target just went straight to `cmake --build` with whatever RUN_TESTS state the dir already happened to have. A dir first created without RUN_TESTS=ON (e.g. by a plain `cmake --preset ...` outside the script, or a prior benchmarks_main-only build) has no tests/ subdirectory at all, since CMakeLists.txt only adds it under `if(RUN_TESTS)` -- so `--target test_main --test` failed with "unknown target 'test_main'" instead of building anything. Reproduced directly: build/openblas-cpu had RUN_TESTS=OFF from an earlier benchmarks_main build in this session, and `build_preset.sh openblas-cpu --target test_main --test` failed exactly this way before the fix. Reconfigure in place for every non-fresh build now, with RUN_TESTS=ON for every target except benchmarks_main. benchmarks_main is deliberately excluded: RUN_TESTS adds --coverage instrumentation to the cytnx library, which benchmarks_main also links against, and that overhead would skew benchmark timings -- so RUN_TESTS is left untouched (neither forced on nor off) for that target, and a clean comparison needs a build dir that's never had RUN_TESTS turned on. Verified end to end: after the fix, `build_preset.sh openblas-cpu --target test_main --test` on the same previously-broken dir reconfigures RUN_TESTS=ON and passes all 1173 tests. Co-Authored-By: Claude <noreply@anthropic.com>
…visions Step 1's whole-tree `git checkout <rev>` touches .claude/ along with everything else. For the common base-vs-tip comparison, <rev> is any commit before this skill's own build_preset.sh existed in the tree (or lived at tools/build_preset.sh, a different path), so "$S" pointed at a path the checkout had just made disappear, and step 3 would fail before measuring anything. Copy the script to a stable temp path before the loop starts and use that copy throughout, relying on build_preset.sh now resolving its repo root via `git rev-parse --show-toplevel` rather than a path relative to its own location, so the copy still operates on the right repo. Also note the RUN_TESTS/coverage-instrumentation interaction from the build-test-workflow fix: prefer a build dir this skill hasn't shared with a test_main/pycytnx build, since --target benchmarks_main leaves an already-set RUN_TESTS=ON untouched. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcfe34b69c
ℹ️ 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".
gtest_discover_tests defaults to POST_BUILD discovery, which runs the freshly built test binary as part of `cmake --build` itself to enumerate its cases. For gpu_test_main (a CUDA-linked binary), this means the build step itself invokes the binary, which aborts on a machine with no GPU driver even though the binary compiled and linked correctly -- contradicting the documented workflow of compile-checking a CUDA preset without a GPU (CLAUDE.md, build-test-workflow skill). Switch both test_main and gpu_test_main to DISCOVERY_MODE PRE_TEST, which defers running the binary to enumerate tests until ctest is actually invoked, matching the same fix already made for this in the GPU CI workflow branch. Verified against the CPU suite: debug-openblas-cpu still discovers and runs individual tests correctly through ctest -R after this change. Co-Authored-By: Claude <noreply@anthropic.com>
…n, strip coverage via a launcher Replace the per-target conditional configure logic (RUN_TESTS forced on except for benchmarks_main, RUN_BENCHMARKS only for benchmarks_main, reconfigure-in-place when an existing dir was missing a flag the current target needed) with a single rule: a fresh build dir's first configure turns both RUN_TESTS and RUN_BENCHMARKS on unconditionally, regardless of --target. Every dir this script creates can build test_main/gpu_test_main/benchmarks_main from then on with no later reconfigure, at the cost of requiring Google Benchmark to be installed for any build through this script, not only a --target benchmarks_main one -- expected on an agent's own dev machine, unlike a minimal CI runner this script isn't targeting. RUN_TESTS=ON normally adds --coverage instrumentation to the cytnx library, which would skew benchmarks_main's timings too since it links the same library. Add strip-coverage-launcher.sh, wired in via CMAKE_CXX_COMPILER_LAUNCHER/CMAKE_CXX_LINKER_LAUNCHER at that same first configure, which strips the literal --coverage token before it reaches the real compiler/linker -- verified empirically that -fno-profile-arcs/-fno-test-coverage do NOT cancel --coverage (a .gcno file is still produced regardless of flag order), so token removal is what actually works. Verified end to end: reconfiguring debug-openblas-cpu with the launcher and rebuilding produced zero fresh .gcno files across 169 recompiled translation units, and the resulting test_main still built, linked (no gcov runtime), and passed its tests. Also: - Use a completion marker, not bin/activate's mere existence, to decide whether a Python target's first-build pip install still needs to run: `python3 -m venv` creates bin/activate before `pip install --editable` ever executes, so a venv left behind by a failed install (missing native deps, a compile error) would otherwise look "done" and silently skip the pip path on every retry. - Revert the --doctest-modules default and drop "matches CI" framing from the ctest/pytest rationale throughout: this script targets an agent's own iteration loop, not CI parity. Co-Authored-By: Claude <noreply@anthropic.com>
…table script copy Three fixes to the same recipe: - Revert the stable-copy-before-checkout step from the previous commit: step 1's whole-tree checkout only makes "$S" disappear for a revision that predates build_preset.sh's own existence in the tree, which is a one-time transitional concern for this branch's own history, not a standing property of comparisons entirely within this repo going forward. Note it as an edge case (copy the script out first) instead of the default recipe. - Scope the Case A overlay to the one benchmark file under comparison: overlaying <tip>'s whole benchmarks/CMakeLists.txt could drag in other benchmark sources added or changed after <rev> that don't exist or don't compile against <rev>'s API, breaking the build over a benchmark that isn't even the one being measured. Only copy the .cpp and add its one line to <rev>'s own CMakeLists.txt when <rev> doesn't already list it; skip the overlay/restore steps entirely when it already does (the common case for an existing benchmark). - Drop the "prefer a build dir this skill hasn't shared with a test build" caveat: build-test-workflow's script now strips --coverage from every build it produces (not only benchmarks_main ones), so a dir it already built for test_main/pycytnx is safe to reuse here. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0089a636f0
ℹ️ 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".
…t, not the script's location
coverage_launcher was derived from the running script's own path
(dirname "${BASH_SOURCE[0]}"), which breaks the moment the script is
copied elsewhere -- e.g. cross-revision-benchmark's documented edge
case of copying build_preset.sh to /tmp before comparing a revision
that predates it. A copy at /tmp/build_preset.sh would point CMake at
/tmp/strip-coverage-launcher.sh, which was never copied alongside it,
so the first compile/link through the launcher fails.
Resolve it from repo_root instead, the same way repo_root itself is
now resolved via git rather than the script's location: the real repo
checkout always has strip-coverage-launcher.sh at this fixed path,
regardless of where the running copy of build_preset.sh lives.
Co-Authored-By: Claude <noreply@anthropic.com>
…n CUDA presets A CUDA preset's shared build dir registers both test_main's and gpu_test_main's tests once RUN_TESTS=ON (both are added under tests/CMakeLists.txt's if(RUN_TESTS) block once USE_CUDA=ON), regardless of which one a given --target actually built. --test-dir alone doesn't scope a ctest run to just the built binary, so e.g. --target test_main --test on a CUDA preset would report gpu_test_main's tests as NOT_BUILT (or, if gpu_test_main happened to be built already, run its tests too) even though only test_main was requested. Add a `gpu` ctest label to gpu_test_main's gtest_discover_tests (tests/gpu/CMakeLists.txt), mirroring the GPU CI workflow's use of the same label, and have build_preset.sh pick -LE gpu / -L gpu based on which of test_main/gpu_test_main is actually in the requested --target, reusing the already-computed target_words array. Verified the -LE gpu exclusion is a no-op on a CPU preset (no gpu-labeled tests exist there): build_preset.sh debug-openblas-cpu --target test_main --test 'TypeTest\..*' still filters and passes correctly. Co-Authored-By: Claude <noreply@anthropic.com>
…rce in Case A Step 2 only copied <tip>'s benchmark file when <rev>'s own CMakeLists.txt didn't already list it -- but Case A's entire premise is one *fixed* benchmark source across every column, so a revision that already has its own (possibly different) version of the file must never run that version. Skipping the overlay whenever <rev> already lists the file meant that column silently ran its historical benchmark body instead of <tip>'s, mixing a benchmark-code difference into what should be a pure library comparison. Copy the .cpp from <tip> unconditionally now; only the CMakeLists.txt registration stays conditional (skipped when <rev> already lists the file). Step 4's restore follows the same split: the .cpp is always overlaid so it's always restored, while CMakeLists.txt is only touched -- and only needs restoring -- when step 2 also edited it. Also removed a duplicated paragraph (the git restore vs git checkout HEAD rationale was stated twice, left over from an earlier edit). Co-Authored-By: Claude <noreply@anthropic.com>
…t invocation The default (no-args) pytest path now runs `pytest pytests/ --doctest-modules`, catching doctest regressions (stale/wrong `>>>` examples in docstrings) through the same --target pycytnx --test entry point used for the rest of the Python test suite. An explicit filter/path still replaces the default collection verbatim and does not get --doctest-modules added automatically. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa698f0406
ℹ️ 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".
IvanaGyro
commented
Jul 11, 2026
IvanaGyro
commented
Jul 11, 2026
IvanaGyro
commented
Jul 11, 2026
IvanaGyro
marked this pull request as draft
July 11, 2026 19:37
gtest_discover_tests's PROPERTIES LABELS only attaches to dynamically discovered real tests, not to the <target>_NOT_BUILT sentinel test CMake creates under DISCOVERY_MODE PRE_TEST when a binary is absent at ctest time. That leaves -LE gpu/-L gpu unable to exclude an unbuilt sibling binary's sentinel, so the scoping added in b5bb33f does not actually solve the case it was meant to solve. build_preset.sh now runs ctest --test-dir unfiltered again and its --test doc recommends building --target "test_main gpu_test_main" together on a CUDA preset for a clean run instead. Co-Authored-By: Claude <noreply@anthropic.com>
…story DISCOVERY_MODE PRE_TEST is a permanent property of these CMakeLists.txt files now, not a workaround layered on top of a default; state it as the current behavior instead of contrasting it with POST_BUILD. Same for the ASan export: say when it happens, not why an alternative timing would have been wrong. Co-Authored-By: Claude <noreply@anthropic.com>
…rk-first ordering CLAUDE.local.md's rule that a benchmark lands before the change it measures guarantees the benchmark file and its CMakeLists.txt registration already exist at every revision worth comparing, as long as the earliest comparison point is chosen correctly: the commit where the benchmark landed, or the base branch tip if the benchmark predates the branch. That removes the need to overlay a fixed benchmark source onto a revision that lacks it and restore the tree afterward, so the Case A/B split and the associated git checkout/restore procedure are gone. The procedure is now a plain sequential checkout-build-run loop in the main working tree. Also drops the "comparing against a revision predating this script" note: the script did not exist before this PR, so there is no earlier revision to compare against that would hit that case. Co-Authored-By: Claude <noreply@anthropic.com>
…t -LE
An unbuilt sibling binary's <target>_NOT_BUILT sentinel (registered by
gtest_discover_tests's PRE_TEST discovery mode when the executable is
absent) never gets a PROPERTIES LABELS value, since labels only attach
to dynamically discovered real tests. -LE gpu ("run anything not
labeled gpu") let that unlabeled sentinel through; -L '^cpu$' ("run
only things labeled cpu") excludes it by construction instead, since
it has no label to match.
Re-adds PROPERTIES LABELS cpu/gpu to test_main/gpu_test_main's
gtest_discover_tests calls, and has build_preset.sh pick -L '^cpu$'/
-L '^gpu$' from the requested --target. Also adds --no-tests=error
(supported since CMake 3.25, the repo's floor) so a run that selects
zero tests fails loudly instead of reporting a silent pass, guarding
against the label ever going missing.
Verified against debug-openblas-cpu: -L '^cpu$' selects test_main's
tests correctly (Label Time Summary confirms 5/5 filtered tests ran
under the cpu label), and ctest -L '^gpu$' --no-tests=error against
that same non-CUDA build dir (no gpu-labeled tests exist) exits 8
with "No tests were found!!!" instead of silently reporting success.
Co-Authored-By: Claude <noreply@anthropic.com>
Trim both SKILL.md files down to what an agent needs at decision time: build-test-workflow keeps the entry-point usage (presets, targets, the target-dependent meaning of --test args), the two-phase build logic (editable pip install first for pybind11/dev deps, plain incremental cmake --build after), the GPU rules (compile check only without a GPU, run the suite with one, ASAN_OPTIONS handled by the script), the regression-test-must-fail-on-pre-fix-code rule, and the standalone harness recipe. Script-internal mechanics (completion marker, generator consistency, preset pinning, max parallelism) stay documented in the script itself. The --coverage-stripping rationale moves out of the testing skill: the benchmark skill is where uninstrumented builds matter (timing fairness), and strip-coverage-launcher.sh itself carries the full why. The entry script's header now just points there. cross-revision-benchmark is restructured around tip-vs-merge-base as the default comparison, benchmark-as-first-commit, release presets with in-place revision switching for incremental rebuilds, strictly sequential execution, and reporting (label + SHA, aggregation, numbers into the PR description). Co-Authored-By: Claude <noreply@anthropic.com>
The Test section duplicated the skills' essential facts for agents without .claude/skills/ auto-discovery. Supporting such agents is not a goal, and the duplicated commands drift from the skills they summarize (the pip/venv recipe already disagreed with what build_preset.sh actually does). CLAUDE.md now just names the two skills. Co-Authored-By: Claude <noreply@anthropic.com>
Back into build-test-workflow: never delete a build dir over a generator/cache mismatch (reconfigure without -G instead), the "how much to run, when" staging guidance (affected suite while iterating, full CLAUDE.md gates before push), the literal ASAN_OPTIONS value the script exports for debug-*-cuda presets, and the macOS variant of the standalone-harness link line. Back into cross-revision-benchmark: the correct-baseline caveat -- an old revision that is buggy in a way that changes how much work it does is not a fair baseline, and the report must say so. Co-Authored-By: Claude <noreply@anthropic.com>
A skill's frontmatter description is what an agent reads when deciding
whether to load the skill at all -- the body is only seen afterward. A
topic summary loses that dispatch when the task's phrasing doesn't
overlap it; explicit trigger conditions ("use whenever ... must be
built or tested", "use whenever a performance claim must be made or
checked") match the decision the agent is actually making. CLAUDE.md's
two skill bullets get the same treatment: name the moment to invoke,
not the table of contents.
Co-Authored-By: Claude <noreply@anthropic.com>
IvanaGyro
marked this pull request as ready for review
July 12, 2026 19:54
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 522cc6b561
ℹ️ 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".
Comment on lines
+190
to
+191
| --config-settings=cmake.args="-DCMAKE_CXX_COMPILER_LAUNCHER=${coverage_launcher}" | ||
| --config-settings=cmake.args="-DCMAKE_CXX_LINKER_LAUNCHER=${coverage_launcher}" |
There was a problem hiding this comment.
For fresh CUDA presets this only installs the coverage-stripping launcher for CXX compilations. Fresh evidence since the coverage workaround: cmake --help-command target_compile_options says these options apply to all languages unless guarded by COMPILE_LANGUAGE, and CMAKE_<LANG>_COMPILER_LAUNCHER is per-language and separately supports CUDA, so RUN_TESTS=ON still leaves the top-level --coverage option on cytnx's .cu/CUDA-language sources. That makes CUDA test/benchmark builds either fail under nvcc or remain coverage-instrumented despite the script promising uninstrumented builds; add CMAKE_CUDA_COMPILER_LAUNCHER here and in the raw configure_args path, or make the coverage option CXX-only.
Useful? React with 👍 / 👎.
yingjerkao
added a commit
that referenced
this pull request
Jul 13, 2026
Brings in #1039, #1047 (39 commits). Clean textual merge, but #1026's new test `UniTensor_baseTest.rank_detects_label_bond_mismatch` desyncs bonds/labels via `bonds().pop_back()` -- which no longer compiles now that this PR makes the mutable `bonds()` accessor const (immutable Bond, #846). That desync is unreachable through the public API by design, so the test now pokes the internal `_bonds` vector (const_cast) to still exercise rank()'s defensive size-mismatch guard. Verified: openblas-cpu + RUN_TESTS build clean (CI's editable-install compile gate reproduced locally); UniTensor_base/BlockFermionic/Bond gtests 38 passed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Two recurring gaps surfaced while working #853/#506 (
OptimalTreeSolver::solve):libcytnx.afor a differential/fuzz check. Each of these was re-derived from scratch mid-session.Separately, comparing a heuristic planner's contraction order across revisions surfaced that a tie-break change (which of several equal-cost candidates gets picked) is itself an observable behavior change, not a no-op —
CLAUDE.md's existing physics/numerical-correctness guardrail didn't cover it.Fix
build-test-workflow(.claude/skills/build-test-workflow/SKILL.md), alongside the existinggoogle-cpp-style/google-python-styleskills: accumulated/incremental builds, runningctest/pytestwithout reconfiguring, per-suite--gtest_filteriteration with theASAN_OPTIONSworkaround, the requirement that a regression test be shown to fail on the pre-fix revision, and the exact link line for a standalone verification harness againstlibcytnx.a.cross-revision-benchmark(.claude/skills/cross-revision-benchmark/SKILL.md): the checkout-files → rebuild-library-only → run → restore-tree procedure, default base-vs-tip scope, and reporting rules (named commits, same session/machine, stated aggregation, flagging baselines whose bugs change how much work they do).CLAUDE.md's Test section at both skills.CLAUDE.mdguardrail: tie-break/selection-order changes in a heuristic planner are observable behavior and need the same flag-and-sign-off treatment as physics/numerical changes. Mirror it inGEMINI.md's review priorities to keep the two files in sync, perCLAUDE.md's own instruction.Testing
Documentation-only change (
CLAUDE.md,GEMINI.md, two newSKILL.mdfiles) — no buildable code touched, so the build-and-test cycle does not apply.pre-commit runover the changed files passes.Draft — opening for early feedback on skill scope/content before requesting review.
Generated by Claude Code