diff --git a/internal/router/eval/modelid.go b/internal/llm/modelid.go similarity index 55% rename from internal/router/eval/modelid.go rename to internal/llm/modelid.go index 5a9102d..36b90da 100644 --- a/internal/router/eval/modelid.go +++ b/internal/llm/modelid.go @@ -1,4 +1,4 @@ -package eval +package llm import ( "context" @@ -6,8 +6,20 @@ import ( "fmt" "net/http" "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 // 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. @@ -19,7 +31,7 @@ func ModelID(ctx context.Context, base string) (string, error) { if err != nil { return "", err } - resp, err := http.DefaultClient.Do(req) + resp, err := modelHTTP.Do(req) if err != nil { return "", err } @@ -38,14 +50,21 @@ func ModelID(ctx context.Context, base string) (string, error) { if len(out.Data) == 0 { 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 // file name it was started with, which is too long for a table header. func shortModelID(id string) string { + id = strings.TrimSpace(id) if i := strings.LastIndexAny(id, "/\\"); i >= 0 { id = id[i+1:] } - return strings.TrimSuffix(id, ".gguf") + return strings.TrimSpace(strings.TrimSuffix(id, ".gguf")) } diff --git a/internal/llm/modelid_test.go b/internal/llm/modelid_test.go new file mode 100644 index 0000000..c5ce1df --- /dev/null +++ b/internal/llm/modelid_test.go @@ -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", ``, 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) + } +} diff --git a/internal/phraser/eval/llmphraser_test.go b/internal/phraser/eval/llmphraser_test.go index 12b108a..316eaa2 100644 --- a/internal/phraser/eval/llmphraser_test.go +++ b/internal/phraser/eval/llmphraser_test.go @@ -7,6 +7,7 @@ import ( "testing" "time" + "github.com/kami/maven/internal/llm" "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". noProxyLoopback(t) + ctx := context.Background() f, err := Load() if err != nil { t.Fatalf("Load: %v", err) @@ -44,7 +46,19 @@ func TestLLMPhrasingBaseline(t *testing.T) { p := phraser.NewLLMPhraserAt(base, cfg) 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 { t.Fatalf("Score: %v", err) } diff --git a/internal/router/eval/llmrouter_test.go b/internal/router/eval/llmrouter_test.go index 940eea9..12a8e76 100644 --- a/internal/router/eval/llmrouter_test.go +++ b/internal/router/eval/llmrouter_test.go @@ -61,12 +61,12 @@ func TestLLMRouterBaseline(t *testing.T) { } ctx := context.Background() - model, err := ModelID(ctx, base) + model, err := llm.ModelID(ctx, base) if err != nil { // Not fatal: an unlabelled score is still a score. But say so loudly, // 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") - model = "unknown-model" + t.Logf("could not read model id from %s: %v — reports will say %q", base, err, llm.UnknownModel) + model = llm.UnknownModel } t.Logf("scoring model %s at %s", model, base) lr := router.NewLLMRouter(client) diff --git a/kill-maven.sh b/kill-maven.sh index f0754bd..df3ec80 100755 --- a/kill-maven.sh +++ b/kill-maven.sh @@ -1,8 +1,10 @@ #!/usr/bin/env bash # Unified script to stop all Maven services. # Usage: ./kill-maven.sh -# - Graceful SIGTERM is attempted first. -# - If any process lingers, force with SIGKILL. +# - Docker deploy: `docker compose stop` (see why below). +# - Bare-metal / dev run: graceful SIGTERM first, SIGKILL if anything lingers. +# Exits non-zero if it cannot confirm everything is stopped. It must never say +# "stopped" unless it checked. set -euo pipefail @@ -24,6 +26,73 @@ else LLM='llama-server.*\.gguf' fi +COMPOSE_FILE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/docker-compose.yml" + +# --- containerised deploy ------------------------------------------------ +# docker-compose.yml does not set `pid: host`, so each container has its own +# PID namespace: pkill on the host sees nothing inside them. This script used +# to print "all stopped" while every daemon was still happily running. Stop the +# containers through compose instead — that actually reaches them. +# +# running_containers prints the ids of the project's running containers, or +# nothing. Empty output plus a non-zero return means "could not ask docker", +# which is different from "nothing is running" and is handled below. +running_containers() { + docker compose -f "$COMPOSE_FILE" ps -q --status running 2>/dev/null +} + +DOCKER_OK=0 +CONTAINERS="" +if command -v docker >/dev/null 2>&1 && [ -f "$COMPOSE_FILE" ]; then + if CONTAINERS="$(running_containers)"; then + DOCKER_OK=1 + fi +fi + +if [ "$DOCKER_OK" = 1 ] && [ -n "$CONTAINERS" ]; then + echo "--- Maven is running in containers: stopping via docker compose ---" + if ! docker compose -f "$COMPOSE_FILE" stop; then + echo "ERROR: 'docker compose stop' failed. Containers may still be running." >&2 + exit 1 + fi + echo "--- Verifying containers are gone ---" + LEFT="$(running_containers || true)" + if [ -n "$LEFT" ]; then + echo "ERROR: containers still running after stop:" >&2 + docker compose -f "$COMPOSE_FILE" ps >&2 || true + exit 1 + fi + echo "All containers stopped." + exit 0 +fi + +# --- bare-metal / dev run ----------------------------------------------- +# pgrep -f matches whole command lines, so a shell that merely mentions +# "mavend" (this script's own parent, for one) shows up. Drop ourselves and our +# parent, otherwise the SIGKILL sweep can take out the terminal you ran this in. +host_pids() { + pgrep -f "$PAT|$LLM" | grep -v -e "^$$\$" -e "^$PPID\$" | paste -sd, - || true +} +HOST_PIDS=$(host_pids) + +if [ -z "$HOST_PIDS" ]; then + # Nothing on the host. Whether that means "already down" depends on whether + # we managed to ask docker, and the two must not read the same. + if [ "$DOCKER_OK" = 1 ]; then + # Docker answered and named no running containers, and there is nothing + # on the host either. That is a real answer: Maven is already stopped. + echo "Nothing to stop: no Maven processes and no running containers." + exit 0 + fi + # We could not ask docker, so Maven may be alive in a container we cannot + # see. Saying "stopped" here is the exact false success this script had. + echo "ERROR: no Maven processes on this host, and docker could not be asked." >&2 + echo " If this is the container deploy it may still be running:" >&2 + echo " docker compose -f $COMPOSE_FILE stop" >&2 + echo " Nothing was stopped. Check by hand before assuming Maven is down." >&2 + exit 1 +fi + echo "--- Sending graceful SIGTERM to Maven services ---" pkill -TERM -f "$PAT" || true # mavend's Pdeathsig SIGKILLs its llama-server on exit, but sweep strays too @@ -32,12 +101,18 @@ pkill -TERM -f "$LLM" || true echo "--- Verifying processes are gone ---" sleep 1 -PIDS=$(pgrep -d ',' -f "$PAT|$LLM") || PIDS="" +PIDS=$(host_pids) if [ -n "$PIDS" ]; then echo "Warning: some processes still alive. PIDs: $PIDS" echo "--- Force killing with SIGKILL ---" echo "$PIDS" | tr ',' '\n' | xargs -r kill -9 + sleep 1 + LEFT=$(host_pids) + if [ -n "$LEFT" ]; then + echo "ERROR: still alive after SIGKILL. PIDs: $LEFT" >&2 + exit 1 + fi echo "Done (SIGKILL)." else echo "All services gracefully stopped." -fi \ No newline at end of file +fi