Merge pull request #238 from symflower/followup-early

Early merger for "docker runtime" follow up

cmd/eval-dev-quality/cmd/evaluate.go

@@ -156,7 +156,7 @@ func (command *Evaluate) Initialize(args []string) (evaluationContext *evaluate.
 		}
 
 		if command.Parallel == 0 {
-			command.logger.Panic("the 'parallel' parameter has to be greater then 0")
+			command.logger.Panic("the 'parallel' parameter has to be greater then zero")
 		}
 
 		if command.RuntimeImage == "" {
@@ -446,13 +446,18 @@ func (command *Evaluate) evaluateLocal(evaluationContext *evaluate.Context) (err
 
 // evaluateDocker executes the evaluation for each model inside a docker container.
 func (command *Evaluate) evaluateDocker(ctx *evaluate.Context) (err error) {
-	// Filter all the args to pass them onto the container.
-	args := util.FilterArgs(os.Args[2:], []string{
-		"--model",
-		"--parallel",
-		"--result-path",
-		"--runtime",
-	})
+	availableFlags := util.Flags(command)
+	ignoredFlags := []string{
+		"model",
+		"parallel",
+		"result-path",
+		"runtime",
+	}
+
+	// Filter all the args to only contain flags which can be used.
+	args := util.FilterArgsKeep(os.Args[2:], availableFlags)
+	// Filter the args to remove all flags unsuited for running the container.
+	args = util.FilterArgsRemove(args, ignoredFlags)
 
 	parallel := util.NewParallel(command.Parallel)
 
@@ -521,13 +526,18 @@ func (command *Evaluate) evaluateKubernetes(ctx *evaluate.Context) (err error) {
 		return pkgerrors.Wrap(err, "could not create kubernetes job template")
 	}
 
-	// Filter all the args to pass them onto the container.
-	args := util.FilterArgs(os.Args[2:], []string{
-		"--model",
-		"--parallel",
-		"--result-path",
-		"--runtime",
-	})
+	availableFlags := util.Flags(command)
+	ignoredFlags := []string{
+		"model",
+		"parallel",
+		"result-path",
+		"runtime",
+	}
+
+	// Filter all the args to only contain flags which can be used.
+	args := util.FilterArgsKeep(os.Args[2:], availableFlags)
+	// Filter the args to remove all flags unsuited for running the container.
+	args = util.FilterArgsRemove(args, ignoredFlags)
 
 	parallel := util.NewParallel(command.Parallel)
 

cmd/eval-dev-quality/cmd/evaluate_test.go

@@ -836,6 +836,7 @@ func TestEvaluateInitialize(t *testing.T) {
 		ValidateCommand func(t *testing.T, command *Evaluate)
 		ValidateContext func(t *testing.T, context *evaluate.Context)
 		ValidateResults func(t *testing.T, resultsPath string)
+		ValidatePanic   string
 	}
 
 	validate := func(t *testing.T, tc *testCase) {
@@ -854,6 +855,17 @@ func TestEvaluateInitialize(t *testing.T) {
 			tc.Command.ResultPath = strings.ReplaceAll(tc.Command.ResultPath, "$TEMP_PATH", temporaryDirectory)
 
 			var actualEvaluationContext *evaluate.Context
+
+			if tc.ValidatePanic != "" {
+				assert.PanicsWithValue(t, tc.ValidatePanic, func() {
+					c, cleanup := tc.Command.Initialize([]string{})
+					cleanup()
+					actualEvaluationContext = c
+				})
+
+				return
+			}
+
 			assert.NotPanics(t, func() {
 				c, cleanup := tc.Command.Initialize([]string{})
 				cleanup()
@@ -1013,4 +1025,59 @@ func TestEvaluateInitialize(t *testing.T) {
 			}
 		},
 	})
+	validate(t, &testCase{
+		Name: "Local runtime does not allow parallel parameter",
+
+		Command: makeValidCommand(func(command *Evaluate) {
+			command.Runtime = "local"
+			command.Parallel = 2
+		}),
+
+		ValidatePanic: "the 'parallel' parameter can't be used with local execution",
+	})
+	validate(t, &testCase{
+		Name: "Attempts parameter hast to be greater then zero",
+
+		Command: makeValidCommand(func(command *Evaluate) {
+			command.QueryAttempts = 0
+		}),
+
+		ValidatePanic: "number of configured query attempts must be greater than zero",
+	})
+	validate(t, &testCase{
+		Name: "Execution timeout parameter hast to be greater then zero",
+
+		Command: makeValidCommand(func(command *Evaluate) {
+			command.ExecutionTimeout = 0
+		}),
+
+		ValidatePanic: "execution timeout for compilation and tests must be greater than zero",
+	})
+	validate(t, &testCase{
+		Name: "Runs parameter hast to be greater then zero",
+
+		Command: makeValidCommand(func(command *Evaluate) {
+			command.Runs = 0
+		}),
+
+		ValidatePanic: "number of configured runs must be greater than zero",
+	})
+
+	t.Run("Docker", func(t *testing.T) {
+		if osutil.IsDarwin() { // The MacOS runner on Github do not have "docker" in their path and would mess with the test.
+			t.Skip("Unsupported OS")
+		}
+
+		validate(t, &testCase{
+			Name: "Parallel parameter hast to be greater then zero",
+
+			Command: makeValidCommand(func(command *Evaluate) {
+				command.Runtime = "docker"
+				command.Parallel = 0
+			}),
+
+			ValidatePanic: "the 'parallel' parameter has to be greater then zero",
+		})
+	})
+
 }

util/exec.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"io"
 	"os/exec"
+	"reflect"
 	"strings"
 	"sync"
 	"time"
@@ -62,11 +63,37 @@ func CommandWithResult(ctx context.Context, logger *log.Logger, command *Command
 	return writer.String(), nil
 }
 
-// FilterArgs parses args and removes the ignored ones.
-func FilterArgs(args []string, ignored []string) (filtered []string) {
+// Flags returns a list of `long` flags bound on the command or nil.
+func Flags(cmd any) (args []string) {
+	typ := reflect.TypeOf(cmd)
+
+	// Dereference pointer
+	if typ.Kind() == reflect.Pointer {
+		typ = typ.Elem()
+	}
+
+	if typ.Kind() != reflect.Struct {
+		return nil
+	}
+
+	for i := 0; i < typ.NumField(); i++ {
+		field := typ.Field(i)
+		arg, ok := field.Tag.Lookup("long")
+		if !ok {
+			continue
+		}
+
+		args = append(args, arg)
+	}
+
+	return args
+}
+
+// FilterArgs filters the arguments by either ignoring/allowing them in the result.
+func FilterArgs(args []string, filter []string, ignore bool) (filtered []string) {
 	filterMap := map[string]bool{}
-	for _, v := range ignored {
-		filterMap[v] = true
+	for _, v := range filter {
+		filterMap["--"+v] = true
 	}
 
 	// Resolve args with equals sign.
@@ -79,13 +106,14 @@ func FilterArgs(args []string, ignored []string) (filtered []string) {
 		}
 	}
 
-	skip := false
+	skip := true
 	for _, v := range resolvedArgs {
-		if skip && strings.HasPrefix(v, "--") {
-			skip = false
-		}
-		if filterMap[v] {
-			skip = true
+		if strings.HasPrefix(v, "--") {
+			if ignore {
+				skip = filterMap[v]
+			} else {
+				skip = !filterMap[v]
+			}
 		}
 
 		if skip {
@@ -98,6 +126,16 @@ func FilterArgs(args []string, ignored []string) (filtered []string) {
 	return filtered
 }
 
+// FilterArgsKeep filters the given argument list and only returns arguments defined present in "filter".
+func FilterArgsKeep(args []string, filter []string) (filtered []string) {
+	return FilterArgs(args, filter, false)
+}
+
+// FilterArgsRemove filters the given argument list and returns arguments where "filter" entries are removed.
+func FilterArgsRemove(args []string, filter []string) (filtered []string) {
+	return FilterArgs(args, filter, true)
+}
+
 // Parallel holds a buffered channel for limiting parallel executions.
 type Parallel struct {
 	ch chan struct{}