-
Notifications
You must be signed in to change notification settings - Fork 586
Add --eot_line option #33
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
Conversation
15b6e4c to
1a95cf3
Compare
|
There's definitely a need for this. If you notice in the shell demo there's this It does change the function interface slightly and presence/absence of whitespace like "\n" can have subtle effects on quality, so the details require a bit of care. |
austinvhuang
left a comment
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.
This is really useful. let's bring this into dev - if folks have thoughts on the comments below or anything else feel free to chime in.
| hwy::ThreadPool& inner_pool, const InferenceArgs& args, | ||
| int verbosity, const gcpp::AcceptFunc& accept_token) { | ||
| int verbosity, const gcpp::AcceptFunc& accept_token, | ||
| std::string &eot_line |
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.
can eot_line be const?
| "estimate of " | ||
| "how many concurrent threads are supported.", | ||
| 2); | ||
| visitor(eot_line, "eot_line", std::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.
@jan-wassenberg the way the CLI arg parsing works, are there certain characters that would be inexpressible (eg if you wanted some other whitespace like '\t' or ' '?
Another consideration to think about is, is end-of-prompt best specified as:
- a string?
- a character?
- a token ID?
I wonder if either a character or token ID might be preferable to string (and then '\n' becomes a special case), but open to having other perspectives chime in here.
OTOH the nice thing about using a line string is that:
- it can easily be entered in interactively (not always the case for a rare token/character)
- if you need to allow for the entire vocabulary, you can effectively use a unique-by-design string as a quasi-out-of-vocabulary sentinel token.
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 suppose we can post-process the string to implement escaping, then we can express anything, right?
| if (line == eot_line) { | ||
| break; | ||
| } | ||
| prompt_string += line + "\n"; |
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 think this makes inputs that don't end in "\n" inexpressible. This might be okay, but would be good to better understand the implications for quality.
Added
--eot_lineoption.This allows multi lines input.
For example, we can use the following prompt with
--eot_line EOT.