[V1] Optimize handling of sampling metadata and req_ids list

[njhill]

• Move the current SamplingMetadata object to a field in the persistent batch, updated only when the batch changes rather than constructed every step
• Keep input_batch.req_ids sized to the number of requests in the batch, so that anywhere that iterates over it doesn't need to slice (copy) the list or keep track of the separate request count. It is still updated in-place

[github-actions]

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @njhill.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[njhill]

@WoosukKwon this is the first step, I am working on follow-on simplification for the penalty parameters, etc.

[njhill]

@WoosukKwon apologies, I am looking into the test failure.

[njhill]

@WoosukKwon the test failure should be fixed now... the shared apply penalties code was doing in-place unsqueezes on the sampling penalty tensors - which I think is a bad thing to do but didn't cause a problem before because we were passing new slices every step.

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @njhill.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[WoosukKwon]

Hi @njhill, do you mind if we merge #12193 first and review this PR? I'd like to prioritize the spec decode PR as it already got rebased many many times.

[njhill]

@WoosukKwon that's fine with me.

[mergify]

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @njhill.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

[WoosukKwon]

@njhill Sorry for the delay. I will review this PR once it's rebased.

[njhill]

@WoosukKwon I have now rebased. #13360 partially overlaps with this (e,g. I simplified some of the min_tokens handling in this one but have refactored completely in the other one based on the new abstraction). But I think it would be fine to get this in first and I can rebase the other one if you're ok with that.

[WoosukKwon]

@njhill I'm not sure it's worthwhile to change from [] to ().
I did a microbenchmark:

N = 1024
x = []
# List
start = time.perf_counter()
for i in range(N):
    x.append([])
end = time.perf_counter()
print(f"list: {(end - start) * 1000:.3f} ms")

y = []
# Tuple
start = time.perf_counter()
for i in range(N):
    y.append(())
end = time.perf_counter()
print(f"tuple: {(end - start) * 1000:.3f} ms")

I find that adding 1024 (maximum number of requests in the batch) empty lists only takes 80-90 us. While using tuple reduces this time to 30-40 us, I think the 50 us gap (in the worst case) cannot justify the extra complexity here. When the batch size is 32, the gap becomes even smaller (7 us vs 2 us). WDYT?

[njhill]

@WoosukKwon I agree it's not worth any extra complexity. Just might as well use () where it doesn't otherwise make any difference to the code. Let me check and revert where such changes were made..

[WoosukKwon]

@njhill I think changing List to Sequence itself is increasing complexity? After that, we need to consider whether it's a tuple or list. I'd prefer to keep using List and [] if the performance is the only concern.

[njhill]

@WoosukKwon sure, let me revert those too. I think mostly we don't need to consider the tuple/list difference because these are args or fields that would be considered read-only.

[njhill]

@WoosukKwon I need to fix up some of the gpu_model_runner tests, but I'll wait for your first review to make sure you are good with the changes overall before spending time on that.

[WoosukKwon]

Amazing. Looks much cleaner! 😄

[WoosukKwon]

What is this change for?

[njhill]

It avoids creating a new list, just trims the existing one down to num_scheduled_spec_tokens, since any later spec token ids are essentially discarded anyhow.

[WoosukKwon]

Got it! Maybe worth a comment.

[WoosukKwon]

LGTM! Very nice simplification!