From d3fb51ee8256610055aa4bd3d3838f490aacf51d Mon Sep 17 00:00:00 2001 From: BeaconCat Date: Sun, 12 Apr 2026 16:20:16 +0800 Subject: [PATCH] fix: address Copilot review round 2 - Validate --eval-mode accepts only 'token' or 'llm' - Normalize base URL to avoid /v1/v1 duplication - Separate token/LLM results for correct PrintComparison labeling - Log ExpandMessages errors instead of silently ignoring - Short-circuit with 0 scores when no context retrieved (match token eval) - Add --timeout flag wired to LLMClientOptions.Timeout --- cmd/membench/eval_llm.go | 22 +++++++++++++++++----- cmd/membench/llm_client.go | 6 +++++- cmd/membench/main.go | 29 ++++++++++++++++++++++------- 3 files changed, 44 insertions(+), 13 deletions(-) diff --git a/cmd/membench/eval_llm.go b/cmd/membench/eval_llm.go index 46b996cc9..7092c2bef 100644 --- a/cmd/membench/eval_llm.go +++ b/cmd/membench/eval_llm.go @@ -192,19 +192,31 @@ func EvalSeahorseLLM( var contentParts []string if len(messageIDs) > 0 { expandResult, err := retrieval.ExpandMessages(ctx, messageIDs) - if err == nil { + if err != nil { + log.Printf("WARN: expand failed for sample %s: %v", sample.SampleID, err) + } else { for _, msg := range expandResult.Messages { contentParts = append(contentParts, msg.Content) } } } - contextText := "" - if len(contentParts) > 0 { - truncated, _ := BudgetTruncate(contentParts, budgetTokens) - contextText = StringListToContent(truncated) + if len(contentParts) == 0 { + qaResults = append(qaResults, QAResult{ + Question: qa.Question, + Category: qa.Category, + GoldAnswer: qa.AnswerString(), + TokenF1: 0.0, + HitRate: 0.0, + }) + log.Printf("[seahorse-llm] sample=%s q=%d/%d score=0.00 answer=(no context)", + sample.SampleID, total, totalQA) + continue } + truncated, _ := BudgetTruncate(contentParts, budgetTokens) + contextText := StringListToContent(truncated) + // Generate answer with LLM llmAnswer := "" score := 0.0 diff --git a/cmd/membench/llm_client.go b/cmd/membench/llm_client.go index cb7b1ee2a..bb0ff54c4 100644 --- a/cmd/membench/llm_client.go +++ b/cmd/membench/llm_client.go @@ -91,7 +91,11 @@ func (c *LLMClient) Complete(ctx context.Context, systemPrompt, userPrompt strin return "", fmt.Errorf("marshal request: %w", err) } - req, err := http.NewRequestWithContext(ctx, "POST", c.BaseURL+"/v1/chat/completions", bytes.NewReader(jsonBody)) + endpoint := c.BaseURL + "/v1/chat/completions" + if strings.HasSuffix(c.BaseURL, "/v1") || strings.HasSuffix(c.BaseURL, "/v1/") { + endpoint = strings.TrimRight(c.BaseURL, "/") + "/chat/completions" + } + req, err := http.NewRequestWithContext(ctx, "POST", endpoint, bytes.NewReader(jsonBody)) if err != nil { return "", fmt.Errorf("create request: %w", err) } diff --git a/cmd/membench/main.go b/cmd/membench/main.go index f75d4b1b4..33d52ee0c 100644 --- a/cmd/membench/main.go +++ b/cmd/membench/main.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "strings" + "time" "github.com/spf13/cobra" @@ -25,6 +26,7 @@ var ( flagModel string flagNoThinking bool flagLimit int + flagTimeout int ) func main() { @@ -62,6 +64,7 @@ func main() { evalCmd.Flags(). BoolVar(&flagNoThinking, "no-thinking", false, "disable thinking mode via chat_template_kwargs (llama.cpp + Qwen)") evalCmd.Flags().IntVar(&flagLimit, "limit", 0, "max QA questions per sample (0 = all)") + evalCmd.Flags().IntVar(&flagTimeout, "timeout", 120, "HTTP timeout in seconds for LLM requests") reportCmd := &cobra.Command{ Use: "report", @@ -87,6 +90,7 @@ func main() { runCmd.Flags(). BoolVar(&flagNoThinking, "no-thinking", false, "disable thinking mode via chat_template_kwargs (llama.cpp + Qwen)") runCmd.Flags().IntVar(&flagLimit, "limit", 0, "max QA questions per sample (0 = all)") + runCmd.Flags().IntVar(&flagTimeout, "timeout", 120, "HTTP timeout in seconds for LLM requests") rootCmd.AddCommand(ingestCmd, evalCmd, reportCmd, runCmd) @@ -167,7 +171,16 @@ func runEval(cmd *cobra.Command, args []string) error { log.Printf("Limited to %d QA per sample", flagLimit) } - useLLM := strings.ToLower(flagEvalMode) == "llm" + evalMode := strings.ToLower(strings.TrimSpace(flagEvalMode)) + var useLLM bool + switch evalMode { + case "token": + useLLM = false + case "llm": + useLLM = true + default: + return fmt.Errorf("invalid --eval-mode %q: must be token or llm", flagEvalMode) + } var llmClient *LLMClient if useLLM { opts, err := buildLLMOptions() @@ -178,7 +191,7 @@ func runEval(cmd *cobra.Command, args []string) error { log.Printf("LLM eval mode: model=%s base=%s no-thinking=%v", opts.Model, opts.BaseURL, opts.NoThinking) } - var allResults []EvalResult + var tokenResults, llmResults []EvalResult for _, mode := range modes { switch mode { @@ -189,11 +202,11 @@ func runEval(cmd *cobra.Command, args []string) error { } if useLLM { results := EvalLegacyLLM(ctx, samples, legacy, flagBudget, llmClient) - allResults = append(allResults, results...) + llmResults = append(llmResults, results...) log.Printf("legacy-llm: evaluated %d samples", len(results)) } else { results := EvalLegacy(ctx, samples, legacy, flagBudget) - allResults = append(allResults, results...) + tokenResults = append(tokenResults, results...) log.Printf("legacy: evaluated %d samples", len(results)) } case "seahorse": @@ -204,16 +217,17 @@ func runEval(cmd *cobra.Command, args []string) error { } if useLLM { results := EvalSeahorseLLM(ctx, samples, ir, flagBudget, llmClient) - allResults = append(allResults, results...) + llmResults = append(llmResults, results...) log.Printf("seahorse-llm: evaluated %d samples", len(results)) } else { results := EvalSeahorse(ctx, samples, ir, flagBudget) - allResults = append(allResults, results...) + tokenResults = append(tokenResults, results...) log.Printf("seahorse: evaluated %d samples", len(results)) } } } + allResults := append(tokenResults, llmResults...) if err := SaveResults(allResults, flagOut); err != nil { return fmt.Errorf("save results: %w", err) } @@ -221,7 +235,7 @@ func runEval(cmd *cobra.Command, args []string) error { return fmt.Errorf("save aggregated: %w", err) } - PrintComparison(allResults, nil) + PrintComparison(tokenResults, llmResults) return nil } @@ -296,5 +310,6 @@ func buildLLMOptions() (LLMClientOptions, error) { Model: model, APIKey: apiKey, NoThinking: flagNoThinking, + Timeout: time.Duration(flagTimeout) * time.Second, }, nil }