sampling : one sequence per sampling context#3601
Conversation
|
I won't get a chance to review this in depth until tomorrow. I think my original thinking was that it would require less changes/manual management to do it the way I did with sampling contexts being automatically created from default settings (including a default grammar if provided). You also could manually manage it if you wanted, by just manually inserting the sequence-specific context and setting grammar to NULL when initializing the main sampling context. (The default settings only got used if it looked for a sequence-specific state and didn't find one.) I'm not sure that's a good enough justification for the complexity, and I don't have any problem with the general idea your proposing. @FSSRepo said we should make the init function just take the sampling params instead of gpt_params. That part I definitely agree with. (If we want to move this stuff out of common and into llama.cpp that also would make it easier.) |
KerfuffleV2
left a comment
There was a problem hiding this comment.
Not too much to add to what I said before. I thought this change would require more work fixing the examples, but apparently not. They all seem to compile. I tried a few, still seems to work.
The only thing I'd say is that changing the init function to just take parsing params and not gpt_params should probably be done before this is merged.
|
Yes, I already have a few more changes to |
|
Superseded by #3624 |
ref #3543
Simplify
llama_sampling_contextto contain information for only one sequenceI'm working on tree-based speculative decoding and so far it seems to me this simplification would make things easier