[Frontend][Core] Update Outlines Integration from FSM to Guide

[br3no]

This PR updates the Outlines Integration from FSM to the new Guide interface.

Since I'm not sure where to place the change, I added both labels [Frontend] and [Core].

FIX #3715

BEFORE SUBMITTING, PLEASE READ THE CHECKLIST BELOW AND FILL IN THE DESCRIPTION ABOVE

---

PR Checklist (Click to Expand)

Thank you for your contribution to vLLM! Before submitting the pull request, please ensure the PR meets the following criteria. This helps vLLM maintain the code quality and improve the efficiency of the review process.

PR Title and Classification

Only specific types of PRs will be reviewed. The PR title is prefixed appropriately to indicate the type of change. Please use one of the following:

• [Bugfix] for bug fixes.
• [CI/Build] for build or continuous integration improvements.
• [Doc] for documentation fixes and improvements.
• [Model] for adding a new model or improving an existing model. Model name should appear in the title.
• [Frontend] For changes on the vLLM frontend (e.g., OpenAI API server, LLM class, etc.)
• [Kernel] for changes affecting CUDA kernels or other compute kernels.
• [Core] for changes in the core vLLM logic (e.g., LLMEngine, AsyncLLMEngine, Scheduler, etc.)
• [Hardware][Vendor] for hardware-specific changes. Vendor name should appear in the prefix (e.g., [Hardware][AMD]).
• [Misc] for PRs that do not fit the above categories. Please use this sparingly.

Note: If the PR spans more than one category, please include all relevant prefixes.

Code Quality

The PR need to meet the following code quality standards:

• We adhere to Google Python style guide and Google C++ style guide.
• Pass all linter checks. Please use format.sh to format your code.
• The code need to be well-documented to ensure future contributors can easily understand the code.
• Include sufficient tests to ensure the project to stay correct and robust. This includes both unit tests and integration tests.
• Please add documentation to docs/source/ if the PR modifies the user-facing behaviors of vLLM. It helps vLLM user understand and utilize the new features or changes.

Notes for Large Changes

Please keep the changes as concise as possible. For major architectural changes (>500 LOC excluding kernel/data/config/test), we would expect a GitHub issue (RFC) discussing the technical design and justification. Otherwise, we will tag it with rfc-required and might not go through the PR.

What to Expect for the Reviews

The goal of the vLLM team is to be a transparent reviewing machine. We would like to make the review process transparent and efficient and make sure no contributor feel confused or frustrated. However, the vLLM team is small, so we need to prioritize some PRs over others. Here is what you can expect from the review process:

• After the PR is submitted, the PR will be assigned to a reviewer. Every reviewer will pick up the PRs based on their expertise and availability.
• After the PR is assigned, the reviewer will provide status update every 2-3 days. If the PR is not reviewed within 7 days, please feel free to ping the reviewer or the vLLM team.
• After the review, the reviewer will put an action-required label on the PR if there are changes required. The contributor should address the comments and ping the reviewer to re-review the PR.
• Please respond to all comments within a reasonable time frame. If a comment isn't clear or you disagree with a suggestion, feel free to ask for clarification or discuss the suggestion.

Thank You

Finally, thank you for taking the time to read these guidelines and for your interest in contributing to vLLM. Your contributions make vLLM a great tool for everyone!

[simon-mo]

LGTM.

[simon-mo]

I think the test failure in the entrypoints test might be related, and there's merge conflict. 🙏

[br3no]

I'll have a look at the failing frontend test.

[br3no]

I can reproduce the error in the test on my dev environment. The generation does not stop when it should, generating IP addresses like this: 100.101.102.10319216. I'm investigating why this happens and reached out to @rlouf in the outlines discord.

I have pushed some small improvements to the test code:

• corrected the regex for IPv4 the regex was correct. I reverted the change
• changed the pytest fixture to make sure the same python interpreter is used when starting the vLLM server for the integration test (see the warning box here: https://docs.python.org/3/library/subprocess.html#subprocess.Popen)

[navster888]

hey @br3no @simon-mo any updates on getting this PR merged? The pinned version of outlines is preventing us from picking up a bug fix in included in 0.0.40. Is there anyway we can pull the relaxed version constraint into it's own PR to unblock?

[br3no]

I'm having a call with outlines contributors on Thursday. While there is no guarantee we will have a solution for the problem, I'd propose to wait until then. If there's no progress by the end of the week, I'd open a separate PR for the unpinning.

What do you think?

[br3no]

I have opened #4558 because moving to the Guide API will require dottxt-ai/outlines#856 to be fixed first.

[br3no]

I have closed #4558 in favor of this PR. I expect to make progress on this next week. Waiting for dottxt-ai/outlines#874.

[rlouf]

dottxt-ai/outlines#874 merged, thank you for your patience!

[br3no]

Great, thanks for the support @rlouf!

Can you tell already when you plan to release?

[rlouf]

Next week I think. I need to make sure other downstream libraries have pinned the outlines version to avoid surprises.

[njhill]

Sorry, missed @simon-mo tagging me above. Yes we encountered the same problem and had been thinking to introduce a LogitsProcessorFactory abstract class that can be included in the SamplingParams instead of LogitsProcessors (but allowing those too for backwards compatibility. This could have both create_processor() and return_processor(lp) methods. Latter not required but could be used for pooling.

vLLM would then ensure to call this separately for each sequence. Stateless LP factories can just return a constant LP from the method.

Note this is currently also problem if a list of prompts is passed in the API, and/or if n > 1 like you said (including beam search).

WDYT?

@maxdebayser has started prototyping this.

[br3no]

@njhill I like the direction of your proposal. This would allow us to invert control and get rid of the 10 lines setting up guided decoding in the create_chat_completion method. Is there a PR with @maxdebayser's sketch?

While I think this is the right thing to do on the long run, I believe we should fix this problem ASAP. Would you mind having a look at the code in this PR which handles this issue by pushing the cache one level down? I believe this would work nicely as a band-aid until the refactoring you proposed is implemented.

The core changes are:

• we are no longer caching the LogitProcessors. They are now created on each request anew.
• the LogitsProcessors no longer reset state – so all n sequences can safely share the same state cache in the lifetime of one request.
• the Guide object is cached globally for every guide/tokenizer pair. This is the expensive thing we don't want to recompute on every request. (this is how we were caching the LogitProcessors)

[br3no]

PS: this PR is ready for review @simon-mo

I'm just waiting for outlines to be released, so that we can get rid of the regression in the tests.

[maxdebayser]

Hi @br3no, we also found a problem with the FSM state being shared between sequences. This curl here causes a crash:

curl http://localhost:8000/v1/completions   -H "Content-Type: application/json"   -d '{
    "model": "<MY_MODEL>",
    "prompt": ["An example of a json document: ", "Another example of a json document: "],
    "max_tokens": 100,
    "temperature": 0,
    "guided_decoding_backend": "outlines",
    "response_format": {"type":"json_object"},
    "logit_bias": {"100": -100}
  }'

We have a sketch for a PR here: IBM/vllm#38 . It uses factories like @njhill mentioned, so that each sequence can have it's own logits processor copy.

The changes in our PR solve this particular issue, but I think the CFGLogitsProcessor would still crash if the sequence is preempted with the recompute policy. But I don't know yet how to test this hypothesis.

[maxdebayser]

@br3no , I've tested your changes with the curl command above. The code doesn't crash anymore, but I think the output ends prematurely:

{
  "id": "cmpl-d1e40aa8af734fcc98238fa5e0c2ecac",
  "object": "text_completion",
  "created": 1717517730,
  "choices": [
    {
      "index": 0,
      "text": "\n\n\n{\n",
      "logprobs": null,
      "finish_reason": "stop",
      "stop_reason": null
    },
    {
      "index": 1,
      "text": "\n\n\n{",
      "logprobs": null,
      "finish_reason": "stop",
      "stop_reason": null
    }
  ],
  "usage": {
    "prompt_tokens": 18,
    "total_tokens": 29,
    "completion_tokens": 11
  }
}

So while I do think that your changes are an improvement, I think we need to add the factory PR as well to deal with stateful LogitsProcessors properly. Just for reference, the curl request returns this on our PR:

{
  "id": "cmpl-861186441f9c459c981fcab5abd33f5b",
  "object": "text_completion",
  "created": 1717515348,
  "choices": [
    {
      "index": 0,
      "text": "\n\n\n{\n\"name\" \n: \n\"John Doe\"\n,\n\"age\" \n: 30\n,\n\"city\" \n: \n\"New York\"\n}\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n",
      "logprobs": null,
      "finish_reason": "length",
      "stop_reason": null
    },
    {
      "index": 1,
      "text": "\n\n\n{\n\"name\" \n: \n\"John Doe\"\n,\n\"age\" \n: 30\n,\n\"city\" \n: \n\"New York\"\n}\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n",
      "logprobs": null,
      "finish_reason": "length",
      "stop_reason": null
    }
  ],
  "usage": {
    "prompt_tokens": 18,
    "total_tokens": 218,
    "completion_tokens": 200
  }
}

[maxdebayser]

LGTM

[maxdebayser]

Actually, if I run

curl http://localhost:8000/v1/completions   -H "Content-Type: application/json"   -d '{
    "model": "<MY_MODEL>",
    "prompt": ["An example of a json document: ", "Another example of a json document: "],
    "max_tokens": 100,
    "temperature": 0,
    "guided_decoding_backend": "outlines",
    "response_format": {"type":"json_object"},
    "logit_bias": {"100": -100}
  }'

followed by

curl http://localhost:8000/v1/completions   -H "Content-Type: application/json"   -d '{
    "model": "<MY_MODEL>",
    "prompt": ["An example of a json document: "],                                        
    "max_tokens": 100,
    "temperature": 0,
    "guided_decoding_backend": "outlines",
    "response_format": {"type":"json_object"},
    "logit_bias": {"100": -100}
  }' | jq

I get this crash:

    File "/home/develop/.local/lib/python3.11/site-packages/vllm/model_executor/guided_decoding/outlines_logits_processors.py", line 47, in __call__
    instruction = self._guide.get_next_instruction(
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/develop/.local/lib/python3.11/site-packages/outlines/fsm/guide.py", line 349, in get_next_instruction
    interactive.exhaust_lexer()
  File "/opt/vllm/lib/python3.11/site-packages/lark/parsers/lalr_interactive_parser.py", line 52, in exhaust_lexer
    return list(self.iter_parse())
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/opt/vllm/lib/python3.11/site-packages/lark/parsers/lalr_interactive_parser.py", line 43, in iter_parse
    for token in self.lexer_thread.lex(self.parser_state):
  File "/opt/vllm/lib/python3.11/site-packages/lark/lexer.py", line 674, in lex
    raise UnexpectedToken(token, e.allowed, state=parser_state, token_history=[last_token], terminals_by_name=self.root_lexer.terminals_by_name)
lark.exceptions.UnexpectedToken: Unexpected token Token('LBRACE', '{') at line 7, column 2.
Expected one of: 
	* UNESCAPED_STRING
	* RBRACE
Previous tokens: [Token('LBRACE', '{')]

If I restart the server and just send the curl request with a single prompt several times, only the first request generates useful JSON. After that it returns:

{
  "id": "cmpl-948cb1c4033b40f1a07846bed5ac4de9",
  "object": "text_completion",
  "created": 1717518689,
  "model": "/llama_eval_storage/LLaMa/models/hf/7B-F",
  "choices": [
    {
      "index": 0,
      "text": "\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n",
      "logprobs": null,
      "finish_reason": "length",
      "stop_reason": null
    }
  ],
  "usage": {
    "prompt_tokens": 9,
    "total_tokens": 109,
    "completion_tokens": 100
  }
}

I'm testing with LLama2 7b and outlines==0.0.41

[br3no]

@maxdebayser thanks for looking into it!

we also found a problem with the FSM state being shared between sequences

I have just looked into Outlines and while RegexGuide is thread-safe, CFGGuide is not.

I'll check if I can increase the size of the band-aid a bit to deal with this case...

[br3no]

I have pushed a commit that comes close to what was there before and at the same time does not lead to crashes on n > 1. It's still not correct, though...

Every CFGGuide can only be used by one sequence at a time. We need the factory idea in IBM/vllm#38. Note that the performance will still be horrible, since we will create a new Guide for every sequence. This is unavoidable, unfortunately.

It probably makes sense to pre-build a (large) pool of CFGGuides for the use-case where request.response_format.type == "json_object". I believe this is a pragmatic and useful solution, since this will be the most common case.

To really solve this, Outlines would need to be changed to make the CFGGuide thread-safe. I believe this would require a large effort.

@rlouf, could you give us your expert opinion on this?

[maxdebayser]

@br3no , I can confirm that your latest commit fixes the problem where state from previous single-sequence requests is carried over to new requests.

[simon-mo]

Thank you for doing this. Please let me know when this PR is ready to be merged!

[simon-mo]

Actually it seems complete given @maxdebayser's comment. I will merge now.