Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .agents/roadmap_v1.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<cmath>` 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 |
Expand Down
149 changes: 149 additions & 0 deletions .agents/specs/windows-msvc-m-pi.md
Original file line number Diff line number Diff line change
@@ -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<double>`, 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 `<cmath>` 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<double>` from `<numbers>`, 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
`<cmath>`. 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 `<cmath>` 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<double>` 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 `<numbers>` 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`.
2 changes: 1 addition & 1 deletion docs/FEATURES.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
9 changes: 6 additions & 3 deletions src/vllm/model_executor/models/ltx2.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <cmath>
#include <cstring>
#include <limits>
#include <numbers>
#include <string>
#include <vector>

Expand Down Expand Up @@ -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<float>(std::sqrt(2.0 / M_PI));
const float kBeta = static_cast<float>(std::sqrt(2.0 / std::numbers::pi_v<double>));
const float kKappa = 0.044715f;
const float inner = kBeta * (x + kKappa * x * x * x);
return 0.5f * x * (1.0f + std::tanh(inner));
Expand Down Expand Up @@ -554,7 +555,8 @@ std::vector<float> FreqGridPytorch(double theta, int64_t n_pos_dims, int64_t dim
const float t = i < halfway ? step * static_cast<float>(i)
: 1.0f - step * static_cast<float>(n - 1 - i);
out[static_cast<size_t>(i)] =
std::pow(static_cast<float>(theta), t) * static_cast<float>(M_PI / 2.0);
std::pow(static_cast<float>(theta), t) *
static_cast<float>(std::numbers::pi_v<double> / 2.0);
}
return out;
}
Expand All @@ -569,7 +571,8 @@ std::vector<float> 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<double>(i);
out[static_cast<size_t>(i)] = static_cast<float>(std::pow(theta, t) * (M_PI / 2.0));
out[static_cast<size_t>(i)] =
static_cast<float>(std::pow(theta, t) * (std::numbers::pi_v<double> / 2.0));
}
return out;
}
Expand Down
22 changes: 13 additions & 9 deletions src/vllm/model_executor/models/ltx2_audio_vae.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,14 @@
#include <algorithm>
#include <cmath>
#include <cstddef>
#include <numbers>
#include <string>
#include <vector>

#include "vllm/model_executor/models/minimax_h3.h"
#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<float>& Ltx2VaeWeights::Get(const std::string& name) const {
Expand Down Expand Up @@ -433,9 +430,14 @@ std::vector<float> Ltx2HannSincResampleFilter1d(int64_t ratio, int64_t* kernel_s
const double clamped =
std::max(-static_cast<double>(lowpass_filter_width),
std::min(static_cast<double>(lowpass_filter_width), t));
const double window =
std::pow(std::cos(clamped * M_PI / static_cast<double>(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<double> /
static_cast<double>(lowpass_filter_width) / 2.0),
2.0);
const double sinc =
t == 0.0 ? 1.0
: std::sin(std::numbers::pi_v<double> * t) /
(std::numbers::pi_v<double> * t);
filter[static_cast<size_t>(i)] =
static_cast<float>(sinc * window * rolloff / static_cast<double>(ratio));
}
Expand Down Expand Up @@ -1043,7 +1045,8 @@ std::vector<float> Ltx2WaveformToLogMel(const Ltx2AudioProcessorConfig& config,
std::vector<double> window(static_cast<size_t>(n_fft));
for (int64_t i = 0; i < n_fft; ++i) {
window[static_cast<size_t>(i)] =
0.5 - 0.5 * std::cos(2.0 * M_PI * static_cast<double>(i) / static_cast<double>(n_fft));
0.5 - 0.5 * std::cos(2.0 * std::numbers::pi_v<double> *
static_cast<double>(i) / static_cast<double>(n_fft));
}

// `center=True, pad_mode="reflect"`: pad n_fft/2 on BOTH sides, so frame 0 is
Expand Down Expand Up @@ -1081,7 +1084,8 @@ std::vector<float> 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<double>(f) / static_cast<double>(n_fft);
const double omega = -2.0 * std::numbers::pi_v<double> *
static_cast<double>(f) / static_cast<double>(n_fft);
for (int64_t i = 0; i < n_fft; ++i) {
const double angle = omega * static_cast<double>(i);
real += frame[static_cast<size_t>(i)] * std::cos(angle);
Expand Down
4 changes: 0 additions & 4 deletions src/vllm/model_executor/models/ltx2_video_vae.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
5 changes: 3 additions & 2 deletions tests/vllm/models/test_vocoder1d.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
// ABSOLUTE floor under any epsilon, which would accept almost anything.
#include <cmath>
#include <cstdint>
#include <numbers>
#include <vector>

#include "doctest/doctest.h"
Expand Down Expand Up @@ -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<double> / 2.0;
std::vector<float> x{static_cast<float>(x0)};
const std::vector<float> alpha{1.0F};
const std::vector<float> beta{1.0F};
Expand All @@ -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<double> / 4.0;
std::vector<float> x{static_cast<float>(x0)};
const std::vector<float> alpha{2.0F};
vllm::vocoder1d::SnakeActivation(x, 1, 1, alpha, /*beta=*/nullptr, /*logscale=*/false);
Expand Down
Loading