Make reverse prompt option act as a stop token in non-interactive sce…

[data-angel]

…narios. This use case is important for exposing the command line main example in non-interactive mode as an API through a tool like Cortex. Without a stop token, the generation will just continue to generate until the token limit is reached, which is not ideal if you know what kind of output you're expecting. This is particularly useful if you're trying to use a few-shot markup like chatML to separate instruction or chat responses.

This change does separate the reverse prompt from interactive mode, so now interactive mode needs to be specified along with reverse prompt for interactive scenarios. The help has been updated accordingly. I could have just added a separate stop token string, but that seemed bloaty.

[SlyEcho]

Reverse prompt will switch to interactive mode right now, but if you pipe in /dev/null to stdin the program will exit. I've used this method to connect main.cpp to Langchain, for example.

[data-angel]

Reverse prompt will switch to interactive mode right now, but if you pipe in /dev/null to stdin the program will exit. I've used this method to connect main.cpp to Langchain, for example.

Thanks for the info - that's good to know! I still like the idea of making it a first-class citizen to align better with the way we typically use other gen models.

[DannyDaemonic]

Nothing in this PR causes this issue. The problem this is trying to fix is the one that pops up when people end their reverse prompts with a space. If you remove the trailing spaces from your reverse prompt, it should work. There's been talk of adding a warning to the program when users include a trailing space. I'd suggest we just remove it.

Suggested change

	size_t extra_padding = params.interactive ? 0 : 2;
	size_t search_start_pos = last_output.length() > static_cast<size_t>(antiprompt.length() + extra_padding)
	? last_output.length() - static_cast<size_t>(antiprompt.length() + extra_padding)
	: 0;
	if (last_output.find(antiprompt.c_str(), search_start_pos) != std::string::npos) {
	if (params.interactive) {
	is_interacting = true;
	set_console_color(con_st, CONSOLE_COLOR_USER_INPUT);
	}

[data-angel]

Not done. I've found this code is necessary in several scenarios. For example, if your stop token is chatML like "<|IM_END|>", it often is tokenized with trailing characters like "\", so if you don't do a fuzzy search for the stop token you miss it in the generated stream. In the non-interactive case, it's not a big deal to have extra trailing characters in the response as that can be easily handled by the code that's processing the response.

[DannyDaemonic]

I think there's a proper way to do this such that it works with both interactive mode and noninteractive mode that involves looking at the next token right when we first get it from llama_sample, before it's printed to the terminal or evaluated by llama_eval. It would be more efficient as well because you're skipping an eval. Of course I don't expect you to do that. It's far from a trivial fix and should be its own pull request anyway.

I guess what's holding me back from approving this is that we've made everyone else adjust their reverse prompts not to include the token that gets split. So we tell people to remove the trailing space ( ) from their reverse prompt, or in your case the equivalent suggestion would be to drop the right angle bracket (>) from your reverse prompt (it may take dropping both |>).

The decision to rigidly enforce the reverse prompt is from before I became a collaborator, so as much as I agree that reverse prompts should not automatically enable interactive mode, this isn't part of that. I'm going to have to wait for someone more senior than me to approve this particular part of the PR.

[data-angel]

I understand your perspective, but I think it would be a mistake to require arcane tokenization knowledge to have people "properly" set a stop token. Better to just set it to the string you want to look for. I agree that the more correct answer is to wire the stop token detection at a deeper level, but as you pointed out that also comes with more risk and complexity. I think this is a fine compromise and the potential for negative impact seems minimal.

[ejones]

Can we split out this hunk into its own PR perhaps? It appears everything else here has consensus and could be landed. Even without this, non-interactive -r would be a net improvement and it would at worst be consistent with the current (buggy) behavior of -r.

[data-angel]

Sure we could, but I feel like it's a pretty concise, contained, low risk change as it sits. Allowing for a more natural expression of the reverse prompt in the non-interactive case I think is far more accessible for users who aren't going to understand how their prompt string is tokenized exactly and more maintainable for those folks down the road. Imagine a case in which your reverse prompt is expressed as "<|IM_END" in your code - you'll need to comment why it's expressed like that as a quirk of the underlying platform so that someone doesn't change it to the more correct looking "<|IM_END|>" and be confused when that doesn't terminate the execution despite being clearly present in the output.

The way the code is currently written, it only changes it in the non-interactive case, which is pretty surgical. IMO the bigger issue with this PR is that it's a breaking change for people who are just using -r to trigger interactivity in their scripts, but I think that part is acceptable and folks seem to agree.

@DannyDaemonic - who should take a look at this to see if they're bothered by allowing reverse prompts that aren't precisely token aligned?

[DannyDaemonic]

How does everyone feel about this approach? I think it's cleaner than having a separate set of exit-program prompts that are just a copy and paste of the antiprompt checks. It may require a few scripts to be updated but with 1305 happening, people can just fix both at once.

@data-angel Are you willing to bring this up to date?

[SlyEcho]

I like this more since it is predictable, if I don't enable interactive, it is not enabled for me.

[data-angel]

@DannyDaemonic Thanks for the feedback! It should be up to date now. Let me know if you need anything else.

[DannyDaemonic]

You should probably also update common.cpp's gpt_params_parse to reflect the test for interactivity because antiprompts no longer trigger interactive mode.
So something like this:

    if (params.prompt_cache_all && 
        (params.interactive || params.interactive_first || params.instruct)) {

[DannyDaemonic]

I think there's a proper way to do this such that it works with both interactive mode and noninteractive mode that involves looking at the next token right when we first get it from llama_sample, before it's printed to the terminal or evaluated by llama_eval. It would be more efficient as well because you're skipping an eval. Of course I don't expect you to do that. It's far from a trivial fix and should be its own pull request anyway.

I guess what's holding me back from approving this is that we've made everyone else adjust their reverse prompts not to include the token that gets split. So we tell people to remove the trailing space ( ) from their reverse prompt, or in your case the equivalent suggestion would be to drop the right angle bracket (>) from your reverse prompt (it may take dropping both |>).

The decision to rigidly enforce the reverse prompt is from before I became a collaborator, so as much as I agree that reverse prompts should not automatically enable interactive mode, this isn't part of that. I'm going to have to wait for someone more senior than me to approve this particular part of the PR.

[data-angel]

You should probably also update common.cpp's gpt_params_parse to reflect the test for interactivity because antiprompts no longer trigger interactive mode. So something like this:

    if (params.prompt_cache_all && 
        (params.interactive || params.interactive_first || params.instruct)) {

Good catch - will update that.

[data-angel]

You should probably also update common.cpp's gpt_params_parse to reflect the test for interactivity because antiprompts no longer trigger interactive mode. So something like this:

    if (params.prompt_cache_all && 
        (params.interactive || params.interactive_first || params.instruct)) {

Good catch - will update that.

This is done.