feat(llama2): add template for chat messages #782
Conversation
dave-gray101
changed the title
pkg/model/loader.go
Outdated
| if err := ml.loadTemplateIfExists(modelName, modelFile); err != nil { | ||
| return "", err | ||
| } | ||
| func evaluateTemplate[T any](templateName string, in T, modelPath string, templateMap *(map[string]*template.Template), mutex *sync.Mutex) (string, error) { |
There was a problem hiding this comment.
any specific need for the generics here? I'd keep them only when strictly needed otherwise it is just confusing (what's the catch instead of using an interface?)
There was a problem hiding this comment.
Hey @mudler ! I'm still catching on to go style, but I'll admit I'm a big fan of generics in most cases... probably more than I should be!
I suppose we could rewrite this to interface{} instead of T - personally, I think the T approach is clearer to read, as we could point at line 119 for example to see that TemplateForChatMessage expects and only works with ChatMesageTemplate, and not interface{}. Is there another way to express this type of constraint in go that I've overlooked?
Theoretically, we could just drop the generic parameter and pass it in as interface{} if that convention is already clear in most cases to go programmers :)
There was a problem hiding this comment.
I'm fine either ways. I'm just a bit not super convinced in this case because raises code complexity (see e.g. pointer of maps) for no real catch
pkg/model/loader.go
Outdated
| } | ||
|
|
||
| func (ml *ModelLoader) TemplateForChatMessage(templateName string, messageData ChatMessageTemplateData) (string, error) { | ||
| return evaluateTemplate[ChatMessageTemplateData](templateName, messageData, ml.ModelPath, &(ml.chatMessageTemplates), &(ml.mu)) |
There was a problem hiding this comment.
just wondering, why passing the pointer to the map around?
There was a problem hiding this comment.
Passing the pointer to the map is because evaluateTemplate[T] was generic - See other comment for style choice on why this line in general is generic.
If that function is generic, it can't accept an *ModelLoader receiver, so I just have it accept a pointer to the relevant lock and map... admittedly, a hack, but it's "contained" to the private method here in this file at least.
If we de-generic that method, I would presumably remove it.
There was a problem hiding this comment.
Looks good overall here! great job @dave-gray101 and @tmm1! just few nits from my side but that shouldn't block this
api/openai/chat.go
Outdated
| content = fmt.Sprint(*i.Content) | ||
| if templatedChatMessage == "" { | ||
| log.Warn().Msgf("template \"%s\" produced blank output for %+v. Skipping!", config.TemplateConfig.ChatMessage, chatMessageData) | ||
| continue |
There was a problem hiding this comment.
maybe instead of skipping here would be better to skip templating and using the message as-is? making it disappear would be probably confusing
There was a problem hiding this comment.
Well - I think we'd theoretically want to allow a template to ignore a message and return "" - so the continue here really only avoids appending "" on to a slice of strings, right - would result in one additional \n if we don't skip adding it.
I admit it's probably not super clear that's the only effect of that statement, so I'll see if I can revise it!
The Warn is because I assume rarely the user will intend for a "" to be returned from a template, so we shouldn't call that an outright error, but it also is worth making visible when debug=true since most of the time it's probably a template logic error.
api/openai/chat.go
Outdated
| templatedChatMessage, err := o.Loader.TemplateForChatMessage(config.TemplateConfig.ChatMessage, chatMessageData) | ||
| if err != nil { | ||
| log.Error().Msgf("error processing message %+v using template \"%s\": %v. Skipping!", chatMessageData, config.TemplateConfig.ChatMessage, err) | ||
| continue |
There was a problem hiding this comment.
However, in this case, I think I see a much stronger argument for falling back on the sprintf formatting, as long as we keep the error message for the template error :)
…plexity wtill, but cleans up the weird map pointers.
I was planning on squash merging the PR to a single commit, but I can rewrite history into a new branch and merge as several if that's the style we prefer. |
|
@dave-gray101 that's fine, just squash them during merge |
mudler
changed the title
4 participants
Description
Lays some of the groundwork for LLAMA2 compatibility as well as other future models with complex prompting schemes.
sprintfmethod.Big thanks to @tmm1 who helped work out the proper LLAMA2 prompt text as well as helping with testing!
Notes for Reviewers
This obviously isn't 100% compatibility with LLAMA2. We'll need some upstream changes for the larger models, as well as the proper BOS/EOS support... but we might as well get started now.
Signed commits