-
Notifications
You must be signed in to change notification settings - Fork 984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(core): support configuring stop words in model config #3209
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also test the behavior locally (e.g with screen recording). Otherwise LGTM
crates/tabby-common/src/config.rs
Outdated
@@ -252,6 +254,9 @@ pub struct HttpModelConfig { | |||
/// Used by Completion API to construct a chat model. | |||
#[builder(default)] | |||
pub chat_template: Option<String>, | |||
|
|||
#[builder(default)] | |||
pub stop_words: Vec<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub stop_words: Vec<String>, | |
pub additional_stop_words: Vec<String>, |
@@ -4,6 +4,7 @@ use trie_rs::{Trie, TrieBuilder}; | |||
|
|||
pub struct StopConditionFactory { | |||
stop_trie_cache: DashMap<String, Trie<u8>>, | |||
model_stop_words: Vec<String>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
model_stop_words: Vec<String>, | |
stop_words_from_model_config: Vec<String>, |
when using the following config, I can reproduce the error: [model.completion.http]
kind = "ollama/completion"
model_name = "qwen2.5-coder:7b-base"
api_endpoint = "http://localhost:11434"
prompt_template = "<|fim_prefix|>{prefix}<|fim_suffix|>{suffix}<|fim_middle|>"
stop_words = [] but some of the stop words seems to be not working, even with the following config: [model.completion.http]
kind = "ollama/completion"
model_name = "qwen2.5-coder:7b-base"
api_endpoint = "http://localhost:11434"
prompt_template = "<|fim_prefix|>{prefix}<|fim_suffix|>{suffix}<|fim_middle|>"
stop_words = ["<|endoftext|>", "<|file_sep|>"] still working on it and trying to figure out why |
455b59e
to
2e645bd
Compare
crates/tabby-inference/src/code.rs
Outdated
let stop_condition_factory = match config { | ||
Some(ModelConfig::Local(config)) => StopConditionFactory::with_stop_words( | ||
config.additional_stop_words.unwrap_or_default(), | ||
), | ||
Some(ModelConfig::Http(config)) => StopConditionFactory::with_stop_words( | ||
config.additional_stop_words.unwrap_or_default(), | ||
), | ||
_ => StopConditionFactory::default(), | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let stop_condition_factory = match config { | |
Some(ModelConfig::Local(config)) => StopConditionFactory::with_stop_words( | |
config.additional_stop_words.unwrap_or_default(), | |
), | |
Some(ModelConfig::Http(config)) => StopConditionFactory::with_stop_words( | |
config.additional_stop_words.unwrap_or_default(), | |
), | |
_ => StopConditionFactory::default(), | |
}; | |
let additional_stop_words = match config { | |
Some(ModelConfig::Local(config)) => config.additional_stop_words.unwrap_or_default(), | |
... | |
}; | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the stop_condition_factory
here means all the stop words, not only the additional stop words, are we really want to rename the stop_condition_factory
to additional_stop_words
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm asking a restructure, rather than a renaming.
let additional_stop_words = match config {
Some(ModelConfig::Local(config)) => config.additional_stop_words.unwrap_or_default(),
...
};
let stop_condition_factory = StopConditionFactory::with_stop_words(additional_stop_words).
So you don't need have to write StopConditionFactory::with_stop_words
multiple times.
13d9f9d
to
9c85c1d
Compare
completion_model: Option<ModelConfig>, | ||
completion_model: &Option<ModelConfig>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the config passes in will be used later, so this must change to a borrowed args, or it will take over the life time of the config passed in
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend just clone it in this particular case.
9c85c1d
to
abcfe6b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix unit test
fix #3188