server: stop generation at n_ctx_train if n_predict is not set

[phymbert]

Context

If the model hallucinates (EOS-less generation), server will go to infinite loop if n_predict is not set.

It is a wrong usage of the server or the model:

• https://github.com/ggerganov/llama.cpp/blob/master/examples/server/tests/features/wrong_usages.feature
• Infinite loop of "context shift" #3969
• Please upgrade the KV cache size yes using --ctx-size #6617 (comment)

But as it brings confusion, I propose to stop the generation at the size of the context which with the model was trained if self-extent context is disabled.

Tests

server --hf-repo ggml-org/models --hf-file phi-2/ggml-model-q4_0.gguf --model phi-2-q4_0.gguf --parallel 1 --ctx-size 4096 --log-format text -ngl 33

curl http://localhost:8080/completion --data '{"prompt": "hallucinate"}'

WARN [ update_slots] n_predict is not set and self-context extend is disabled. Limiting generated tokens to n_ctx_train to avoid EOS-less generation infinite loop | tid="127864424759296" timestamp=1713526370 params.n_predict=-1 slot.n_predict=-1 slot.n_decoded=2048 n_slots=1 n_ctx=4096 n_ctx_train=2048 ga_n=1

[ggerganov]

Before we make the change, we should see if the update_slots error is really reproducible. If it is, it's a bug that we first need to fix. If we merge the change now, we might hide the underlying issue

[FSSRepo]

@ggerganov I have created a web application to stress-test the server and see how it handles multiple clients sending random questions and documents simultaneously. I tested it with four clients using mixtral 8x7B q8_0 in x3 RTX 3090 for one hour, and the server didn't encounter any issues.

master:

But the gg/flash-attn branch sometimes generates NaNs in the query tensor, which are not generated by the flash attention kernel (this happens before the flash attention kernel).

I will keep investigating for the meantime.

[phymbert]

Agreed, I am doing performance and capacity tests since 2 month+, there is no such bug. The server is stable and production ready.

[ochafik]

Alongside an optional cap, I think we should make the server stop generating when the connection is closed for whatever reason (clients may well have a timeout or interrupt things manually, but the server keeps going / stays busy needlessly). Maybe an interrupt check callback to call before generating each token?

[phymbert]

Yeah, it is identified in:

• Feature Request: Task Cancellation on Client Disconnection #6421

[phymbert]

Before we make the change, we should see if the update_slots error is really reproducible.

We can conclude that the user was using an old version. That's it.

[phymbert]

@ggerganov, finally, I would prefer not to go this way but to stop the generation at n_ctx with a warning, instead of printing a warning each time if n_predict is not set.

[ggerganov]

Ok. I'm not sure we have to da anything at this point - seems the latest version work OK

[slaren]

There should still be some limit to avoid getting into an infinite loop in the server.

[phymbert]

@ggerganov @slaren please have a look to this proposal

[slaren]

When this happens, the response of /completion has these fields:

  "truncated": true,
  "stopped_eos": false,
  "stopped_word": false,
  "stopped_limit": false,

I am not familiar with the meaning of each of these flags. Should this be different? Maybe stopped_limit should be true to indicate that a limit was hit?

[slaren]

Maybe it would be simpler to set n_predict to n_ctx_train by default if not set in the request.

[phymbert]

Maybe it would be simpler to set n_predict to n_ctx_train by default if not set in the request.

Yeah, it was the first version, but I feel it noisy to log this warning at each request: 6fd5ad5

[slaren]

That's not exactly what I mean. Basically I would just change the default to n_ctx_train (or other value) in the line that sets slot.params.n_predict = json_value(data, "n_predict", default_params.n_predict);. No need to print any warnings, just document it. The user can check stopped_limit or set a different limit if needed.

[phymbert]

I see, I am OK with both solutions even if it will be sort of a breaking change to set n_predict all the time.

AFAIK not all models hallucinate and not on all completion, plus normally it should always emmit EOS token if the trained chat template is in used in chat completion endpoint.

@ggerganov up to you, but we need to stop this infinite loop recurrent concern some way.

[slaren]

This would be simple if context shifting was opt-in, then there would always be a hard limit of n_ctx tokens. I am not sure that enabling context shift by default and without a way to disable it is a good idea, it can result in very poor generation quality and it is not the normal behavior for LLM providers, as far as I know.

[phymbert]

This would be simple if context shifting was opt-in, then there would always be a hard limit of n_ctx tokens. I am not sure that enabling context shift by default and without a way to disable it is a good idea, it can result in very poor generation quality and it is not the normal behavior for LLM providers, as far as I know.

Oh yes, and it is so slow in the current implementation, blocking the whole server.

[phymbert]

@ggerganov up to you, but we need to stop this infinite loop recurrent concern some way.

@ggerganov I think with the removal of hard coded stop tokens, this PR is becoming more important

[ggerganov]

Maybe it would be simpler to set n_predict to n_ctx_train by default if not set in the request.

Yes, let's do that. Context-shift has to be refactored and become optional (in a future PR)

[phymbert]

Maybe it would be simpler to set n_predict to n_ctx_train by default if not set in the request.

Yes, let's do that. Context-shift has to be refactored and become optional (in a future PR)

@ggerganov @slaren Finally I prefer to keep checking at each token if we do not exceed n_ctx_train because one can simply pass n_predict = -1 in the request payload and the server will still go to infinite loop. I feel this approach safer with a proper warning.