Conversation
juliendenize
force-pushed
the
add_grammar
branch
from
d4f62ea to
84598af
Compare
| r"""Returns a lark grammar that only accepts JSON objects matching the given schema.""" | ||
| return f"start: SAFE_WS? %json {json.dumps(json_schema, ensure_ascii=False)} \nSAFE_WS: /[ \t\r\n]+/" | ||
|
|
||
| def get_matcher(self, lark: str) -> "llg.LLMatcher": |
There was a problem hiding this comment.
why do we have so many functions in this grammar factory? I would have imagined to just have a single "get_grammar" function here
There was a problem hiding this comment.
So get_matcher i could remove.
Otherwise, I tried to match internally implem. Can be changed if needed.
There was a problem hiding this comment.
Don't really see the point of having this method - can we remove?
There was a problem hiding this comment.
yes think it can be added later if we need it but agree that for now it doesn't bring any value.
|
|
||
|
|
||
| class MistralLLGTokenizer: | ||
| r"""Wraps a Tekken tokenizer for use with llguidance. |
There was a problem hiding this comment.
is this better than abstracting Tokenizer here?
There was a problem hiding this comment.
it doesn't have the same scope, here this tokenizer is just meant to be fed to guidance. I could even had a preleading _ to the class name to mark it private tbh
bbrowning
left a comment
bbrowning
left a comment
There was a problem hiding this comment.
Because I have a bit of experience writing Lark grammars for other models, I took a pass at reviewing this one. I'm not an expert in these models output formats, so please feel free to disregard or ignore these comments if they aren't helpful.
src/mistral_common/guidance/data/plain_text_think_grammar.lark.jinja
Outdated
Show resolved
Hide resolved
| if args == {}: | ||
| args = {"type": "object", "properties": {}, "additionalProperties": False} | ||
| return args |
There was a problem hiding this comment.
| if args == {}: | |
| args = {"type": "object", "properties": {}, "additionalProperties": False} | |
| return args | |
| return args or {"type": "object", "properties": {}, "additionalProperties": False} |
| for tool in tools: | ||
| args = _get_tool_args_json(tool) | ||
| grammar_per_tool.append( | ||
| f'({tool_calls_token} SAFE_WS? "{tool.function.name}" {args_token} SAFE_WS? %json ' |
There was a problem hiding this comment.
let's not dup the same string here again
| f"{len(self._special_token_ids)}" | ||
| ) | ||
|
|
||
| def __call__(self, s: str, *args: Any, **kwargs: Any) -> list[int]: |
There was a problem hiding this comment.
you should never be allowed to call bos=True or eos=True here?
There was a problem hiding this comment.
Should we throw a warning/error if *args / **kwargs includes anything? Not sure it's good to just silently swallow the input here
There was a problem hiding this comment.
I replicated what we had internally but they aren't use so i just remove them.
| ) | ||
| assert '"additionalProperties": false' in result | ||
| assert '"properties": {}' in result | ||
| assert not result.endswith(")+") |
Co-authored-by: Julien Denize <40604584+juliendenize@users.noreply.github.com>
4 participants
This PR adds Mistral guidance with the following restrictions:
So right now in the current state of the common PR what we do is:
auto strict=False=> grammar enforce that optional content is before optional tool calls with a name (not enforced) and some json args.auto strict=True=> grammar enforce that optional content is before optional tool calls with a valid name and valid json args.required strict=False=> grammar enforce that optional content is before mandatory tool calls with a name (not enforced) and some json args.required strict=True=> grammar enforce that optional content is before mandatory tool calls with a valid name and valid json args.named strict=False=> grammar enforce that optional content is before optional tool calls with the correct name and some json args.named strict=True=> grammar enforce that optional content is before optional tool calls with the correct name and valid json args.If we add reasoning then it it the same but thinking trace is forced for tokenizer < v11 and optional above