diff --git a/.agents/roadmap_v1.md b/.agents/roadmap_v1.md index c7d09a647..a5d56cc6b 100644 --- a/.agents/roadmap_v1.md +++ b/.agents/roadmap_v1.md @@ -87,6 +87,7 @@ issue is not yet placed. Keyed record: update in place, never append. | [#475](https://github.com/mudler/vllm.cpp/issues/475) | `ENG-RELEASE-WINDOWS` | PR #446 release benchmark projection exceeds its page budget after merging current main | bug | | [#499](https://github.com/mudler/vllm.cpp/issues/499) | `ENG-RELEASE-WINDOWS` | Release binaries report project version instead of prerelease identity | bug | | [#500](https://github.com/mudler/vllm.cpp/issues/500) | `ENG-RELEASE-WINDOWS` | MSVC release build lacks ostream definition for string_view doctest diagnostics | bug | +| [#720](https://github.com/mudler/vllm.cpp/issues/720) | `ENG-RELEASE-WINDOWS` | `M_PI` is a POSIX extension MSVC's `` does not define, so `windows-msvc-*` hard-error on every PR; spec [`windows-msvc-m-pi.md`](specs/windows-msvc-m-pi.md). MEASURED, and the intake's "three TUs" is wrong both ways: the two LTX-2.5 VAEs carry their own `#define` and compile (one of them with ZERO uses), while `tests/vllm/models/test_vocoder1d.cpp` — which `git grep -- src include` never looked at and the CI log never reached — is the second real break | bug | | [#170](https://github.com/mudler/vllm.cpp/issues/170) | `ENG-RELEASE-BINARIES` | Publish container images to GHCR (cuda, vulkan, cpu) | feature | | [#322](https://github.com/mudler/vllm.cpp/issues/322) | `ENG-RELEASE-BINARIES` | Release handoff collides with tracked checkout `assets` directory | bug | | [#406](https://github.com/mudler/vllm.cpp/issues/406) | `ENG-TRAILER-MERGE-ARTIFACTS` | The trailer gate fails on how commits LAND: GitHub's Co-authored-by displaces the trailer block | bug | diff --git a/.agents/specs/windows-msvc-m-pi.md b/.agents/specs/windows-msvc-m-pi.md new file mode 100644 index 000000000..8a88d3b41 --- /dev/null +++ b/.agents/specs/windows-msvc-m-pi.md @@ -0,0 +1,149 @@ +# Native MSVC `M_PI` repair — the LTX-2.5 pi spellings + +Identity: `ENG-RELEASE-WINDOWS` + +Issue: [#720](https://github.com/mudler/vllm.cpp/issues/720) + +Parent specification: [windows-binary-release.md](windows-binary-release.md) + +Predecessor repair: +[windows-msvc-strict-build.md](windows-msvc-strict-build.md), whose Design +already ruled: "Use standard C++20 math constants rather than enabling +non-standard `M_PI`." This repair applies that ruling to the LTX-2.5 sources +that landed after it. + +Status: `ACTIVE`. Base `a4313641413394e48e6e7a2f05d8e13d541f8be0`. + +## Scope + +Remove every `M_PI` from `src/` and `tests/` and replace it with +`std::numbers::pi_v`, the spelling this repository already uses at four +existing sites. Delete the two hand-rolled `#ifndef M_PI / #define M_PI` blocks +in the LTX-2.5 VAEs. Change no value, no expression structure, no tolerance, no +golden, and no runtime behavior. + +Explicitly excluded: `_USE_MATH_DEFINES` in any form; any new named pi constant +of our own; `src/vllm/multimodal/video_engine.cpp` (issue #664, PR #677); the +second, unrelated failure in the same portability suite (issue #680). + +## Observed baseline and root cause + +`M_PI` is a POSIX extension, not standard C++. MSVC's `` defines it only +when `_USE_MATH_DEFINES` precedes the include, so every unguarded use is a hard +C2065. `windows-msvc-cpu` on PR #677 reported: + +``` +src\vllm\model_executor\models\ltx2.cpp(72,58): error C2065: 'M_PI': undeclared identifier +src\vllm\model_executor\models\ltx2.cpp(72,15): error C2737: 'kBeta': const object must be initialized +src\vllm\model_executor\models\ltx2.cpp(557,69): error C2065: 'M_PI': undeclared identifier +src\vllm\model_executor\models\ltx2.cpp(572,76): error C2065: 'M_PI': undeclared identifier +``` + +The intake counted "three LTX-2.5 TUs", from `git grep -l M_PI -- src include`. +**That count is wrong in both directions**, and measurement rather than grep is +what separates them: + +- `ltx2_audio_vae.cpp` and `ltx2_video_vae.cpp` carry their own + `#ifndef M_PI / #define M_PI 3.14159265358979323846` guard, so both compile + under MSVC today. They are not build breaks. They are two extra spellings of + pi — the defect [#687](https://github.com/mudler/vllm.cpp/issues/687) names — + and `ltx2_video_vae.cpp`'s guard has **zero uses**: it is dead. +- `tests/vllm/models/test_vocoder1d.cpp` has two unguarded uses and **is** a + hard MSVC break. `git grep -- src include` never looked at `tests/`, and the + CI log never named it because the library build stops at `ltx2.cpp` first. + +So the real MSVC break is two TUs, not three, and one of the two was invisible +to both the CI message and the intake grep. + +## Design + +`std::numbers::pi_v` from ``, available at this project's +`CMAKE_CXX_STANDARD 20` (`CMakeLists.txt:35`), and already the spelling at +`mla_attention.cpp:82`, `deepseek_v4.cpp:602`, `minimax_h3_video_vae.cpp:258`, +and `vocoder1d.cpp:35`. One definition — the standard's — reached by an include +rather than by a macro whose correctness depends on preceding every transitive +``. No fifth spelling is introduced. + +`_USE_MATH_DEFINES` is rejected: it is order-dependent across transitive +includes, and it re-declares the POSIX extension instead of removing the +dependency on it. + +## Tests and RED evidence + +No checker changes. `tests/scripts/test_check_windows_portability.py`'s +`test_real_tree_uses_portable_windows_allocation_and_math` **already** asserts +that no file under `src/` or `tests/` contains `\bM_PI\b`, and it is already RED +on `main` for exactly these four files. That committed assertion is the RED, and +it is also why the `tests/` TU is in scope: the existing contract always covered +it. Because that suite runs in no workflow (#646, #680), the assertion had gone +unheld while the LTX-2.5 lane landed. + +The MSVC diagnostic itself is additionally reproduced on Linux by compiling the +real TUs with `-U_GNU_SOURCE -D_ISOC99_SOURCE`, which removes glibc's `M_PI` +from `` and so recreates MSVC's header condition on g++. + +Value identity is asserted, not assumed, three ways: the bit pattern of both +spellings, a token-sequence equivalence proof that the diff restructures no +expression, and a before/after run of the seven LTX-2.5 and vocoder golden +suites. + +## Gates + +1. RED: `test_real_tree_uses_portable_windows_allocation_and_math` fails at base + naming all four files; the Linux MSVC-condition probe reproduces the C2065 + sites at matching line and column. +2. GREEN: the same test passes; the suite's case count is unchanged. +3. Before/after golden equality across `test_vocoder1d`, `test_ltx2`, + `test_ltx2_vae`, `test_ltx2_device`, `test_ltx2_loader`, + `test_ltx2_text_encoder`, `test_ltx2_pipeline`. +4. Mutation: reverting the constant re-reds the gate; perturbing pi's value reds + the goldens, proving arm 3 is not a vacuous green. +5. Full `scripts/agent-preflight.sh --staged` and full `ctest`. +6. Native `windows-msvc-cpu` and `windows-msvc-vulkan` on the PR head. Linux + cannot substitute for the MSVC compiler gate. + +## Risks and stop conditions + +The risk is a silent value or associativity change hiding behind a mechanical +substitution — a token gate cannot see a constant that is merely slightly wrong. +The token-equivalence proof and the value-perturbation mutation exist for that. +Stop with `NEEDS_DECISION` if any golden moves; a moved value would mean the +constant is not what it is claimed to be. + +## Outcome + +The base tree's own committed portability test was already RED, naming +`ltx2.cpp`, `ltx2_video_vae.cpp`, `ltx2_audio_vae.cpp` and +`tests/vllm/models/test_vocoder1d.cpp` — four files, not the three the issue +title counts, and 71 cases ran in both arms with failures going 2 to 1. The +surviving failure, `test_real_unsupported_tier_helper_is_structurally_scoped`, +is pre-existing and owned by #680; it was captured failing on the pristine base +before any edit. + +The Linux `-U_GNU_SOURCE -D_ISOC99_SOURCE` probe reproduced the hosted +diagnostic at `ltx2.cpp` 72:58, 557:69 and 572:76 — the same line **and column** +as the MSVC log — and additionally at `test_vocoder1d.cpp` 106:21 and 121:21, +which no MSVC log had yet reached. It emitted nothing for the two VAEs, +confirming by measurement that their guards make them compile. + +Value identity holds three ways. `M_PI`, `std::numbers::pi_v` and the +hand-rolled `3.14159265358979323846` are the same double, `400921fb54442d18`, +and the three derived quantities (`sqrt(2/pi)` as float, `pi/2` as float and as +double) are bit-equal. A token-sequence comparison over all four files, 41,191 +tokens with the two pi spellings unified, reports pure token substitution: the +only differences are the added `` includes and the two removed +hand-rolled defines, so no operator order, cast or operand moved. The seven +golden suites report identical counts before and after — 185 cases and 17,378 +assertions, all passing in both arms. + +That third arm is not vacuous. Perturbing pi to `3.14159` at the three +`ltx2.cpp` sites builds clean and reds 60 assertions in `test_ltx2` and 5 in +`test_ltx2_pipeline`, so those goldens do see this constant. `test_ltx2_vae` +stays green under that mutation — it does not reach these sites, and its +unchanged result is therefore evidence of nothing in particular. + +Reverting the constant to `M_PI` in `ltx2.cpp` still **builds clean on Linux** +(exit 0) while re-reding the portability gate and the MSVC-condition probe. That +is the whole shape of this bug: the Linux build is structurally blind to it, and +only a source-pattern gate or the MSVC compiler can see it. Every mutation was +restored from the index and verified by `sha256sum -c`, never by `git status`. diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 39d635481..63a59c153 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -27,7 +27,7 @@ are our reading of their documented behavior, not measurements. | Correctness gate | token-exact vs vLLM | reference | own | own | | Architectures | 38 registered, 27 gated | 130+ | 100+ | 100+ | | Downloadable server binaries | ✅ v0.0.2: eight indexed archives with checksums, provenance, manifests, and SBOMs. Windows ZIP downloads do not exist; native CPU/Vulkan lanes await hosted runtime, dry-run, prerelease, and authenticated audit gates | ✅ wheels/containers | ✅ wheels/containers | ✅ host-specific binaries | -| Native Windows builds | ◐ CPU/Vulkan: `/MT /W4 /WX`, central `NOMINMAX`, UTF-8, aligned allocation, runtime ISA dispatch. Local closure includes the float-domain DeepSeek probe; hosted compile/runtime/release pending | ✅ | ✅ | ✅ | +| Native Windows builds | ◐ CPU/Vulkan: `/MT /W4 /WX`, central `NOMINMAX`, UTF-8, aligned allocation, C++20 `std::numbers` pi, runtime ISA dispatch. Local closure includes the float-domain DeepSeek probe; hosted compile/runtime/release pending | ✅ | ✅ | ✅ | ## Serving and scheduling diff --git a/src/vllm/model_executor/models/ltx2.cpp b/src/vllm/model_executor/models/ltx2.cpp index 3d9af69ce..f4ec1202f 100644 --- a/src/vllm/model_executor/models/ltx2.cpp +++ b/src/vllm/model_executor/models/ltx2.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -69,7 +70,7 @@ float Silu(float x) { return x / (1.0f + std::exp(-x)); } // torch.nn.functional.gelu(..., approximate="tanh"), the activation // `activation_fn="gelu-approximate"` selects (gelu_approx.py:10). float GeluTanh(float x) { - const float kBeta = static_cast(std::sqrt(2.0 / M_PI)); + const float kBeta = static_cast(std::sqrt(2.0 / std::numbers::pi_v)); const float kKappa = 0.044715f; const float inner = kBeta * (x + kKappa * x * x * x); return 0.5f * x * (1.0f + std::tanh(inner)); @@ -554,7 +555,8 @@ std::vector FreqGridPytorch(double theta, int64_t n_pos_dims, int64_t dim const float t = i < halfway ? step * static_cast(i) : 1.0f - step * static_cast(n - 1 - i); out[static_cast(i)] = - std::pow(static_cast(theta), t) * static_cast(M_PI / 2.0); + std::pow(static_cast(theta), t) * + static_cast(std::numbers::pi_v / 2.0); } return out; } @@ -569,7 +571,8 @@ std::vector FreqGridNumpy(double theta, int64_t n_pos_dims, int64_t dim) for (int64_t i = 0; i < n; ++i) { // numpy's linspace is arange(n) * step, with the final sample forced to `stop`. const double t = (i == n - 1) ? 1.0 : step * static_cast(i); - out[static_cast(i)] = static_cast(std::pow(theta, t) * (M_PI / 2.0)); + out[static_cast(i)] = + static_cast(std::pow(theta, t) * (std::numbers::pi_v / 2.0)); } return out; } diff --git a/src/vllm/model_executor/models/ltx2_audio_vae.cpp b/src/vllm/model_executor/models/ltx2_audio_vae.cpp index cff349f48..c84d591da 100644 --- a/src/vllm/model_executor/models/ltx2_audio_vae.cpp +++ b/src/vllm/model_executor/models/ltx2_audio_vae.cpp @@ -24,6 +24,7 @@ #include #include #include +#include #include #include @@ -31,10 +32,6 @@ #include "vllm/model_executor/models/vocoder1d.h" #include "vt/dtype.h" -#ifndef M_PI -#define M_PI 3.14159265358979323846 -#endif - namespace vllm { const std::vector& Ltx2VaeWeights::Get(const std::string& name) const { @@ -433,9 +430,14 @@ std::vector Ltx2HannSincResampleFilter1d(int64_t ratio, int64_t* kernel_s const double clamped = std::max(-static_cast(lowpass_filter_width), std::min(static_cast(lowpass_filter_width), t)); - const double window = - std::pow(std::cos(clamped * M_PI / static_cast(lowpass_filter_width) / 2.0), 2.0); - const double sinc = t == 0.0 ? 1.0 : std::sin(M_PI * t) / (M_PI * t); + const double window = std::pow( + std::cos(clamped * std::numbers::pi_v / + static_cast(lowpass_filter_width) / 2.0), + 2.0); + const double sinc = + t == 0.0 ? 1.0 + : std::sin(std::numbers::pi_v * t) / + (std::numbers::pi_v * t); filter[static_cast(i)] = static_cast(sinc * window * rolloff / static_cast(ratio)); } @@ -1043,7 +1045,8 @@ std::vector Ltx2WaveformToLogMel(const Ltx2AudioProcessorConfig& config, std::vector window(static_cast(n_fft)); for (int64_t i = 0; i < n_fft; ++i) { window[static_cast(i)] = - 0.5 - 0.5 * std::cos(2.0 * M_PI * static_cast(i) / static_cast(n_fft)); + 0.5 - 0.5 * std::cos(2.0 * std::numbers::pi_v * + static_cast(i) / static_cast(n_fft)); } // `center=True, pad_mode="reflect"`: pad n_fft/2 on BOTH sides, so frame 0 is @@ -1081,7 +1084,8 @@ std::vector Ltx2WaveformToLogMel(const Ltx2AudioProcessorConfig& config, for (int64_t f = 0; f < n_freqs; ++f) { double real = 0.0; double imag = 0.0; - const double omega = -2.0 * M_PI * static_cast(f) / static_cast(n_fft); + const double omega = -2.0 * std::numbers::pi_v * + static_cast(f) / static_cast(n_fft); for (int64_t i = 0; i < n_fft; ++i) { const double angle = omega * static_cast(i); real += frame[static_cast(i)] * std::cos(angle); diff --git a/src/vllm/model_executor/models/ltx2_video_vae.cpp b/src/vllm/model_executor/models/ltx2_video_vae.cpp index c7cb88825..592f5b8b4 100644 --- a/src/vllm/model_executor/models/ltx2_video_vae.cpp +++ b/src/vllm/model_executor/models/ltx2_video_vae.cpp @@ -55,10 +55,6 @@ #include "vllm/model_executor/models/minimax_h3.h" #include "vt/dtype.h" -#ifndef M_PI -#define M_PI 3.14159265358979323846 -#endif - namespace vllm { namespace { diff --git a/tests/vllm/models/test_vocoder1d.cpp b/tests/vllm/models/test_vocoder1d.cpp index 01075f92c..ee9e86d26 100644 --- a/tests/vllm/models/test_vocoder1d.cpp +++ b/tests/vllm/models/test_vocoder1d.cpp @@ -17,6 +17,7 @@ // ABSOLUTE floor under any epsilon, which would accept almost anything. #include #include +#include #include #include "doctest/doctest.h" @@ -103,7 +104,7 @@ TEST_CASE("vocoder1d Pad1d zero mode does not replicate") { TEST_CASE("vocoder1d SnakeActivation adds sin^2 scaled by 1/beta") { // x + (beta + eps)^-1 * sin^2(alpha * x), alpha = 1, beta = 1, x = pi/2. // sin(pi/2) = 1, so the result is pi/2 + 1/(1 + 1e-9). - const double x0 = M_PI / 2.0; + const double x0 = std::numbers::pi_v / 2.0; std::vector x{static_cast(x0)}; const std::vector alpha{1.0F}; const std::vector beta{1.0F}; @@ -118,7 +119,7 @@ TEST_CASE("vocoder1d SnakeActivation with a null beta reuses alpha as the scale" // separate beta. alpha = 2, x = pi/4 => sin(2 * pi/4) = sin(pi/2) = 1, so the // result is pi/4 + 1/(2 + eps). Passing beta here instead would give a // different number, which is what makes the null-beta arm distinguishable. - const double x0 = M_PI / 4.0; + const double x0 = std::numbers::pi_v / 4.0; std::vector x{static_cast(x0)}; const std::vector alpha{2.0F}; vllm::vocoder1d::SnakeActivation(x, 1, 1, alpha, /*beta=*/nullptr, /*logscale=*/false);