llm: do not silently fail for supplied, but invalid formats (#8130)
Changes in #8002 introduced fixes for bugs with mangling JSON Schemas.
It also fixed a bug where the server would silently fail when clients
requested invalid formats. It also, unfortunately, introduced a bug
where the server would reject requests with an empty format, which
should be allowed.
The change in #8127 updated the code to allow the empty format, but also
reintroduced the regression where the server would silently fail when
the format was set, but invalid.
This commit fixes both regressions. The server does not reject the empty
format, but it does reject invalid formats. It also adds tests to help
us catch regressions in the future.
Also, the updated code provides a more detailed error message when a
client sends a non-empty, but invalid format, echoing the invalid format
in the response.
This commits also takes the opportunity to remove superfluous linter
checks.
2024-12-16 21:57:49 -08:00
|
|
|
package llm
|
|
|
|
|
|
|
|
import (
|
|
|
|
"context"
|
|
|
|
"errors"
|
|
|
|
"fmt"
|
|
|
|
"strings"
|
|
|
|
"testing"
|
|
|
|
|
|
|
|
"github.com/ollama/ollama/api"
|
|
|
|
"golang.org/x/sync/semaphore"
|
|
|
|
)
|
|
|
|
|
|
|
|
func TestLLMServerCompletionFormat(t *testing.T) {
|
|
|
|
// This test was written to fix an already deployed issue. It is a bit
|
|
|
|
// of a mess, and but it's good enough, until we can refactoring the
|
|
|
|
// Completion method to be more testable.
|
|
|
|
|
|
|
|
ctx, cancel := context.WithCancel(context.Background())
|
|
|
|
s := &llmServer{
|
|
|
|
sem: semaphore.NewWeighted(1), // required to prevent nil panic
|
|
|
|
}
|
|
|
|
|
|
|
|
checkInvalid := func(format string) {
|
|
|
|
t.Helper()
|
|
|
|
err := s.Completion(ctx, CompletionRequest{
|
|
|
|
Options: new(api.Options),
|
|
|
|
Format: []byte(format),
|
|
|
|
}, nil)
|
|
|
|
|
|
|
|
want := fmt.Sprintf("invalid format: %q; expected \"json\" or a valid JSON Schema", format)
|
|
|
|
if err == nil || !strings.Contains(err.Error(), want) {
|
|
|
|
t.Fatalf("err = %v; want %q", err, want)
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
checkInvalid("X") // invalid format
|
|
|
|
checkInvalid(`"X"`) // invalid JSON Schema
|
|
|
|
|
|
|
|
cancel() // prevent further processing if request makes it past the format check
|
|
|
|
|
2024-12-17 09:49:37 -08:00
|
|
|
checkValid := func(err error) {
|
llm: do not silently fail for supplied, but invalid formats (#8130)
Changes in #8002 introduced fixes for bugs with mangling JSON Schemas.
It also fixed a bug where the server would silently fail when clients
requested invalid formats. It also, unfortunately, introduced a bug
where the server would reject requests with an empty format, which
should be allowed.
The change in #8127 updated the code to allow the empty format, but also
reintroduced the regression where the server would silently fail when
the format was set, but invalid.
This commit fixes both regressions. The server does not reject the empty
format, but it does reject invalid formats. It also adds tests to help
us catch regressions in the future.
Also, the updated code provides a more detailed error message when a
client sends a non-empty, but invalid format, echoing the invalid format
in the response.
This commits also takes the opportunity to remove superfluous linter
checks.
2024-12-16 21:57:49 -08:00
|
|
|
t.Helper()
|
|
|
|
if !errors.Is(err, context.Canceled) {
|
|
|
|
t.Fatalf("Completion: err = %v; expected context.Canceled", err)
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
2024-12-17 09:49:37 -08:00
|
|
|
valids := []string{
|
|
|
|
// "missing"
|
|
|
|
``,
|
|
|
|
`""`,
|
|
|
|
`null`,
|
|
|
|
|
|
|
|
// JSON
|
|
|
|
`"json"`,
|
|
|
|
`{"type":"object"}`,
|
|
|
|
}
|
llm: do not silently fail for supplied, but invalid formats (#8130)
Changes in #8002 introduced fixes for bugs with mangling JSON Schemas.
It also fixed a bug where the server would silently fail when clients
requested invalid formats. It also, unfortunately, introduced a bug
where the server would reject requests with an empty format, which
should be allowed.
The change in #8127 updated the code to allow the empty format, but also
reintroduced the regression where the server would silently fail when
the format was set, but invalid.
This commit fixes both regressions. The server does not reject the empty
format, but it does reject invalid formats. It also adds tests to help
us catch regressions in the future.
Also, the updated code provides a more detailed error message when a
client sends a non-empty, but invalid format, echoing the invalid format
in the response.
This commits also takes the opportunity to remove superfluous linter
checks.
2024-12-16 21:57:49 -08:00
|
|
|
for _, valid := range valids {
|
|
|
|
err := s.Completion(ctx, CompletionRequest{
|
|
|
|
Options: new(api.Options),
|
|
|
|
Format: []byte(valid),
|
|
|
|
}, nil)
|
2024-12-17 09:49:37 -08:00
|
|
|
checkValid(err)
|
llm: do not silently fail for supplied, but invalid formats (#8130)
Changes in #8002 introduced fixes for bugs with mangling JSON Schemas.
It also fixed a bug where the server would silently fail when clients
requested invalid formats. It also, unfortunately, introduced a bug
where the server would reject requests with an empty format, which
should be allowed.
The change in #8127 updated the code to allow the empty format, but also
reintroduced the regression where the server would silently fail when
the format was set, but invalid.
This commit fixes both regressions. The server does not reject the empty
format, but it does reject invalid formats. It also adds tests to help
us catch regressions in the future.
Also, the updated code provides a more detailed error message when a
client sends a non-empty, but invalid format, echoing the invalid format
in the response.
This commits also takes the opportunity to remove superfluous linter
checks.
2024-12-16 21:57:49 -08:00
|
|
|
}
|
|
|
|
|
|
|
|
err := s.Completion(ctx, CompletionRequest{
|
|
|
|
Options: new(api.Options),
|
|
|
|
Format: nil, // missing format
|
|
|
|
}, nil)
|
2024-12-17 09:49:37 -08:00
|
|
|
checkValid(err)
|
llm: do not silently fail for supplied, but invalid formats (#8130)
Changes in #8002 introduced fixes for bugs with mangling JSON Schemas.
It also fixed a bug where the server would silently fail when clients
requested invalid formats. It also, unfortunately, introduced a bug
where the server would reject requests with an empty format, which
should be allowed.
The change in #8127 updated the code to allow the empty format, but also
reintroduced the regression where the server would silently fail when
the format was set, but invalid.
This commit fixes both regressions. The server does not reject the empty
format, but it does reject invalid formats. It also adds tests to help
us catch regressions in the future.
Also, the updated code provides a more detailed error message when a
client sends a non-empty, but invalid format, echoing the invalid format
in the response.
This commits also takes the opportunity to remove superfluous linter
checks.
2024-12-16 21:57:49 -08:00
|
|
|
}
|