Skip to content

Commit 2bf417f

Browse files
committed
fix(windows): close Logprobs shadow review gaps (#465)
Rename the slice length parameter without changing its function type, and cover both installed declaration and source definition independently. Characterize a nonzero row slice whose width differs from its requested row count so a wrong stride cannot pass. FOLLOWING_AGENTS_PROTOCOL Following-Agents-Protocol: true AI-Assisted: true Assisted-by: Codex:GPT-5 [Codex]
1 parent 2047200 commit 2bf417f

6 files changed

Lines changed: 109 additions & 5 deletions

File tree

.agents/specs/windows-msvc-logprobs-shadow.md

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -81,4 +81,32 @@ complete native log contains any other diagnostic family.
8181

8282
## Outcome
8383

84-
Pending RED, implementation, and immutable gate evidence.
84+
The structural regression failed first with the exact shadowing declaration
85+
`LogprobsTensors::slice_request(int req_idx, int num_positions) const`. The
86+
implementation renames only that parameter to `request_num_positions` in the
87+
declaration and definition, assigns `out.num_positions` explicitly from it, and
88+
uses it for the slice end. The function types, public symbol, selected rows,
89+
payload values, warning gates, and release surfaces remain unchanged.
90+
91+
The first structural regression read only `outputs.cpp`, so restoring the
92+
shadowing parameter in the installed header alone passed both the focused test
93+
and direct checker. The final regression recognizes the exact `slice_request`
94+
declaration and definition independently, limits the C4458 assertion to those
95+
two scopes, and ignores comments and the valid `empty_cpu` parameter of the
96+
same name. Header-only and source-only mutations each fail their own subtest;
97+
mutating both fails both.
98+
99+
The first semantic fixture also made the row width and requested row count both
100+
`2`, so using `request_num_positions` as the flat-vector stride passed. The
101+
final payload has width `3`, requests two rows from nonzero offset `1`, and
102+
checks all six token IDs and logprobs. The wrong-stride mutation selects four
103+
values beginning in the previous row and fails both payload assertions, while
104+
the correct member assignment, slice end, offset, and width pass.
105+
106+
The focused structural test and direct production portability checker pass, as
107+
does the complete 69-test Windows portability suite. Clean Release CPU and
108+
Vulkan source closures built the focused linked tests; in both configurations,
109+
`test_sampling_metadata` passes 7/7 cases and 61/61 assertions, while
110+
`test_outputs` passes 9/9 cases and 48/48 assertions. Native MSVC CPU and Vulkan
111+
reruns remain the authoritative C4458 `/W4 /WX` validation and are not inferred
112+
from Linux.

docs/USAGE.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1295,6 +1295,11 @@ SAMPLED from — a token top-k masked away reads `-inf` there and its true value
12951295
under the raw pair. It is selectable by constructing a `Sampler` directly; there
12961296
is no config, CLI or request field for it yet.
12971297

1298+
`LogprobsTensors::slice_request(req_idx, request_num_positions)` cuts that
1299+
batch-wide payload by rows. The second argument is the requested row count;
1300+
each row keeps the source tensor's independent `num_tokens_per_position`
1301+
width.
1302+
12981303
The LoRA adapter headers ([`lora/lora_weights.h`](../include/vllm/lora/lora_weights.h),
12991304
[`lora/punica.h`](../include/vllm/lora/punica.h),
13001305
[`lora/layers.h`](../include/vllm/lora/layers.h)) are present but **not yet wired

include/vllm/v1/outputs.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,8 @@ struct LogprobsTensors {
5656
// token per request per step) and cu_num_generated_tokens is None, so req_idx
5757
// indexes positions directly. (Our host-vector LogprobsTensors doubles as the
5858
// numpy LogprobsLists twin — no torch/numpy split.)
59-
LogprobsTensors slice_request(int req_idx, int num_positions) const;
59+
LogprobsTensors slice_request(int req_idx,
60+
int request_num_positions) const;
6061
};
6162

6263
} // namespace vllm::v1

src/vllm/v1/outputs.cpp

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,13 @@ LogprobsTensors LogprobsTensors::empty_cpu(int num_positions,
2121
}
2222

2323
LogprobsTensors LogprobsTensors::slice_request(int req_idx,
24-
int num_positions) const {
24+
int request_num_positions) const {
2525
LogprobsTensors out;
26-
out.num_positions = num_positions;
26+
out.num_positions = request_num_positions;
2727
out.num_tokens_per_position = num_tokens_per_position;
2828
const size_t w = static_cast<size_t>(num_tokens_per_position);
2929
const size_t begin = static_cast<size_t>(req_idx);
30-
const size_t end = begin + static_cast<size_t>(num_positions);
30+
const size_t end = begin + static_cast<size_t>(request_num_positions);
3131
out.logprob_token_ids.assign(logprob_token_ids.begin() + static_cast<std::ptrdiff_t>(begin * w),
3232
logprob_token_ids.begin() + static_cast<std::ptrdiff_t>(end * w));
3333
out.logprobs.assign(logprobs.begin() + static_cast<std::ptrdiff_t>(begin * w),

tests/scripts/test_check_windows_portability.py

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -468,6 +468,55 @@ def test_real_tree_closes_observed_msvc_source_warnings(self) -> None:
468468
with self.subTest(contract=contract):
469469
self.assertRegex(active_deepseek, contract)
470470

471+
def test_logprobs_slice_parameter_does_not_shadow_member(self) -> None:
472+
header = (REPO / "include/vllm/v1/outputs.h").read_text(
473+
encoding="utf-8"
474+
)
475+
active_header = checker._cpp_structural_view(header)
476+
header_signature = r"\bLogprobsTensors\s+slice_request\s*\("
477+
header_matches = list(re.finditer(header_signature, active_header))
478+
self.assertEqual(
479+
len(header_matches),
480+
1,
481+
"LogprobsTensors::slice_request declaration",
482+
)
483+
header_match = header_matches[0]
484+
header_end = re.match(
485+
r"(?:[^()]|\([^()]*\))*\)\s*const\s*;",
486+
active_header[header_match.end():],
487+
)
488+
self.assertIsNotNone(
489+
header_end,
490+
"LogprobsTensors::slice_request declaration",
491+
)
492+
assert header_end is not None
493+
header_declaration = active_header[
494+
header_match.start():header_match.end() + header_end.end()
495+
]
496+
497+
source = (REPO / "src/vllm/v1/outputs.cpp").read_text(
498+
encoding="utf-8"
499+
)
500+
active_source = checker._cpp_structural_view(source)
501+
source_signature = r"\bLogprobsTensors::slice_request\s*\("
502+
span = checker._cpp_function_body_span(source, source_signature)
503+
self.assertIsNotNone(span, "LogprobsTensors::slice_request")
504+
assert span is not None
505+
source_matches = list(re.finditer(source_signature, active_source))
506+
self.assertEqual(
507+
len(source_matches),
508+
1,
509+
"LogprobsTensors::slice_request definition",
510+
)
511+
source_declaration = active_source[source_matches[0].start():span[1]]
512+
513+
for location, declaration in (
514+
("header", header_declaration),
515+
("source", source_declaration),
516+
):
517+
with self.subTest(location=location):
518+
self.assertNotRegex(declaration, r"\bint\s+num_positions\b")
519+
471520
def test_posix_cache_source_requires_exact_not_win32_cmake_guard(self) -> None:
472521
source = "src/vt/cuda/nvfp4_persistent_cache.cpp"
473522
for condition, expected in (("NOT WIN32", {source}), ("WIN32", set()), ("NOT APPLE", set())):

tests/vllm/v1/sample/test_metadata.cpp

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,27 @@ TEST_CASE("LogprobsTensors::empty_cpu has the right shapes") {
187187
CHECK(lt.selected_token_ranks.size() == 3);
188188
}
189189

190+
TEST_CASE("LogprobsTensors::slice_request preserves a multi-row payload") {
191+
LogprobsTensors lt = LogprobsTensors::empty_cpu(/*num_positions=*/4,
192+
/*num_tokens=*/3);
193+
lt.logprob_token_ids = {10, 11, 12, 20, 21, 22,
194+
30, 31, 32, 40, 41, 42};
195+
lt.logprobs = {-1.0f, -1.1f, -1.2f, -2.0f, -2.1f, -2.2f,
196+
-3.0f, -3.1f, -3.2f, -4.0f, -4.1f, -4.2f};
197+
lt.selected_token_ranks = {1, 2, 3, 4};
198+
199+
const LogprobsTensors slice = lt.slice_request(/*req_idx=*/1,
200+
/*num_positions=*/2);
201+
202+
CHECK(slice.num_positions == 2);
203+
CHECK(slice.num_tokens_per_position == 3);
204+
CHECK(slice.logprob_token_ids ==
205+
std::vector<int32_t>{20, 21, 22, 30, 31, 32});
206+
CHECK(slice.logprobs ==
207+
std::vector<float>{-2.0f, -2.1f, -2.2f, -3.0f, -3.1f, -3.2f});
208+
CHECK(slice.selected_token_ranks == std::vector<int32_t>{2, 3});
209+
}
210+
190211
TEST_CASE("SamplerOutput now carries an optional LogprobsTensors payload") {
191212
SamplerOutput out;
192213
out.sampled_token_ids = {{42}, {7}};

0 commit comments

Comments
 (0)