diff --git a/cli/azd/extensions/azure.ai.evaluations/README.md b/cli/azd/extensions/azure.ai.evaluations/README.md index ee4faf9de69..a27c5be2706 100644 --- a/cli/azd/extensions/azure.ai.evaluations/README.md +++ b/cli/azd/extensions/azure.ai.evaluations/README.md @@ -60,7 +60,9 @@ That holds for the configuration as a whole. It does **not** hold for a `$ref` on a single catalog entry: azd rebases only the path keys it owns, so a relative `source:` written inside `evals/evaluators/quality.yaml` still resolves against `azure.eval.yaml` and will not be found. An entry pulled in from its own file -should carry the rubric under `definition:` rather than point at a second file: +should carry the rubric rather than point at a second file — either written out +under `definition:`, or as a `$ref` straight at the rubric, whose keys are +spliced in and become that `definition:`: ```yaml evaluators: @@ -68,6 +70,11 @@ evaluators: name: quality ``` +An entry declared this way is read and deployed normally, but it lives in the +referenced file, so `azd ai eval generate` will not update it in place and says +so rather than writing a second declaration of the same rubric beside the +directive. Edit the referenced file, or generate under a different name. + ### Repeated deploys do not create redundant versions Datasets are fingerprinted locally, because the dataset API exposes no content diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/cmd/catalog.go b/cli/azd/extensions/azure.ai.evaluations/internal/cmd/catalog.go index 60b1db39cbf..9800fe1ff61 100644 --- a/cli/azd/extensions/azure.ai.evaluations/internal/cmd/catalog.go +++ b/cli/azd/extensions/azure.ai.evaluations/internal/cmd/catalog.go @@ -66,21 +66,32 @@ func addEvaluatorToCatalog(cmd *cobra.Command, evalDir string, ref *project.Arti }) } -// checkNameNotBehindAnInclude refuses a name whose entry lives in another file. +// checkCatalogEntryIsEditable refuses a name this command cannot rewrite in +// place without corrupting the entry. // -// Two shapes reach this. A pure `$ref` has no name here at all, so the duplicate -// scan had nothing to match on and appended a second entry with the same name -- -// a collision that surfaced only on the next resolving read. A `$ref` carrying -// an overlay `name` does match, and updating it in place writes `source:` beside -// the directive, so resolution then produces a rubric and a source and the -// configuration is rejected for declaring it twice. Neither is editable here. +// Four shapes reach this. A pure `$ref` has no name here at all, so the +// duplicate scan had nothing to match on and appended a second entry with the +// same name -- a collision that surfaced only on the next resolving read. A +// `$ref` carrying an overlay `name` does match, and updating it in place writes +// `source:` beside the directive, so resolution then produces a rubric and a +// source and the configuration is rejected for declaring it twice. An entry +// already carrying its rubric under `definition:`, and an entry pinned to a +// registered `version:`, both fail the same way with no include involved: +// recording the generated file leaves two rubrics, or a pin and a file, in one +// entry. Each of those is refused on the next read -- after the generation job +// has been billed and the file written, which is why they are refused here. // // A configuration that will not resolve is left to the commands that resolve it: // failing a generate over an unrelated broken include would be its own surprise. -func checkNameNotBehindAnInclude(evalDir string, asWritten *project.EvalConfig, kind, name string) error { - if ref, ok := catalogEntryRef(asWritten, kind, name); ok { - if ref != "" { +func checkCatalogEntryIsEditable(evalDir string, asWritten *project.EvalConfig, kind, name string) error { + if entry, ok := catalogEntryShapeOf(asWritten, kind, name); ok { + switch { + case entry.ref != "": return messages.CatalogNameBehindAnInclude(kind, name) + case entry.inlineRubric: + return messages.EvaluatorRubricWrittenInPlace(name) + case entry.pinned: + return messages.EvaluatorPinnedToAVersion(name) } return nil } @@ -88,28 +99,45 @@ func checkNameNotBehindAnInclude(evalDir string, asWritten *project.EvalConfig, if err != nil || resolved == nil { return nil } - if _, ok := catalogEntryRef(resolved, kind, name); ok { + if _, ok := catalogEntryShapeOf(resolved, kind, name); ok { return messages.CatalogNameBehindAnInclude(kind, name) } return nil } -// catalogEntryRef returns the include this entry was written as, and whether the +// catalogEntryShape is how an entry was written, for deciding whether this +// command may rewrite it. +type catalogEntryShape struct { + // ref is the `$ref` directive the entry carries, empty when it is written + // out here. + ref string + // inlineRubric is an evaluator holding its rubric under `definition:`. + inlineRubric bool + // pinned is an evaluator naming a registered `version:`. A dataset may hold + // a file and a version together; an evaluator may not. + pinned bool +} + +// catalogEntryShapeOf returns how the entry was written, and whether the // configuration names it at all. -func catalogEntryRef(cfg *project.EvalConfig, kind, name string) (string, bool) { +func catalogEntryShapeOf(cfg *project.EvalConfig, kind, name string) (catalogEntryShape, bool) { if cfg == nil { - return "", false + return catalogEntryShape{}, false } if kind == "dataset" { if decl, ok := cfg.DatasetDeclaration(name); ok { - return decl.Ref, true + return catalogEntryShape{ref: decl.Ref}, true } - return "", false + return catalogEntryShape{}, false } if decl, ok := cfg.EvaluatorDeclaration(name); ok { - return decl.Ref, true + return catalogEntryShape{ + ref: decl.Ref, + inlineRubric: decl.Definition != nil, + pinned: decl.Version != "", + }, true } - return "", false + return catalogEntryShape{}, false } // updateCatalog applies a change to the configuration and writes it back. @@ -142,7 +170,7 @@ func updateCatalog( if created { cfg = &project.EvalConfig{} } - if err := checkNameNotBehindAnInclude(evalDir, cfg, kind, ref.Name); err != nil { + if err := checkCatalogEntryIsEditable(evalDir, cfg, kind, ref.Name); err != nil { return err } if !apply(cfg) { diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/cmd/catalog_include_test.go b/cli/azd/extensions/azure.ai.evaluations/internal/cmd/catalog_include_test.go index f6a1a68cedd..27e569f828c 100644 --- a/cli/azd/extensions/azure.ai.evaluations/internal/cmd/catalog_include_test.go +++ b/cli/azd/extensions/azure.ai.evaluations/internal/cmd/catalog_include_test.go @@ -32,7 +32,7 @@ evaluators: - $ref: ./parts/quality.yaml `), 0o600)) - err := checkNameNotBehindAnInclude( + err := checkCatalogEntryIsEditable( dir, mustOpenForEdit(t, dir), "evaluator", "quality") require.Error(t, err, "the name is taken, even though this file does not show it") @@ -53,7 +53,7 @@ evaluators: - $ref: ./parts/quality.yaml `), 0o600)) - require.NoError(t, checkNameNotBehindAnInclude( + require.NoError(t, checkCatalogEntryIsEditable( dir, mustOpenForEdit(t, dir), "evaluator", "tone")) } @@ -77,13 +77,141 @@ evaluators: name: quality `), 0o600)) - err := checkNameNotBehindAnInclude( + err := checkCatalogEntryIsEditable( dir, mustOpenForEdit(t, dir), "evaluator", "quality") require.Error(t, err, "the entry is an include, so it cannot be updated in place") assert.Contains(t, err.Error(), "quality") } +// The dataset branch of the guard is its own lookup, so it gets its own tests. +// +// Every case above is an evaluator, and `catalogEntryShapeOf` dispatches on kind +// before it looks anything up. A regression in the dataset branch would +// reintroduce the duplicate entries the guard exists to prevent while the +// evaluator tests stayed green. +func TestGenerateRefusesADatasetNameAnIncludeAlreadyDeclares(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "parts"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "parts", "golden.yaml"), + []byte("name: golden\nfile: ./datasets/golden.jsonl\n"), 0o600)) + + require.NoError(t, os.WriteFile(filepath.Join(dir, project.EvalConfigBase), []byte(` +datasets: + - $ref: ./parts/golden.yaml +`), 0o600)) + + err := checkCatalogEntryIsEditable( + dir, mustOpenForEdit(t, dir), "dataset", "golden") + + require.Error(t, err, "the dataset name is taken by the included file") + assert.Contains(t, err.Error(), "golden") + assert.Contains(t, err.Error(), "dataset", "the message names the kind it refused") +} + +// A dataset include carrying an overlay `name`, the shape the evaluator test +// above covers, refused through the dataset branch. +func TestGenerateRefusesADatasetIncludeThatCarriesItsName(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "parts"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "parts", "golden.yaml"), + []byte("file: ./datasets/golden.jsonl\n"), 0o600)) + + require.NoError(t, os.WriteFile(filepath.Join(dir, project.EvalConfigBase), []byte(` +datasets: + - $ref: ./parts/golden.yaml + name: golden +`), 0o600)) + + err := checkCatalogEntryIsEditable( + dir, mustOpenForEdit(t, dir), "dataset", "golden") + + require.Error(t, err, "the entry is an include, so it cannot be updated in place") + assert.Contains(t, err.Error(), "golden") +} + +// An evaluator already carrying its rubric under `definition:` is refused. +// +// No include is involved. Recording a generated file against it writes +// `source:` into an entry that already holds a `definition:`, and the next read +// rejects the whole configuration for declaring the rubric twice -- after the +// generation job has been billed and the file written. Refusing here is what +// keeps the failure ahead of the cost. +func TestGenerateRefusesAnEvaluatorThatAlreadyCarriesItsRubric(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, project.EvalConfigBase), []byte(` +evaluators: + - name: quality + definition: + type: rubric + dimensions: + - id: tone + weight: 3 +`), 0o600)) + + err := checkCatalogEntryIsEditable( + dir, mustOpenForEdit(t, dir), "evaluator", "quality") + + require.Error(t, err, "there is nowhere to record a file without declaring the rubric twice") + assert.Contains(t, err.Error(), "quality") + assert.Contains(t, err.Error(), "definition", "the reader has to be told which half is already there") +} + +// An entry written out here, with no include and no inline rubric, stays +// editable -- the case the guard must not catch. +func TestGenerateStillUpdatesAnEntryWrittenInThisFile(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, project.EvalConfigBase), []byte(` +datasets: + - name: golden + file: ./datasets/golden.jsonl +evaluators: + - name: quality + source: ./evaluators/quality.json +`), 0o600)) + + cfg := mustOpenForEdit(t, dir) + require.NoError(t, checkCatalogEntryIsEditable(dir, cfg, "evaluator", "quality")) + require.NoError(t, checkCatalogEntryIsEditable(dir, cfg, "dataset", "golden")) +} + +// An evaluator pinned to a registered version is refused, for the same reason +// an inline rubric is: there is nowhere to record the generated file. +// +// A pin says the rubric already lives in the service. Writing `source:` beside +// it leaves the entry claiming both, which the next read rejects -- again after +// the job has been billed and the file written. +func TestGenerateRefusesAnEvaluatorPinnedToAVersion(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, project.EvalConfigBase), []byte(` +evaluators: + - name: quality + version: "3" +`), 0o600)) + + err := checkCatalogEntryIsEditable( + dir, mustOpenForEdit(t, dir), "evaluator", "quality") + + require.Error(t, err, "a pin and a file in one entry is refused on the next read") + assert.Contains(t, err.Error(), "quality") + assert.Contains(t, err.Error(), "version", "the reader has to be told what is already there") +} + +// A dataset may carry a file and a version together -- the version says which +// one to publish -- so the pin must not make it uneditable. +func TestGenerateStillUpdatesAVersionedDataset(t *testing.T) { + dir := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(dir, project.EvalConfigBase), []byte(` +datasets: + - name: golden + file: ./datasets/golden.jsonl + version: "4" +`), 0o600)) + + require.NoError(t, checkCatalogEntryIsEditable( + dir, mustOpenForEdit(t, dir), "dataset", "golden")) +} + func mustOpenForEdit(t *testing.T, dir string) *project.EvalConfig { t.Helper() cfg, err := project.OpenEvalConfigForEdit(dir) diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/cmd/eval_group.go b/cli/azd/extensions/azure.ai.evaluations/internal/cmd/eval_group.go index e255a4dfa25..56486220048 100644 --- a/cli/azd/extensions/azure.ai.evaluations/internal/cmd/eval_group.go +++ b/cli/azd/extensions/azure.ai.evaluations/internal/cmd/eval_group.go @@ -118,7 +118,7 @@ func newEvalCreateCommand() *cobra.Command { for _, ref := range eval.Evaluators { decl, ok := cfg.EvaluatorDeclaration(ref.Evaluator) // A built-in, or one already registered, has nothing local to publish. - if !ok || (decl.Source == "" && decl.Definition == nil) { + if !ok || !decl.CarriesItsRubric() { continue } // A rubric written out in the configuration has no file to read. diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/messages/messages.go b/cli/azd/extensions/azure.ai.evaluations/internal/messages/messages.go index 2c66e9469a0..affd440b0ec 100644 --- a/cli/azd/extensions/azure.ai.evaluations/internal/messages/messages.go +++ b/cli/azd/extensions/azure.ai.evaluations/internal/messages/messages.go @@ -1432,15 +1432,43 @@ func RefNeedsAProjectRoot(service string) error { // CatalogNameBehindAnInclude reports a name declared through a `$ref`, which // this command cannot edit in place. +// +// One message for two shapes, so the cause is stated as what they share: the +// entry lives in the referenced file. Naming only the duplicate-on-resolve case +// would misdescribe an overlay `name`, which collides with nothing and instead +// ends up declaring the rubric twice. func CatalogNameBehindAnInclude(kind, name string) error { return fmt.Errorf( - "%s %q is already declared through a `$ref`, so this command cannot update "+ - "it here: adding a second entry would collide with the first only after "+ - "the include is resolved. Edit the referenced file, or generate under a "+ - "different name", + "%s %q is declared in a file pulled in with `$ref`, so this command cannot "+ + "update it here: an entry written beside the directive takes effect only "+ + "once the include is resolved, and not as it reads. Edit the referenced "+ + "file, or generate under a different name", kind, name) } +// EvaluatorRubricWrittenInPlace reports an evaluator whose rubric is already +// written out under `definition:`, so there is nowhere to record a generated +// file without declaring the rubric twice. +func EvaluatorRubricWrittenInPlace(name string) error { + return fmt.Errorf( + "evaluator %q already carries its rubric under `definition:`, so this "+ + "command cannot record a generated file against it: an entry holding "+ + "both a `definition:` and a `source:` is refused on the next read. Edit "+ + "the rubric in place, or generate under a different name", + name) +} + +// EvaluatorPinnedToAVersion reports an evaluator pinned to a registered +// version, which leaves nowhere to record a generated file. +func EvaluatorPinnedToAVersion(name string) error { + return fmt.Errorf( + "evaluator %q is pinned to a registered `version:`, so this command cannot "+ + "record a generated file against it: an entry holding both a `version:` "+ + "and a `source:` is refused on the next read. Remove the pin to publish "+ + "from a file, or generate under a different name", + name) +} + // ReadingServiceConfig reports the service entry failing to serialize. func ReadingServiceConfig(err error) error { return fmt.Errorf("reading the eval service configuration: %w", err) diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config.go b/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config.go index 90e6a9d856c..ffb38bd20ca 100644 --- a/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config.go +++ b/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config.go @@ -215,13 +215,27 @@ func (c *EvalConfig) EvaluatorDeclaration(name string) (*EvaluatorDecl, bool) { return nil, false } +// CarriesItsRubric reports whether this configuration owns the evaluator and +// has to publish it, rather than referring to a built-in or to one already +// registered under this name. +// +// Both fields have to be tested, and this is the only place that should test +// them. Validation forbids declaring the rubric twice, so `definition` implies +// an empty `source`: a selector written as `source == ""` reads as "nothing +// local to publish" but silently drops every evaluator carrying its rubric +// inline. That shipped once already -- the eval was created bound to an +// evaluator the service had never been told about. +func (d EvaluatorDecl) CarriesItsRubric() bool { + return d.Source != "" || d.Definition != nil +} + // CustomEvaluators are the catalog entries this configuration owns -- the ones // carrying a rubric, either as a local source or written out under // `definition`, published before the evals that name them. func (c *EvalConfig) CustomEvaluators() []EvaluatorDecl { var owned []EvaluatorDecl for _, decl := range c.Evaluators { - if decl.Source == "" && decl.Definition == nil { + if !decl.CarriesItsRubric() { continue } owned = append(owned, decl) diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config_store.go b/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config_store.go index 89f658caa4b..cdd60f1a47c 100644 --- a/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config_store.go +++ b/cli/azd/extensions/azure.ai.evaluations/internal/project/eval_config_store.go @@ -11,6 +11,7 @@ import ( "os" "path/filepath" "runtime" + "slices" "syscall" "time" @@ -236,12 +237,12 @@ func resolveConfigRefs(data []byte, baseDir, name string) ([]byte, error) { // CLI command refused, and later the reverse. Callers differ in how they obtain // the map and what they do with it; everything between is here. func resolveEvalRefs(values map[string]any, baseDir string) (map[string]any, error) { - // Read before resolution, which removes the directive. Both routes gate the - // rescue on it so they cannot disagree: without it, the CLI's no-`$ref` fast - // path would skip nesting while the deploy path still applied it, and a - // hand-written entry carrying rubric keys would deploy and then be refused - // by every command that reads it. - spliced := containsRefDirective(values) + // Read before resolution, which consumes the directive. Both routes gate the + // rescue on the same answer so they cannot disagree: without it, the CLI's + // no-`$ref` fast path would skip nesting while the deploy path still applied + // it, and a hand-written entry carrying rubric keys would deploy and then be + // refused by every command that reads it. + spliced, visible := splicedEvaluators(values) resolved, err := foundry.ResolveFileRefs(values, baseDir) if err != nil { @@ -250,12 +251,41 @@ func resolveEvalRefs(values map[string]any, baseDir string) (map[string]any, err // `$ref` is a directive rather than configuration, and the strict decoder // would report the leftover as a mistyped key. delete(resolved, "$ref") - if spliced { - nestSplicedRubrics(resolved) - } + nestSplicedRubrics(resolved, spliced, visible) return resolved, nil } +// splicedEvaluators reports which evaluator entries carry an include of their +// own, and whether the list could be read at all. +// +// Entry level rather than document level. Asking only whether the document used +// `$ref` anywhere made one entry's meaning depend on another's: a directive on +// an unrelated dataset switched the rescue on for the whole file, so a +// hand-written `dimensions:` -- a mistake the strict decoder exists to report -- +// was filed as rubric content and published instead. The same evaluator was +// then refused or accepted according to a neighbour. +func splicedEvaluators(values map[string]any) (map[int]bool, bool) { + entries, ok := values["evaluators"].([]any) + if !ok { + // The configuration is itself behind a `$ref`, so its entries do not + // exist yet and nothing here was hand-written to protect. + return nil, false + } + spliced := map[int]bool{} + for i, entry := range entries { + m, ok := entry.(map[string]any) + if !ok { + continue + } + if _, has := m[refDirective]; has { + spliced[i] = true + } + } + // Position survives resolution: entries are replaced in place, never added + // or dropped. + return spliced, true +} + // containsRefDirective reports whether the document uses `$ref` anywhere. // // Structural rather than a text scan: the byte "$ref" also appears in comments @@ -273,11 +303,7 @@ func containsRefDirective(value any) bool { } } case []any: - for _, child := range typed { - if containsRefDirective(child) { - return true - } - } + return slices.ContainsFunc(typed, containsRefDirective) } return false } @@ -299,18 +325,22 @@ var evaluatorDeclKeys = map[string]bool{ // from the service -- so its keys land beside `name` and the strict decoder // rejects them. Moving them is what lets a `$ref` name a rubric. // -// `dimensions` is what marks the leftovers as a rubric rather than a typo, and -// it is the same key normalizeRubricBody insists on before it will treat a -// document as a definition. Without that gate this would be a catch-all by -// another name, filing a misspelled `name` as rubric content and publishing it -// to the service instead of reporting it. +// Only the entries that carried a directive are touched. `dimensions` then +// marks the leftovers as a rubric rather than a typo, and it is the same key +// normalizeRubricBody insists on before it will treat a document as a +// definition. Without both gates this is a catch-all by another name, filing a +// misspelled `name` as rubric content and publishing it to the service instead +// of reporting it. // -// Structural rather than positional on purpose: an earlier version marked -// entries by index before resolution, which cannot see the evaluators inside a -// config that is itself behind a `$ref` -- the layout the README documents. -func nestSplicedRubrics(resolved map[string]any) { +// visible is false when the configuration is itself behind a `$ref`, where the +// entries only exist after resolution and there is nothing written here to tell +// them apart from. That is the layout the README documents. +func nestSplicedRubrics(resolved map[string]any, spliced map[int]bool, visible bool) { entries, _ := resolved["evaluators"].([]any) - for _, entry := range entries { + for i, entry := range entries { + if visible && !spliced[i] { + continue + } m, ok := entry.(map[string]any) if !ok { continue diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/project/neighbour_ref_test.go b/cli/azd/extensions/azure.ai.evaluations/internal/project/neighbour_ref_test.go new file mode 100644 index 00000000000..35f87e742a0 --- /dev/null +++ b/cli/azd/extensions/azure.ai.evaluations/internal/project/neighbour_ref_test.go @@ -0,0 +1,66 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package project + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A `$ref` on one entry does not change what a different entry means. +// +// The rescue that moves spliced rubric keys under `definition` used to be gated +// on "this document uses `$ref` somewhere". A directive on an unrelated dataset +// therefore switched it on for every evaluator in the file, and a hand-written +// `dimensions:` -- a mistake the strict decoder exists to report -- was silently +// filed as rubric content and published to the service instead. +// +// The same evaluator, refused in one file and accepted in another because of a +// neighbour, is the shape this whole mechanism is supposed to rule out. +func TestARefOnOneEntryDoesNotRescueAnother(t *testing.T) { + withoutRef := ` +datasets: + - name: golden + file: ./datasets/golden.jsonl +evaluators: + - name: quality + dimensions: + - id: tone + weight: 3 +` + withUnrelatedRef := ` +datasets: + - $ref: ./parts/golden.yaml +evaluators: + - name: quality + dimensions: + - id: tone + weight: 3 +` + + refused := func(t *testing.T, body string) error { + t.Helper() + dir := t.TempDir() + require.NoError(t, os.MkdirAll(filepath.Join(dir, "parts"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(dir, "parts", "golden.yaml"), + []byte("name: golden\nfile: ./datasets/golden.jsonl\n"), 0o600)) + require.NoError(t, os.WriteFile(filepath.Join(dir, EvalConfigBase), []byte(body), 0o600)) + _, err := OpenEvalConfig(dir) + return err + } + + baseline := refused(t, withoutRef) + require.Error(t, baseline, + "a rubric key written at entry level is a mistake, and the strict decoder reports it") + assert.Contains(t, baseline.Error(), "dimensions") + + neighbour := refused(t, withUnrelatedRef) + require.Error(t, neighbour, + "the dataset's `$ref` says nothing about this evaluator, so the same entry is still a mistake") + assert.Contains(t, neighbour.Error(), "dimensions") +} diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/project/one_ownership_test.go b/cli/azd/extensions/azure.ai.evaluations/internal/project/one_ownership_test.go new file mode 100644 index 00000000000..7657a5f34b0 --- /dev/null +++ b/cli/azd/extensions/azure.ai.evaluations/internal/project/one_ownership_test.go @@ -0,0 +1,74 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package project + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Whether this configuration owns an evaluator is decided in exactly one place. +// +// Two publish loops ask it -- `azd up` through CustomEvaluators, and `eval +// create` over the evaluators one eval names. Each carried its own copy of the +// test, and the first version of both read `source == ""`. That is wrong in a +// way nothing surfaces: validation forbids declaring the rubric twice, so an +// evaluator carrying its rubric under `definition:` has an empty `source`, was +// read as "nothing local to publish", and was skipped. The eval was then created +// bound to an evaluator the service had never been told about. +// +// A second copy of the predicate is how that returns, and it returns quietly -- +// the config still decodes, the deploy still reports success. So the shape is +// worth failing the build over rather than trusting a reviewer to spot it. +func TestEvaluatorOwnershipIsDecidedInOnePlace(t *testing.T) { + const predicate = "CarriesItsRubric" + + // Either half of the pair, written inline. `Definition == nil` on its own is + // legitimate -- the reconciler branches on it to choose what to publish -- + // so it is the pairing with a `Source` test that means someone has + // re-derived ownership. + sightings := map[string][]int{} + require.NoError(t, filepath.WalkDir("../..", func(path string, d os.DirEntry, err error) error { + if err != nil || d.IsDir() || !strings.HasSuffix(path, ".go") { + return err + } + if strings.HasSuffix(path, "_test.go") { + return nil // including this file, which spells out the shape it is looking for + } + body, err := os.ReadFile(path) + if err != nil { + return err + } + insidePredicate := false + for i, line := range strings.Split(string(body), "\n") { + // The predicate is the one place allowed to say this, so skip its body. + if strings.Contains(line, predicate+"() bool {") { + insidePredicate = true + continue + } + if insidePredicate { + if strings.HasPrefix(line, "}") { + insidePredicate = false + } + continue + } + source := strings.Contains(line, `Source == ""`) || strings.Contains(line, `Source != ""`) + definition := strings.Contains(line, "Definition == nil") || strings.Contains(line, "Definition != nil") + if source && definition { + sightings[path] = append(sightings[path], i+1) + } + } + return nil + })) + + assert.Empty(t, sightings, + "ownership is re-derived at %v; call %s instead, or the next rule about "+ + "what this configuration publishes will land on one loop and not the other", + sightings, predicate) +} diff --git a/cli/azd/extensions/azure.ai.evaluations/internal/project/service_target_eval.go b/cli/azd/extensions/azure.ai.evaluations/internal/project/service_target_eval.go index dc6252471ee..f4812538cdd 100644 --- a/cli/azd/extensions/azure.ai.evaluations/internal/project/service_target_eval.go +++ b/cli/azd/extensions/azure.ai.evaluations/internal/project/service_target_eval.go @@ -135,7 +135,13 @@ func (p *EvalServiceTargetProvider) Deploy( targetResource *azdext.TargetResource, progress azdext.ProgressReporter, ) (*azdext.ServiceDeployResult, error) { - cfg, err := EvalConfigFromService(serviceConfig, p.projectRoot(ctx)) + // Asked once and reused: a second call could fail where the first + // succeeded, and the empty root that comes back is indistinguishable from a + // project that has none. The include guard below would have passed while + // artifact paths quietly resolved against this process's directory instead. + projectRoot := p.projectRoot(ctx) + + cfg, err := EvalConfigFromService(serviceConfig, projectRoot) if err != nil { return nil, err } @@ -148,7 +154,7 @@ func (p *EvalServiceTargetProvider) Deploy( return nil, err } - baseDir := p.evalBaseDir(ctx, serviceConfig) + baseDir := baseDirUnder(projectRoot, serviceConfig) // 1. Datasets the configuration owns. Paths are kept so an eval that names // one can derive its columns without reading the blob back. @@ -218,7 +224,8 @@ func (p *EvalServiceTargetProvider) projectRoot(ctx context.Context) string { return resp.GetProject().GetPath() } -// evalBaseDir is the directory a declaration's `source:` resolves against. +// baseDirUnder places a service's directory under the project -- the directory +// a declaration's `source:` resolves against. // // serviceRelativeDir answers relative to the project, because that is what the // service's `$ref` and relativePath are written relative to. Left there it was @@ -230,14 +237,6 @@ func (p *EvalServiceTargetProvider) projectRoot(ctx context.Context) string { // generation job to rewrite a file already on disk. // // The same join is what agent_instructions.go does with the same helper. -func (p *EvalServiceTargetProvider) evalBaseDir( - ctx context.Context, - serviceConfig *azdext.ServiceConfig, -) string { - return baseDirUnder(p.projectRoot(ctx), serviceConfig) -} - -// baseDirUnder places a service's directory under the project. // // azd does not re-root an absolute `$ref` or an absolute `project:`, so neither // does this: joining one under the project produced /C:/shared/evals, diff --git a/cli/azd/extensions/azure.ai.evaluations/schemas/azure.ai.eval.json b/cli/azd/extensions/azure.ai.evaluations/schemas/azure.ai.eval.json index 0bcee881584..f980d734e7d 100644 --- a/cli/azd/extensions/azure.ai.evaluations/schemas/azure.ai.eval.json +++ b/cli/azd/extensions/azure.ai.evaluations/schemas/azure.ai.eval.json @@ -61,7 +61,7 @@ }, "file": { "type": "string", - "description": "Path to a local .jsonl whose rows are uploaded on deploy, relative to this file. Deliberately not a $ref: the rows are a data artifact to publish, not a definition to splice in, and a .jsonl is neither YAML nor JSON. Omit to use a dataset already registered under this name." + "description": "Path to a local .jsonl whose rows are uploaded on deploy, resolved against the evaluation configuration -- also when this entry arrived through a $ref, because a spliced path is not rebased to the file it was written in. Deliberately not a $ref: the rows are a data artifact to publish, not a definition to splice in, and a .jsonl is neither YAML nor JSON. Omit to use a dataset already registered under this name." }, "version": { "type": "string", @@ -80,7 +80,7 @@ }, "source": { "type": "string", - "description": "Path to a local .json rubric -- a list of weighted scoring dimensions -- relative to this file. Published on deploy and fingerprinted locally so a later deploy can tell an edit here from a version published elsewhere." + "description": "Path to a local .json rubric -- a list of weighted scoring dimensions -- resolved against the evaluation configuration, also when this entry arrived through a $ref, because a spliced path is not rebased; an entry pulled in from its own file should carry the rubric under definition instead. Published on deploy and fingerprinted locally so a later deploy can tell an edit here from a version published elsewhere." }, "definition": { "type": "object", @@ -107,6 +107,11 @@ "$comment": "A rubric is named or written out, never both.", "if": { "required": ["definition"] }, "then": { "properties": { "source": false } } + }, + { + "$comment": "A version pins one already registered, so there is nothing local to publish alongside it. Both shapes are refused when the configuration is read; stating them here is what makes the editor agree.", + "if": { "required": ["version"] }, + "then": { "properties": { "source": false, "definition": false } } } ] },