Reimplement Placeholders as singleton containers
#638
Conversation
Also replaced all calls to replace_grammar_node with `Placeholder.set` Placeholder being a singleton container allows globally replacing all "references" without traversing grammar trees.
|
Not yet "in love" with my implementation of |
hudson-ai
changed the title
Placeholders as singleton containersPlaceholders as singleton containers
|
Thanks @hudson-ai ! It's great that you are looking into this. I have a few clarifying questions to see about the overlap of this vs. direct guidance function recursion. This enables the replacement of grammar nodes (placeholder was originally for things that might depend on the model executing the grammar...like the EOS token). We can do things like this: And I think this can support the things in the test cases you have as well. and 
Thoughts? |
|
@slundberg thanks so much for the questions/comments! The "need" for this arose when trying to solve the recursion problem in #636, but I think you're right that that problem may totally evaporate if everything over there is implemented in terms of I'll try my hand at doing that (avoiding stepping on @riedgar-ms's toes as much as I can) and see if I still feel the need for a solution like this, but I expect this PR to evaporate too ;) |
|
I'm well convinced now that recursive calls of guidance-decoated functions are more than sufficient for what I wanted from this. I hadn't quite grokked the beauty of the delayed evaluation of the wrapped function! Because I can't think of a use-case outside of this scope, I'm closing this PR. Thanks @slundberg ! |
…ike container object (#1007) Essentially reopening #638 with some major simplifications allowed by the rust re-write (the serialized grammar now directly supports "references"). Reopening because: #995 still takes far too long to process the large JSON schema. Line-profiling revealed that ~80% of the time spent constructing the `GrammarFunction` is due to `replace_grammar_node`. In particular, this function has to traverse the entire grammar tree, and we potentially have to call it many times if there is a lot of mutual recursion. Simply adding a node type that acts as a container (which we can fill after we decide what its contents should be) side-steps this problem entirely.
This PR re-implements the
Placeholderclass in_grammar_as a singleton container (a fairly minimal subclass ofJoin) in order to support more complex use-cases, e.g. replacing multiple nodes in one grammar or replacement "chains" (see the teststest_multiple_replacementandtest_chained_recursive_replacement, which failed for the old implementation).As a singleton container, we can treat it as a "reference" to an underlying node, which we can swap out globally without having to walk any grammar trees.
Placeholderused to intentionally do nothing. As far as I am aware, it's sole purpose was to be the target ofreplace_grammar_node, which walks a grammar's tree of values and replace a node with another one. This function only has one usage across the repo, namely in__init__, where it's being used to prevent re-generation of stateless grammars on recursive calls toguidancedecorated functions.This re-implementation left
replace_grammar_nodeunused, but I was hesitant to delete it in case it has use-cases beyond setting proper values forPlaceholders.