From 57044df382f6cae5f0d0bef583bffd8b98a610c5 Mon Sep 17 00:00:00 2001 From: aceforeverd Date: Tue, 10 Nov 2026 13:27:00 +0800 Subject: [PATCH 1/2] fix: rm the dummpy bad case guard for catalog inputs --- docs/openapi.yaml | 4 +-- internal/catalog/inputs.go | 11 ++++---- internal/catalog/inputs_test.go | 10 +++++-- internal/compose/compose.go | 6 ++-- internal/compose/inputs_test.go | 50 +++++++++++++++++++++++++++++++++ internal/site/inputs_test.go | 14 --------- internal/site/profile.go | 28 ------------------ 7 files changed, 68 insertions(+), 55 deletions(-) diff --git a/docs/openapi.yaml b/docs/openapi.yaml index 1ea3720..3636494 100644 --- a/docs/openapi.yaml +++ b/docs/openapi.yaml @@ -1798,8 +1798,8 @@ components: Answers to variants' `inputs`, by each input's key. Unlike `extra`, an answer reaches only a variant that declares the key, in the `derived` layer. A string may use `model.pathTemplate`'s placeholders - (`{{hf}}`, `{{org}}`, `{{name}}`, `{{model}}`). A key `extra` also sets is - refused. + (`{{hf}}`, `{{org}}`, `{{name}}`, `{{model}}`). Where `extra` sets the + same key, `extra` wins. additionalProperties: { type: [string, number, boolean] } examples: - storageKVOffload.mooncake.master: mooncake-master.svc:50051 diff --git a/internal/catalog/inputs.go b/internal/catalog/inputs.go index ea9e7ec..4dce50d 100644 --- a/internal/catalog/inputs.go +++ b/internal/catalog/inputs.go @@ -34,8 +34,7 @@ const ( var inputKey = regexp.MustCompile(`^[A-Za-z_][A-Za-z0-9_-]*(\.[A-Za-z_][A-Za-z0-9_-]*)*$`) -// ValidInputKey is shared with the site profile, which keys its answers the same way. -func ValidInputKey(k string) bool { return inputKey.MatchString(k) } +func validInputKey(k string) bool { return inputKey.MatchString(k) } func (i Input) TypeOrDefault() string { if i.Type == "" { @@ -94,13 +93,13 @@ func isInteger(v any) bool { return false } -// checkInputs: values leaves every input to compose, which answers it in the derived layer. +// checkInputs: a value values gives an input's key is the base compose's answer overrides. func checkInputs(inputs []Input, vals values.Tree) error { declared := map[string]bool{} for _, in := range inputs { where := "inputs " + in.Key switch { - case !ValidInputKey(in.Key): + case !validInputKey(in.Key): return fmt.Errorf("inputs: key %q is not a dotted chart key", in.Key) case in.Key == "model.localPath": return fmt.Errorf("%s: swiss answers it from the site's model.pathTemplate, so it is never an input", where) @@ -112,8 +111,8 @@ func checkInputs(inputs []Input, vals values.Tree) error { return fmt.Errorf("%s: pattern is only for a string input, and this one is %s", where, in.TypeOrDefault()) } declared[in.Key] = true - if at := setAt(vals, strings.Split(in.Key, ".")); at != "" { - return fmt.Errorf("%s: values.%s sets it; give the value as the input's default", where, at) + if at := setAt(vals, strings.Split(in.Key, ".")); at != "" && at != in.Key { + return fmt.Errorf("%s: values.%s is not a map, so the answer has no place to go", where, at) } if in.Pattern != "" { if _, err := in.pattern(); err != nil { diff --git a/internal/catalog/inputs_test.go b/internal/catalog/inputs_test.go index 018a62b..0745428 100644 --- a/internal/catalog/inputs_test.go +++ b/internal/catalog/inputs_test.go @@ -38,14 +38,20 @@ func TestAnInputsDefaultAndExampleAreAnswers(t *testing.T) { t.Errorf("default %v: %v", in.Default, err) } } + // values may set an input's key: it is the base an answer overrides. + for _, base := range []any{"mooncake-master.example:50051", ""} { + ok := inputVariant([]Input{mooncakeMaster}, values.Tree{"storageKVOffload": map[string]any{"mooncake": map[string]any{"master": base}}}) + if err := ok.validate(); err != nil { + t.Errorf("values base %q: %v", base, err) + } + } one := func(in Input) []Input { return []Input{in} } at := func(in Input, edit func(*Input)) Input { edit(&in); return in } for name, c := range map[string]struct { v Variant want string }{ - "values sets it": {inputVariant(one(mooncakeMaster), values.Tree{"storageKVOffload": map[string]any{"mooncake": map[string]any{"master": "mc.svc:50051"}}}), "values.storageKVOffload.mooncake.master sets it; give the value as the input's default"}, - "a non-map above it": {inputVariant(one(mooncakeMaster), values.Tree{"storageKVOffload": map[string]any{"mooncake": "off"}}), "values.storageKVOffload.mooncake sets it"}, + "a non-map above it": {inputVariant(one(mooncakeMaster), values.Tree{"storageKVOffload": map[string]any{"mooncake": "off"}}), "values.storageKVOffload.mooncake is not a map"}, "default off pattern": {inputVariant(one(at(mooncakeMaster, func(i *Input) { i.Default = "no-port" })), nil), `default "no-port" does not match`}, "a blank default": {inputVariant(one(at(mooncakeMaster, func(i *Input) { i.Default = "" })), nil), `default "" does not match`}, "default of a type": {inputVariant(one(Input{Key: "a.b", Title: "t", Type: InputInteger, Example: 1, Default: "8"}), nil), "default 8 is not an integer"}, diff --git a/internal/compose/compose.go b/internal/compose/compose.go index 4b855d5..d5fa75e 100644 --- a/internal/compose/compose.go +++ b/internal/compose/compose.go @@ -385,8 +385,8 @@ func applyDefaults(site, derived, base values.Tree, in Input) error { return nil } -// applyInputs puts each input's answer in the derived layer, under the form's -// overrides: the site's, else the input's default. +// applyInputs puts each input's answer in the derived layer, over the variant's +// values and under the form's overrides: the site's, else the input's default. func applyInputs(derived, site values.Tree, in Input) error { for _, input := range in.Resolved.Variant.Inputs { answer, ok, err := in.Profile.InputAnswer(input.Key, in.Resolved.Source.HF, in.Resolved.Model) @@ -400,7 +400,7 @@ func applyInputs(derived, site values.Tree, in Input) error { continue } if _, set := values.Get(site, input.Key); set { - continue + continue // extra wins over an input answer } if err := values.Set(derived, input.Key, answer); err != nil { return err diff --git a/internal/compose/inputs_test.go b/internal/compose/inputs_test.go index 322143c..b31191a 100644 --- a/internal/compose/inputs_test.go +++ b/internal/compose/inputs_test.go @@ -106,6 +106,56 @@ func TestTheVariantsDefaultIsTheLowestAnswer(t *testing.T) { } } +// A value the variant's values give the key stands when nobody answers, and +// any answer overrides it. +func TestTheVariantsValueIsTheBaseAnAnswerOverrides(t *testing.T) { + for name, c := range map[string]struct { + site map[string]any + def any + want string + layer string + }{ + "nobody answers": {want: "mooncake-master.example:50051", layer: values.LayerCatalog}, + "the site answers": {site: map[string]any{masterKey: "site.svc:50051"}, want: "site.svc:50051", layer: values.LayerDerived}, + "the default answers": {def: "tuned.svc:50051", want: "tuned.svc:50051", layer: values.LayerDerived}, + } { + in := inputsInput() + in.Resolved.Variant.Values["storageKVOffload"].(map[string]any)["mooncake"].(map[string]any)["master"] = "mooncake-master.example:50051" + in.Resolved.Variant.Inputs[0].Default = c.def + in.Profile.Inputs = c.site + p, err := Compose(in) + if err != nil { + t.Fatalf("%s: %v", name, err) + } + if got, _ := values.Get(p.Values(), masterKey); got != c.want || p.LayerOf(masterKey) != c.layer { + t.Errorf("%s: %s = %v from %s, want %s from %s", name, masterKey, got, p.LayerOf(masterKey), c.want, c.layer) + } + } +} + +// extra sets the key too: extra wins, and the input answer does nothing. +func TestExtraOverridesTheSitesInputAnswer(t *testing.T) { + in := inputsInput() + in.Profile.Inputs = map[string]any{masterKey: "site.svc:50051"} + in.Profile.Extra = values.Tree{"storageKVOffload": map[string]any{"mooncake": map[string]any{"master": "extra.svc:50051"}}} + p, err := Compose(in) + if err != nil { + t.Fatal(err) + } + if got, _ := values.Get(p.Values(), masterKey); got != "extra.svc:50051" || p.LayerOf(masterKey) != values.LayerSite { + t.Errorf("%s = %v from %s, want extra's", masterKey, got, p.LayerOf(masterKey)) + } +} + +// The profile takes inputs as written; compose refuses one it cannot fill. +func TestASiteAnswerWithAnUnknownPlaceholderRefusesThePlan(t *testing.T) { + in := inputsInput() + in.Profile.Inputs = map[string]any{masterKey: "{{release}}.svc:50051"} + if _, err := Compose(in); err == nil || !strings.Contains(err.Error(), "unknown placeholder") { + t.Fatalf("want a refusal naming the placeholder, got %v", err) + } +} + func TestAnAnswerTheInputDoesNotTakeRefusesThePlan(t *testing.T) { in := inputsInput() in.Profile.Inputs = map[string]any{masterKey: "no-port"} diff --git a/internal/site/inputs_test.go b/internal/site/inputs_test.go index 5f5b502..82b6441 100644 --- a/internal/site/inputs_test.go +++ b/internal/site/inputs_test.go @@ -43,17 +43,3 @@ func TestATemplateNamingTheRepoNeedsASource(t *testing.T) { t.Errorf("{{model}} needs no source: %v, %v", got, err) } } - -func TestAnAnswerThatCouldNeverBeUsedIsRefused(t *testing.T) { - for name, c := range map[string]struct{ yaml, want string }{ - "unknown placeholder": {"inputs:\n a.b: /x/{{release}}\n", "unknown placeholder"}, - "not a scalar": {"inputs:\n a.b: {host: h}\n", "string, integer or boolean"}, - "no value": {"inputs:\n a.b:\n", "string, integer or boolean"}, - "not a key": {"inputs:\n a..b: x\n", "not a dotted chart key"}, - "extra answers too": {"extra:\n a:\n b: y\ninputs:\n a.b: x\n", "extra sets the same key"}, - } { - if _, err := Parse([]byte(inputsHead+c.yaml), "test"); err == nil || !strings.Contains(err.Error(), c.want) { - t.Errorf("%s: want an error containing %q, got %v", name, c.want, err) - } - } -} diff --git a/internal/site/profile.go b/internal/site/profile.go index 9abece5..386b6a6 100644 --- a/internal/site/profile.go +++ b/internal/site/profile.go @@ -7,9 +7,7 @@ package site import ( "fmt" - "maps" "os" - "slices" "strings" "time" @@ -208,29 +206,6 @@ func (f FeatureDefaults) Resolve() map[string]bool { } } -// checkInputs refuses an answer that could never be used, or a second answer from extra. -func checkInputs(inputs map[string]any, extra values.Tree) error { - for _, key := range slices.Sorted(maps.Keys(inputs)) { - where := fmt.Sprintf("inputs[%q]", key) - if !catalog.ValidInputKey(key) { - return fmt.Errorf("%s: not a dotted chart key", where) - } - switch v := inputs[key].(type) { - case string: - if _, err := fill(v, "org/name", "model"); err != nil { - return fmt.Errorf("%s: %w", where, err) - } - case bool, int, int64, uint64, float64: - default: - return fmt.Errorf("%s: an answer is a string, integer or boolean, not %T", where, v) - } - if _, set := values.Get(extra, key); set { - return fmt.Errorf("%s: extra sets the same key; give the one answer here", where) - } - } - return nil -} - // InputAnswer is the site's answer to an input, placeholders filled for this model. func (p Profile) InputAnswer(key, hf, model string) (answer any, ok bool, err error) { v, ok := p.Inputs[key] @@ -553,9 +528,6 @@ func Parse(raw []byte, origin string) (*Profile, error) { if err := p.Nodes.checkGPUs(); err != nil { return nil, fmt.Errorf("%s: %w", origin, err) } - if err := checkInputs(p.Inputs, p.Extra); err != nil { - return nil, fmt.Errorf("%s: %w", origin, err) - } return &p, nil } From a31977ff99966dac4b0c60d56f2959ccc408ea7e Mon Sep 17 00:00:00 2001 From: aceforeverd Date: Tue, 10 Nov 2026 13:36:06 +0800 Subject: [PATCH 2/2] doc update --- .github/pull_request_template.md | 1 + AGENTS.md | 7 +++++++ 2 files changed, 8 insertions(+) create mode 100644 .github/pull_request_template.md diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md new file mode 100644 index 0000000..dea5c08 --- /dev/null +++ b/.github/pull_request_template.md @@ -0,0 +1 @@ + diff --git a/AGENTS.md b/AGENTS.md index 97a2a5e..05a14e9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,6 +4,13 @@ swiss is the backend server and cli client, changelog are made from that perspective, UI things left to console, not here. +## Before calling a change done + +- **New feature:** what real problem does it solve that the current code can't? If you can't name one, don't add it. +- **Scope:** is it over-designed, or guarding edge cases nobody asked about? Prefer the simplest change, and let an existing layer or check handle the rest. +- **API:** is every API-level change (endpoint, request or response field, site profile key) synced in `docs/openapi.yaml`? +- **Compatibility:** could it break an older console or an older catalog? swissd runs against both across versions. + ## Comments - Minimal. Only where the *why* cannot be read off the code.