Label eval runs with the model the server actually loaded (#379)
The phrasing eval printed "llm (0.8B, ...)" no matter which gguf llama-server had loaded, so two runs of two different models came out named the same and were easy to mix up when comparing. It now asks llama-server over /v1/models, same as the router eval already did. The helper moved to internal/llm so both share it, and it now errors instead of returning a blank name when the id field is missing — an unreachable server gets labelled "unknown-model", never a plausible-looking guess. Both eval paths stay opt-in behind MAVEN_LLM_URL; no server needed for go test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
This commit is contained in:
@@ -1,4 +1,4 @@
|
|||||||
package eval
|
package llm
|
||||||
|
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
@@ -6,8 +6,20 @@ import (
|
|||||||
"fmt"
|
"fmt"
|
||||||
"net/http"
|
"net/http"
|
||||||
"strings"
|
"strings"
|
||||||
|
"time"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
// UnknownModel is the label to print when the server would not say what it has
|
||||||
|
// loaded. Deliberately ugly: an honest "unknown" is fine, a plausible-looking
|
||||||
|
// but wrong model name is the bug this whole file exists to prevent.
|
||||||
|
const UnknownModel = "unknown-model"
|
||||||
|
|
||||||
|
// llama-server is local, so never send this through a proxy: this box's
|
||||||
|
// http_proxy answers 503 for loopback, which would look like "server won't say
|
||||||
|
// which model it has" when the server is right there and fine.
|
||||||
|
// A Transport with no Proxy set bypasses http_proxy entirely.
|
||||||
|
var modelHTTP = &http.Client{Timeout: 10 * time.Second, Transport: &http.Transport{}}
|
||||||
|
|
||||||
// ModelID asks llama-server which model it has loaded, so a scoring run can
|
// ModelID asks llama-server which model it has loaded, so a scoring run can
|
||||||
// label itself. Without this a bake-off between two models produces two tables
|
// label itself. Without this a bake-off between two models produces two tables
|
||||||
// that look identical, and the operator has to remember which server was up.
|
// that look identical, and the operator has to remember which server was up.
|
||||||
@@ -19,7 +31,7 @@ func ModelID(ctx context.Context, base string) (string, error) {
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return "", err
|
return "", err
|
||||||
}
|
}
|
||||||
resp, err := http.DefaultClient.Do(req)
|
resp, err := modelHTTP.Do(req)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return "", err
|
return "", err
|
||||||
}
|
}
|
||||||
@@ -38,14 +50,21 @@ func ModelID(ctx context.Context, base string) (string, error) {
|
|||||||
if len(out.Data) == 0 {
|
if len(out.Data) == 0 {
|
||||||
return "", fmt.Errorf("models: empty list")
|
return "", fmt.Errorf("models: empty list")
|
||||||
}
|
}
|
||||||
return shortModelID(out.Data[0].ID), nil
|
short := shortModelID(out.Data[0].ID)
|
||||||
|
if short == "" {
|
||||||
|
// Server answered but the id field was missing or blank. Say so
|
||||||
|
// instead of handing back an empty label that reads as a real name.
|
||||||
|
return "", fmt.Errorf("models: no id in response")
|
||||||
|
}
|
||||||
|
return short, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// shortModelID trims the path and the .gguf suffix — llama-server reports the
|
// shortModelID trims the path and the .gguf suffix — llama-server reports the
|
||||||
// file name it was started with, which is too long for a table header.
|
// file name it was started with, which is too long for a table header.
|
||||||
func shortModelID(id string) string {
|
func shortModelID(id string) string {
|
||||||
|
id = strings.TrimSpace(id)
|
||||||
if i := strings.LastIndexAny(id, "/\\"); i >= 0 {
|
if i := strings.LastIndexAny(id, "/\\"); i >= 0 {
|
||||||
id = id[i+1:]
|
id = id[i+1:]
|
||||||
}
|
}
|
||||||
return strings.TrimSuffix(id, ".gguf")
|
return strings.TrimSpace(strings.TrimSuffix(id, ".gguf"))
|
||||||
}
|
}
|
||||||
@@ -0,0 +1,64 @@
|
|||||||
|
package llm
|
||||||
|
|
||||||
|
import (
|
||||||
|
"context"
|
||||||
|
"net/http"
|
||||||
|
"net/http/httptest"
|
||||||
|
"testing"
|
||||||
|
)
|
||||||
|
|
||||||
|
// The point of these tests: a wrong-but-plausible model label is the bug, so
|
||||||
|
// every path that cannot learn the real name must return an error instead of a
|
||||||
|
// guess. No llama-server needed — a stub server stands in.
|
||||||
|
func TestModelID(t *testing.T) {
|
||||||
|
cases := []struct {
|
||||||
|
name string
|
||||||
|
body string
|
||||||
|
code int
|
||||||
|
want string // "" ⇒ expect an error
|
||||||
|
}{
|
||||||
|
{"full path", `{"data":[{"id":"/mnt/hdd1/llms/qwen3.5/Qwen3.5-0.8B.Q4_K_M.gguf"}]}`, 200, "Qwen3.5-0.8B.Q4_K_M"},
|
||||||
|
{"bare name", `{"data":[{"id":"LFM2.5-1.2B"}]}`, 200, "LFM2.5-1.2B"},
|
||||||
|
{"empty list", `{"data":[]}`, 200, ""},
|
||||||
|
{"id missing", `{"data":[{}]}`, 200, ""},
|
||||||
|
{"id blank", `{"data":[{"id":" "}]}`, 200, ""},
|
||||||
|
{"server error", `nope`, 500, ""},
|
||||||
|
{"not json", `<html>`, 200, ""},
|
||||||
|
}
|
||||||
|
for _, c := range cases {
|
||||||
|
t.Run(c.name, func(t *testing.T) {
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
if r.URL.Path != "/v1/models" {
|
||||||
|
t.Errorf("asked for %s, want /v1/models", r.URL.Path)
|
||||||
|
}
|
||||||
|
w.WriteHeader(c.code)
|
||||||
|
_, _ = w.Write([]byte(c.body))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
got, err := ModelID(context.Background(), srv.URL+"/")
|
||||||
|
if c.want == "" {
|
||||||
|
if err == nil {
|
||||||
|
t.Fatalf("want an error, got label %q", got)
|
||||||
|
}
|
||||||
|
return
|
||||||
|
}
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("ModelID: %v", err)
|
||||||
|
}
|
||||||
|
if got != c.want {
|
||||||
|
t.Errorf("got %q, want %q", got, c.want)
|
||||||
|
}
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestModelIDUnreachable(t *testing.T) {
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}))
|
||||||
|
url := srv.URL
|
||||||
|
srv.Close() // nothing listening now
|
||||||
|
|
||||||
|
if got, err := ModelID(context.Background(), url); err == nil {
|
||||||
|
t.Fatalf("want an error from a dead server, got label %q", got)
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -7,6 +7,7 @@ import (
|
|||||||
"testing"
|
"testing"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
|
"github.com/kami/maven/internal/llm"
|
||||||
"github.com/kami/maven/internal/phraser"
|
"github.com/kami/maven/internal/phraser"
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -32,6 +33,7 @@ func TestLLMPhrasingBaseline(t *testing.T) {
|
|||||||
// case as a phrasing error and read as "the model cannot phrase".
|
// case as a phrasing error and read as "the model cannot phrase".
|
||||||
noProxyLoopback(t)
|
noProxyLoopback(t)
|
||||||
|
|
||||||
|
ctx := context.Background()
|
||||||
f, err := Load()
|
f, err := Load()
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("Load: %v", err)
|
t.Fatalf("Load: %v", err)
|
||||||
@@ -44,7 +46,19 @@ func TestLLMPhrasingBaseline(t *testing.T) {
|
|||||||
p := phraser.NewLLMPhraserAt(base, cfg)
|
p := phraser.NewLLMPhraserAt(base, cfg)
|
||||||
defer p.Close()
|
defer p.Close()
|
||||||
|
|
||||||
rep, err := Score(context.Background(), "llm (0.8B, built-in persona)", p, f)
|
// Label the run with whatever gguf the server actually has loaded. It used
|
||||||
|
// to say "0.8B" no matter what, so two runs of two different models came
|
||||||
|
// out named the same and were easy to mix up when comparing.
|
||||||
|
model, err := llm.ModelID(ctx, base)
|
||||||
|
if err != nil {
|
||||||
|
// An unlabelled score is still a score, but say so loudly — a made-up
|
||||||
|
// name in a bake-off table is worse than no name.
|
||||||
|
t.Logf("could not read model id from %s: %v — report will say %q", base, err, llm.UnknownModel)
|
||||||
|
model = llm.UnknownModel
|
||||||
|
}
|
||||||
|
t.Logf("scoring model %s at %s", model, base)
|
||||||
|
|
||||||
|
rep, err := Score(ctx, "llm ("+model+", built-in persona)", p, f)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("Score: %v", err)
|
t.Fatalf("Score: %v", err)
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -61,12 +61,12 @@ func TestLLMRouterBaseline(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
ctx := context.Background()
|
ctx := context.Background()
|
||||||
model, err := ModelID(ctx, base)
|
model, err := llm.ModelID(ctx, base)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
// Not fatal: an unlabelled score is still a score. But say so loudly,
|
// Not fatal: an unlabelled score is still a score. But say so loudly,
|
||||||
// because an unlabelled row in a bake-off table is worthless.
|
// because an unlabelled row in a bake-off table is worthless.
|
||||||
t.Logf("could not read model id from %s: %v — reports will say %q", base, err, "unknown-model")
|
t.Logf("could not read model id from %s: %v — reports will say %q", base, err, llm.UnknownModel)
|
||||||
model = "unknown-model"
|
model = llm.UnknownModel
|
||||||
}
|
}
|
||||||
t.Logf("scoring model %s at %s", model, base)
|
t.Logf("scoring model %s at %s", model, base)
|
||||||
lr := router.NewLLMRouter(client)
|
lr := router.NewLLMRouter(client)
|
||||||
|
|||||||
Reference in New Issue
Block a user