[V1] Wrapper which plumbs request-level logits processors into vLLM batch-level logits processing

[afeldman-nm]

Purpose

This PR adds support for passing request-level logits processors into the vLLM V1 engine. This is accomplished using a wrapper which builds a batch-level logits processor class out of a Callable request-level logits processor that complies with the interface described here https://docs.vllm.ai/en/v0.9.2/api/vllm/logits_process.html

Test Plan

Unit-test wraps request-level logits processor, passes to batch-level logits processor and confirms correct behavior

Test Result

Passes

Documentation update

Will be provided in #22919

[njhill]

I think we should call it RequestLogitsProcessor

[njhill]

The contents of this class looks like the min tokens LP ... is this just a copy-pasted temporary state before updating it with the wrapper LP impl?

[mergify]

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

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

[WoosukKwon]

@afeldman-nm @njhill Do we really want to support this feature? I thought we decided to deprecate it.

[afeldman-nm]

@afeldman-nm @njhill Do we really want to support this feature? I thought we decided to deprecate it.

Hi Woosuk. This PR represents a compromise solution. New logits processors should subclass the V1 LogitsProcessor base class to create a new type of logits processor that operates at batch granularity. However, there are entire libraries of pre-existing V0-style logits processor implementations that operate at the request granularity, for example

https://github.com/NVIDIA/logits-processor-zoo

so the purpose of this PR is to provide the thinnest possible wrapper for plumbing these into the logits processor extensibility interface, without making any changes to the interface defined in #19912

This way we are not committing to support V0-style processors in the engine internals/interface, but we are also still providing a solution for existing logits processor "zoos".

In the long term, anyone with a V0 style logits processor should upgrade their implementation for the perf benefits

[afeldman-nm]

@afeldman-nm @njhill Do we really want to support this feature? I thought we decided to deprecate it.

Once this PR and the logits processor documentation PR land, it might make sense for me to file an issue against https://github.com/NVIDIA/logits-processor-zoo and any other similar repo...

[njhill]

Thanks @afeldman-nm

[njhill]

@afeldman-nm @njhill Do we really want to support this feature? I thought we decided to deprecate it.

@WoosukKwon IMO we do want to support it for users that want to experiment and/or where performance isn't critical, but with the understand that vectorized LPs should be used for performance. As well as support for all the preexisting impls as @afeldman-nm said.

It's much easier to implement something to mutate the LPs of a single request.

[njhill]

Thanks @afeldman-nm, the class looks good to me now, just a couple of nits remaining

[njhill]

I guess we don't need/want any args here. So that they can call super().__init__().

But we want the contract to be that their subclass of this must have those args, since that's expected of our regular LogitsProcessors (and they can also then make use of them if they need to).

I'm just not sure how to reflect that in the abstract class apart from via comments/doc.

[afeldman-nm]

If I am understanding your comment correctly, yes I think these arguments cannot be skipped unfortunately. I added a docstring explaining that these arguments may be used by the subclass but must be present regardless.

This is also worth mentioning in the docs, however I think I will leave documentation of this per-request logits processor wrapper to the docs PR.

[njhill]

I'm was saying that we don't actually need the args here if we require the subclasses to have such a constructor.

However on second thoughts maybe this is better because then implementations don't need to define an init method at all if they don't need any of the args (like your dummy impls for example).

[njhill]

Thanks @afeldman-nm! LGTM just a couple of tiny things

[njhill]

Thanks @afeldman-nm !

[jwkirchenbauer]

@afeldman-nm @njhill Do we really want to support this feature? I thought we decided to deprecate it.

Hi Woosuk. This PR represents a compromise solution. New logits processors should subclass the V1 LogitsProcessor base class to create a new type of logits processor that operates at batch granularity. However, there are entire libraries of pre-existing V0-style logits processor implementations that operate at the request granularity, for example

https://github.com/NVIDIA/logits-processor-zoo

so the purpose of this PR is to provide the thinnest possible wrapper for plumbing these into the logits processor extensibility interface, without making any changes to the interface defined in #19912

This way we are not committing to support V0-style processors in the engine internals/interface, but we are also still providing a solution for existing logits processor "zoos".

In the long term, anyone with a V0 style logits processor should upgrade their implementation for the perf benefits

Note that something here is still a little opaque about how to use logits processors that were developed in the V0 style.

If an old school logits processor is run in version 0.10.1.1, then you land at the export VLLM_USE_V1=0 solution because you get the message "vLLM V1 does not support per request user provided logits processors."
However, then you get the message "ValueError: Logits processors are not supported in multi-step decoding" which was added here: 37efc63#diff-c89ac25bd066e936e80260d21be63c7d2379cfedc371a9ff288fb5ba02ae1350R689-R1985

vllm/vllm/engine/llm_engine.py

Line 689 in 37efc63

"Logits processors are not supported in multi-step decoding")

The fix reach on that zoo repo's side was a downgrade to 0.10.0, which also just "worked" for me as well.

All in all though I get a sense that something needs to be resolved here but I have no experience with either library to understand why/how this is happening or whether it will get resolved.
Usecase is actually this repo, where watermarking methods are implemented as logits processors: https://github.com/THU-BPM/MarkLLM
The current state of vllm essentially breaks this kind of special LP usecase I think, and I assume there are more of them.