outlines icon indicating copy to clipboard operation
outlines copied to clipboard

Add CFG integration to VLLM

Open miftahmoha opened this issue 1 year ago • 17 comments

This test failed, but it's related to the llama.cpp integration.

1

Fixes https://github.com/outlines-dev/outlines/issues/780.

miftahmoha avatar Apr 04 '24 21:04 miftahmoha

Fixed the failing test! Could you add a simple test here ?

rlouf avatar Apr 08 '24 16:04 rlouf

@rlouf I see that you added the test, I guess it's done?

https://github.com/outlines-dev/outlines/blob/9f15e28edcc6c7d623dfe12445afdfafeecd1cfb/tests/generate/test_integration_vllm.py#L236

miftahmoha avatar Apr 11 '24 09:04 miftahmoha

Would you mind removing the pytest.mark.xfail decorator? The test is not run currently.

rlouf avatar Apr 11 '24 10:04 rlouf

@rlouf Done.

miftahmoha avatar Apr 12 '24 00:04 miftahmoha

Ok I'll update the vLLM install so we can run these tests in CI. I'll edit your PR directly so don't touch anything for now!

rlouf avatar Apr 12 '24 07:04 rlouf

I get an error on the CFG test, it generates tokens that are not allowed. We need to determine if it's a problem with the CFGGuide or the integration itself. Can you reproduce this locally?

E               lark.exceptions.UnexpectedCharacters: No terminal matches '.' in the current parser context, at line 1 col 12
E               
E               --2-2-111.9.
E                          ^
E               Expected one of: 
E                       * RPAR
E                       * STAR
E                       * MINUS
E                       * SLASH
E                       * PLUS
E               
E               Previous tokens: Token('NUMBER', '111.9')

../../../.conda/envs/outlines-dev/lib/python3.10/site-packages/lark/lexer.py:598: UnexpectedCharacters

rlouf avatar Apr 12 '24 11:04 rlouf

@rlouf Using the debugger, it seems that is something related to the parser. I'm going to have a look at that.

miftahmoha avatar Apr 13 '24 10:04 miftahmoha

That's what I feared

rlouf avatar Apr 13 '24 12:04 rlouf

@rlouf After inspecting a little bit, generate.cfg first should be fixed since it has a bug.

The def copy(self) -> "CFGGuide" should just return self instead of CFGGuide(self.cfg_string, self.tokenizer), it loses the regex_fsm when copying.

Concerning the error, the problem comes from the built regex from interactive.accepts(). It allows some tokens that interactive.accepts() doesn't ('.' in the error above).

I could reproduce the same type of error using generate.cfg without the VLLM integration.

miftahmoha avatar Apr 14 '24 10:04 miftahmoha

That's what I feared

I'm going to fix it, I'll open a draft PR when the time comes.

miftahmoha avatar Apr 26 '24 21:04 miftahmoha

Thank you! I'm sorry for lack of responsiveness here, I'm really busy with other things but will have more time soon :)

rlouf avatar Apr 27 '24 11:04 rlouf

No problem!

miftahmoha avatar Apr 28 '24 20:04 miftahmoha

@rlouf After inspecting a little bit, generate.cfg first should be fixed since it has a bug.

The def copy(self) -> "CFGGuide" should just return self instead of CFGGuide(self.cfg_string, self.tokenizer), it loses the regex_fsm when copying.

Concerning the error, the problem comes from the built regex from interactive.accepts(). It allows some tokens that interactive.accepts() doesn't ('.' in the error above).

I could reproduce the same type of error using generate.cfg without the VLLM integration.

I have partially debugged the bug in generate.cfg w/ this PR: https://github.com/outlines-dev/outlines/pull/865

leloykun avatar May 08 '24 11:05 leloykun

@miftahmoha @rlouf

It's not a good idea to return self in def copy cuz then we'd end up using the same fsm for all prompts in the batch. See: https://github.com/outlines-dev/outlines/blob/4f8433d8d6633b0780c3a6c27981f9adffbe49f5/outlines/generate/api.py#L189

leloykun avatar May 08 '24 11:05 leloykun

IMO it's better to add another method, say def deep_copy, which returns CFGGuide(self.cfg_string, self.tokenizer, self.refex_fsm, ...) and use that instead in reorder_fsms (and just in reorder_fsms for now)

Alternatively, we can somehow make CFGGuide pickle-able and just use copy.deepcopy in reorder_fsms

leloykun avatar May 08 '24 11:05 leloykun

Concerning the error, the problem comes from the built regex from interactive.accepts(). It allows some tokens that interactive.accepts() doesn't ('.' in the error above).

Hi @miftahmoha , I didnt get you. Could you please expand more on above? Thanks!

ekagra-ranjan avatar May 14 '24 02:05 ekagra-ranjan

@ekagra-ranjan Sorry for the late answer, there are two mechanisms that are in the works here.

First, the one that gives the next possible tokens following the CFG (tokens in the context of a lexer in compilers) which is interactive.accepts(). Second, one that takes those tokens and builds a regex out of them for guided generation.

There is a problem in the second mechanism.

miftahmoha avatar May 25 '24 21:05 miftahmoha