Enable sigint handler even when not in interactive mode

[jaggzh]

Handling two issues with the signal handling:

1. When used as an entrypoint in a docker container main, for instance, would not catch ^C and could not be terminated (without a kill -9).
2. Additionally, the current signal handling code is somewhat inconsistent, defining a signal handler but it not being set as the handler unless -i was requested. This results in the console cleanup not being called (and timings not being output).

This code:

1. Adds --sigint so CTRL-C will do an immediate cleanup+timings+exit in non-interactive mode.

Alternatives:

1. Enable the sigint handler for non-interactive mode; that is, enable the double CTRL-C press to abort. (Note: The prior code seems like it was potentially intended to do this, but it didn't assign (sigaction()) unless -i was specified so it didn't quite get there! (Hence my pointing out the [potential] inconsistency).
2. ... ideas are welcome ...

[cebtenzzre]

The default action of SIGINT is to terminate the process. That already works for me. llama.cpp does not use SIG_IGN, so why does SIGINT not work for you?

[slaren]

It would be nice if CTRL-C printed the timings before closing, as in interactive mode, though. But I don't think this needs a command line option, it should be the normal behavior.

[jaggzh]

The default action of SIGINT is to terminate the process. That already works for me. llama.cpp does not use SIG_IGN, so why does SIGINT not work for you?

As entrypoints, processes become pid 1 (init). The init process does not inherit signals, and only signals it has set up itself can be sent to it (this is by POSIX standard). With the existing code, we do not assign the signal handler unless interactive mode is set. I am guessing you are either:

1. Not invoking main as a docker entrypoint (where it becomes the init process). (It's possible other code sets up signal handlers different from how the main example does).
2. You are using -i
3. ...Maybe you're not using a posix-compliant system? :)

[jaggzh]

It would be nice if CTRL-C printed the timings before closing, as in interactive mode, though. But I don't think this needs a command line option, it should be the normal behavior.

That's how I designed this. Interactive mode did already put out the timings, but with non-interactive mode we weren't setting up a signal handler, so you're seeing the default signal handler kick in and terminate the program. Thus, this change (PR) assigns our signal handler for non-interactive mode as well, if the user specifies --sigint (although see my code about inconsistencies, since it appears it was originally intended to do just this, but would require a ^C to enter interactive mode, then a second ^C to terminate.)

[ggerganov]

I would also prefer the default behaviour of Ctrl-C in non-interactive mode to be to print the timings before terminating. If I'm reading the code correctly, this is almost the case, except we have to add --sigint to the CLI, while I would prefer to not have to do it by default. I could be misreading the logic though, so if it already works like that, then all is good.

[ggerganov]

The old version is the correct one - probably this is an artifact from resolving conflicts

[jaggzh]

You're correct, I kept the prior default behavior of sigint being not-handled in case that was desired behavior. --sigint enables it. I think it's fine to just remove --sigint entirely, while still adding the signal handler for SIGINT though, as I can't think of many use-cases where the user truly wants SIGINT to be ignored even if they run main as init (ie. as the entrypoint). (Optionally we could make --no-sigint)

…

On Thu, Nov 2, 2023, 11:14 PM Georgi Gerganov ***@***.***> wrote: ***@***.**** approved this pull request. I would also prefer the default behaviour of Ctrl-C in non-interactive mode to be to print the timings before terminating. If I'm reading the code correctly, this is almost the case, except we have to add --sigint to the CLI, while I would prefer to not have to do it by default. I could be misreading the logic though, so if it already works like that, then all is good. ------------------------------ In examples/main/main.cpp <#3873 (comment)>: > @@ -298,7 +300,7 @@ int main(int argc, char ** argv) { } // remove any "future" tokens that we might have inherited from the previous session - llama_kv_cache_seq_rm(ctx, -1, n_matching_session_tokens, -1); + llama_kv_cache_tokens_rm(ctx, n_matching_session_tokens, -1); The old version is the correct one - probably this is an artifact from resolving conflicts — Reply to this email directly, view it on GitHub <#3873 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AE3AV7JCMGDWIR532R5PMJ3YCSDVXAVCNFSM6AAAAAA6YGVEHWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMJRHA3TKMRQGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[jaggzh]

It looks like the merge got confused by an adjacent new option. Correct me if I'm wrong, but I merely kept both the "new" --sigint (it was quite a while ago that I added it), and the new --chatml, like this:

printf(" --sigint allow CTRL-C (sigint) to kill even when not in -i mode (only added to main for now)\n");
printf(" -cml, --chatml run in chatml mode (use with ChatML-compatible models)\n");

[ggerganov]

I removed the --sigint arg and made it to print the timings upon ctrl+C when interactive mode is not enabled