Conversation
Krzmbrzl
force-pushed
the
windows-ci
branch
6 times, most recently
from
September 3, 2026 13:27
69218b2 to
be4ab4c
Compare
Krzmbrzl
commented
Sep 3, 2026
Krzmbrzl
force-pushed
the
windows-ci
branch
17 times, most recently
from
September 4, 2026 14:57
4600919 to
b53f4de
Compare
evaleev
added a commit
that referenced
this pull request
Sep 11, 2026
The repository has had no agent-facing conventions file, so every automated contributor has had to rediscover these -- and in one case relearn the first one the hard way. - Never regenerate *.expected fixtures. The suite can only check that today's output matches bytes a human previously blessed; it cannot check that a regenerated equation still computes the right numbers. Regenerating does not fix the failing verify test, it deletes the signal that a human needs to re-verify. Written unconditionally, with the rationalizations that do not justify it spelled out, because PR #596 produced most of them. - Throw sequant::Exception rather than std exceptions. Already uniform (every throw in SeQuant/ and utilities/ uses it), so this records the convention rather than changing it. - A green CI build does not prove includes are right: most CI configurations set CMAKE_UNITY_BUILD=ON, which masks a missing #include. Includes a non-unity configure line for verifying standalone compilation. - Formatting is pinned to clang-format 17; other majors disagree with it. - A short map of the module targets and their dependency direction. CLAUDE.md imports AGENTS.md rather than symlinking to it, so the content lives in one file without relying on symlink support. A committed symlink checks out on Windows as a regular file containing the literal target path unless core.symlinks is enabled, failing silently rather than loudly -- which matters now that #605 puts Windows CI on the roadmap.
evaleev
added a commit
that referenced
this pull request
Sep 11, 2026
The repository has had no agent-facing conventions file, so every automated contributor has had to rediscover these -- and in one case relearn the first one the hard way. - Never regenerate *.expected fixtures. The suite can only check that today's output matches bytes a human previously blessed; it cannot check that a regenerated equation still computes the right numbers. Regenerating does not fix the failing verify test, it deletes the signal that a human needs to re-verify. Written unconditionally, with the rationalizations that do not justify it spelled out, because PR #596 produced most of them. - Do not attribute tools. No Co-Authored-By trailer, no "Generated with Claude Code" line, in commits or PR descriptions. Recorded explicitly because the harness defaults of several agents instruct the opposite, so without a written project rule every session relitigates it. - Throw sequant::Exception rather than std exceptions. Already uniform (every throw in SeQuant/ and utilities/ uses it), so this records the convention rather than changing it. - A green CI build does not prove includes are right: most CI configurations set CMAKE_UNITY_BUILD=ON, which masks a missing #include. Includes a non-unity configure line for verifying standalone compilation. - Formatting is pinned to clang-format 17; other majors disagree with it. - A short map of the module targets and their dependency direction. CLAUDE.md imports AGENTS.md rather than symlinking to it, so the content lives in one file without relying on symlink support. A committed symlink checks out on Windows as a regular file containing the literal target path unless core.symlinks is enabled, failing silently rather than loudly -- which matters now that #605 puts Windows CI on the roadmap.
Krzmbrzl
force-pushed
the
windows-ci
branch
from
September 12, 2026 08:30
4079699 to
b016fe2
Compare
evaleev
added a commit
that referenced
this pull request
Sep 13, 2026
Exceptions: state the rule as "Exception or a class derived from it, nothing else", name the existing subclasses that are caught by subtype, and drop the claim that no helper returns a std type (the remaining sites are converted in a separate PR). SEQUANT_ASSERT_BEHAVIOR: list all three values with their defaults and what CI uses; recommend THROW for local builds since ABORT (the Debug default) kills the test binary. Unity builds: say only that some CI cells use them and point at the workflow file, per review; replace the from-scratch configure recipe with flipping the option on an existing tree and rebuilding one TU. Tests: add check-sequant / ctest -R "^sequant"; unit_tests-sequant alone does not run the fixture comparisons the first section protects. Fixtures: cover dump_tree as well as verify, fix "export trees" to "evaluation-tree dumps", say where the generated files land for each tool so the diff can be inspected, replace the glob list with git ls-files, and drop the #596 narrative. Formatting: route through bin/admin/clang-format.sh instead of a hard-coded clang-format-17; note that pre-commit hooks need pre-commit install and what else they reject. Layout: replace the ambiguous dependency chain with a table taken from target_link_libraries, including bliss, the eval::* backend export names, and the core-directory vs core-target distinction. CLAUDE.md: reduce to the import plus one line on why it is not a symlink; drop the dangling #605 reference.
Krzmbrzl
pushed a commit
to Krzmbrzl/SeQuant
that referenced
this pull request
Sep 16, 2026
Constant did not override Expr::static_less_than, so Constant::operator< fell through to the base implementation, which compares hash values. Hash order is unrelated to numeric order and differs between platforms; the ExprMatcher test that expects Constant(1) < Constant(2) held on macOS and Linux by accident and broke under MSVC (ValeevGroup#605). Order Constants by their value instead: by real part, then by imaginary part. Both are exact rationals. Sum canonicalization sorts by hash and only consults operator< on exact hash ties, and Product folds constants into its scalar, so no canonical or generated output changes. Closes ValeevGroup#614
unit_tests-sequant-eval-dryrun-obj never received target_set_compiler_flags/ target_set_optimization_flags, unlike every sibling test object library, so it built without /bigobj on MSVC. Its two heaviest TUs (deep Catch2 + eval template stacks) exceeded the COFF section limit as a result (C1128: number of sections exceeded object file format limit). test_ordered_executor.cpp used POSIX-only setenv/unsetenv unconditionally, which MSVC doesn't provide. Factored a portable set_env/unset_env pair into the shared catch2_sequant.hpp test header and pointed test_optimize.cpp's existing (locally #ifdef'd) equivalent at it too, rather than growing a third copy of the same _WIN32 guard. Also fixed a real bug the MSVC build flagged as a C4244 warning: ordered_schedule.hpp's cycle-diagnostic message narrowed the offending loop identity's wstring label to a string via truncating iterator construction (std::string(w.begin(), w.end())), corrupting any non-ASCII label instead of transcoding it. Use the existing toUtf8() instead.
Every OBJECT library in tests/unit/CMakeLists.txt called target_set_compiler_flags on itself twice: once right after add_library and again after its target_compile_definitions block. The second call is a no-op repeat of the first, not a distinct step, so keep only the one next to add_library, matching where target_set_optimization_flags already sits.
julia_itensor.hpp, julia_tensor_operations.hpp and text_generator.hpp each
joined a tensor's represented indices with ranges::views::join(", "s) --
an owning, temporary std::string. Under MSVC's range-v3 build the separator
silently disappears (e.g. "g[i_1i_2a_1a_2]" instead of
"g[i_1, i_2, a_1, a_2]"), breaking every generator's output and failing the
KnownGenerators/export unit tests.
Confirmed empirically: a views::join separator that is a std::string prvalue
loses its content on this toolchain, while an lvalue std::string or any
string_view works. The other views::join call sites in this codebase
(index.hpp, tensor_network/v2.cpp, v3.cpp) already pass a string_view
literal rather than an owning one and are unaffected -- switch these three
to match that existing, safe idiom instead of the owning-string literal.
run_python_code() builds its command as two shell_escape()-quoted tokens
("<python.exe>" "<script.py>") and hands it to std::system(). On Windows
that runs through cmd.exe /c, which strips exactly the command line's first
and last character whenever it starts with a quote -- fine for a single
quoted token, but here it instead corrupts the first token's own quoting,
so cmd.exe tries to run the executable path up to its first space (observed
as "'C:/Program' is not recognized as an internal or external command",
since the discovered interpreter lives under Program Files) and every
PythonEinsumGenerator validation case failed as a result.
Wrap the whole command in one more, redundant pair of quotes on Windows so
cmd.exe's stripping removes those instead of corrupting the real ones.
SEQUANT_INTERNAL_SKIP_LONG_TESTS auto-enabled on CMAKE_BUILD_TYPE STREQUAL "Debug", but that variable only reflects the active configuration for a single-config generator (Ninja, Makefiles). For a multi-config generator (Visual Studio, Ninja Multi-Config) it is always empty at configure time -- the real configuration is chosen later, per build/test invocation, via --config, and one configure step services every configuration at once. The check silently evaluated to false there regardless of which configuration ends up being built, defeating the whole mechanism for exactly the Windows workflow (opening the folder directly in Visual Studio) most likely to hit it. Skip long tests by default under a multi-config generator too, since there's no single correct answer to "is this Debug?" to fall back on there.
These fixtures were deliberately excluded from the Debug-default skip (SEQUANT_INTERNAL_SKIP_LONG_TESTS) on the assumption that "a plain Debug build" is cheap enough to always run them, reserving the explicit SEQUANT_SKIP_LONG_TESTS option for genuinely slow configurations (ASan/UBSan/valgrind). Measured on Windows: a plain MSVC Debug build takes 160-280s EACH for these cases (2751s for the whole suite), while the identical Release build takes 4-27s (a 40x+x difference on some of them). MSVC's Debug STL (checked iterators, /RTC) is far more expensive for container-heavy code than GCC/ Clang's -O0, so the "cheap enough to always run" assumption this gate relied on does not hold for that toolchain. Fold it into SEQUANT_INTERNAL_SKIP_LONG_TESTS like every other object library in this file: CI's sanitizer/valgrind jobs and an explicit -DSEQUANT_SKIP_LONG_TESTS=OFF Debug build still exercise these paths.
mbpt_cc, mbpt and wick already fall under SEQUANT_INTERNAL_SKIP_LONG_TESTS's Debug default (unlike the water-20 fixtures in the preceding commit), but each still had a small number of ungated sections dominating its total time under a plain MSVC Debug build (mbpt_cc: 279s, mbpt: 46s, wick: 38s; profiled with Catch2's --durations yes). All of them are secondary cross-checks rather than the primary pinned-equation coverage: - mbpt_cc: the explicit hbar_comm_rank=3 UCC energy override (118s, the bulk of mbpt_cc's total) and the whole bernoulli_hbar_structure/ bernoulli_quccsd sections (48s/63s, already partially gated for their most expensive tail); the amplitude construction now guarded there is read only under the same guard further down, so the CC object itself is marked [[maybe_unused]] rather than duplicated. - mbpt: the H2**T2**T2 use_topology cross-check (7.6s) and the manuscript reference-value EOM-CC/perturbed-amplitude sections (13.3s/13.2s). - wick: the use_op_partitions=false half of wick(P2*H2*T2*T2) (12.6s of its 38s), which reduces/canonicalizes a 544-term sum vs. 7 for =true; the =true path keeps exercising the same partitioning and reduction pipeline. Release and an explicit -DSEQUANT_SKIP_LONG_TESTS=OFF Debug build still run all of this.
The Debug-default reduced variant set for the integration coupled-cluster generators is meant to keep each of srcc/eomcc/ucc/stcc down to one cheap smoke-test variant. ucc and stcc had grown to two and four variants respectively, and under a plain MSVC Debug build several of the dropped ones (60s, 58s, 40s, and ucc's "2 bernoulli 2" alone) dominated the integration suite's total time far more than the single variant each of their siblings already settles for. Reduce both to one variant, matching srcc/eomcc.
CMAKE_BUILD_TYPE only reflects the active configuration for a single-config generator (Ninja, Makefiles); for a multi-config generator (Visual Studio, Ninja Multi-Config) it is always empty at configure time, since one configure step services every configuration at once. Three more places compared it directly, on top of the test-skipping logic fixed earlier: - benchmarks/CMakeLists.txt gated the cheap-benchmark compile definitions on it; switched to a $<CONFIG:Debug> generator expression, which is evaluated per-config and so works correctly for both generator kinds. - target_set_optimization_flags() in cmake/compiler.cmake used it to skip LTO entirely for a Debug build; under multi-config this always evaluated false, so a Debug configuration silently got LTO (hand-set -flto flags, or CMake's own INTERPROCEDURAL_OPTIMIZATION, which applies to every configuration when set unsuffixed) right alongside Release, degrading debuggability. Fixed with the same per-config generator-expression approach for hand-set flags, and the INTERPROCEDURAL_OPTIMIZATION_<CONFIG> per-configuration property for the CMake-driven path. - CMakeLists.txt's SEQUANT_ASSERT_BEHAVIOR default (Release/MinSizeRel vs. everything else) has no equivalent fix available: it is a single cache string forwarded verbatim into TA_ASSERT_POLICY/BTAS_ASSERT_POLICY, which are themselves baked into TiledArray's/BTAS's own non-per-config compile definitions -- a real fix needs upstream changes in those projects too. Documented the gap and why the existing fallback (ABORT, i.e. assertions stay enabled) is the safe direction to be wrong in under multi-config.
python/CMakeLists.txt hardcoded ':' when constructing PYTHONPATH for the sequant/unit/python/basic/run ctest entry. That's the right separator on POSIX but Windows needs ';', which is why the test failed there with "ModuleNotFoundError: No module named '_sequant'" even though the module built successfully. A literal ';' can't be substituted directly because CTest's ENVIRONMENT property is itself a ';'-delimited list, so an unescaped ';' would split into a bogus second entry instead of staying part of PYTHONPATH's value; use the $<SEMICOLON> generator expression on Windows to survive that split. AI-assisted
Note: the parameter was not removed entirely as the comment above explains that these are supposed to be some sort of stub for concrete implementations so presumably the function signature was chosen as-is for some reason. Hence, we leave the unused parameter in the signature and only fix the symptom of the warning itself.
Krzmbrzl
marked this pull request as ready for review
September 21, 2026 09:29
Container correctly used remove_cvref_t, but Value used remove_cvref (the trait class itself, not its ::type), so the requires-expression's parameter was a reference to an unrelated trait struct rather than the decayed Value type. This currently stays latent because emplace/ emplace_back on the containers in use are unconstrained forwarding templates, so the requires-expression only checks that overload resolution succeeds (which it trivially does for any argument type) without instantiating the body -- but the concept no longer verifies what its name promises and is a landmine for a future container with a constrained emplace.
a71880f ("Set compiler flags on more targets") added a call right after target_link_libraries while the pre-existing call (later renamed by 7e59b08) stayed further down, so every integration test target had it applied twice. 03c54eb cleaned up the identical pattern in tests/unit/CMakeLists.txt but missed this file.
…ionally Both "batched DP peak matches oracle with two modes and accumulation" and "reconstruct_batched_modes_emits_external_per_node" are tagged [.] and already self-document known, pre-existing regressions in the batched DP cost model, deferred to a future focused pass. [.] only excludes a test from a bare/no-argument run; an explicit tag filter that also matches (e.g. "[optimize]") still runs -- and fails -- them, as seen while verifying an unrelated change. Add an explicit SKIP() so they never execute regardless of how they're selected, while staying compiled so they don't bit-rot.
Krzmbrzl
force-pushed
the
windows-ci
branch
from
September 21, 2026 10:33
0ff33af to
1198e06
Compare
Krzmbrzl
commented
Sep 21, 2026
Comment on lines
+12
to
+21
| # CMAKE_BUILD_TYPE only reflects the active configuration for a single-config | ||
| # generator (Ninja, Makefiles); for a multi-config generator (Visual Studio, | ||
| # Ninja Multi-Config) it is always empty at configure time -- the actual | ||
| # configuration is chosen later, per build/test invocation, via --config, and | ||
| # one configure step services every configuration at once. There is | ||
| # therefore no single correct answer to "is this a Debug build?" here in that | ||
| # case; default to skipping the long-running tests then too, rather than | ||
| # silently keeping the always-run behavior that only makes sense for a | ||
| # genuine single-config Release build. | ||
| if (SEQUANT_SKIP_LONG_TESTS OR CMAKE_BUILD_TYPE STREQUAL "Debug" OR CMAKE_CONFIGURATION_TYPES) |
Collaborator
Author
There was a problem hiding this comment.
We probably want the skipping (which is a macro definition) to use generator expressions. Disabling these tests also in Release mode is definitely not desirable
This branch has not been deployed
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
1 participant
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.
No description provided.