escaping prompt for cfg_negative_prompt and consecutive prompts in main with interactive #3623
Conversation
…ing space token from suffix when params.escape
…ing space token from suffix when params.escape
…or rm added space token
…or rm added space token
…ed back or rm added space token" This reverts commit 63ba0b6.
examples/main/main.cpp
Outdated
| std::string inp_pfx_str = "\n\n### Instruction:\n\n"; | ||
| std::string inp_sfx_str = "\n\n### Response:\n\n"; | ||
| if (params.escape) { | ||
| process_escapes(inp_pfx_str); | ||
| process_escapes(inp_sfx_str); | ||
| } |
There was a problem hiding this comment.
I'm like 99.9% sure those have to be raw string literals, otherwise it makes no sense, escape sequences in string literals are processed at compile time, so this doesn't do anything
| @@ -631,6 +631,7 @@ bool gpt_params_parse(int argc, char ** argv, gpt_params & params) { | |||
| process_escapes(params.prompt); | |||
| process_escapes(params.input_prefix); | |||
| process_escapes(params.input_suffix); | |||
| process_escapes(sparams.cfg_negative_prompt); | |||
There was a problem hiding this comment.
Keep only this line from this PR and we can merge.
All other changes are not necessary
|
|
||
| if (params.escape) { | ||
| process_escapes(buffer); | ||
| } |
There was a problem hiding this comment.
Do we not also want to keep this to escape the new input?
There was a problem hiding this comment.
I don't think we want to - the model will not generate unescaped stuff
There was a problem hiding this comment.
Would that not result in a situation where we escape the initial prompt but not the following prompts in interactive mode?
There was a problem hiding this comment.
@vvhg1
That's the current behavior, user input in interactive mode tokenizes "\n" as \,n
While enabling escapes for interactive user input would allow "multiline" input via "\n", it would prevent user from writing "\n" literally, making it impossible to do things like copy-pasting code snippets etc.
There was a problem hiding this comment.
@staviq I see your point. What I don't yet understand is, why would we want to treat the initial prompt differently than subsequential prompts?
There was a problem hiding this comment.
Ah, wait - I think I was wrong. Let me check this again later, but we might want to keep this escape
There was a problem hiding this comment.
Ok, I agree we should keep this escape as well
There was a problem hiding this comment.
ok, should all be ready to merge
only when -e:
cfg_negative_prompt gets escaped
consecutive inputs in interactive mode are getting escaped
not sure if I got all cases covered, review needed