Reuse origin metadata across configuration collections
This commit is contained in:
@@ -82,6 +82,9 @@ declaration; absent and null fields remain distinguishable from explicit
|
||||
strings, including empty strings. It closes content-file handles before
|
||||
returning and retains no source capability for public inspection.
|
||||
|
||||
Prompt materialization shares a lazy metadata snapshot across root and collection
|
||||
resources during construction, avoiding a full metadata copy per collection.
|
||||
|
||||
Role and cache-control normalization is applied to execution-owned message
|
||||
copies. ConfigKit v0.8.0 retains effective input message metadata and its
|
||||
resource origins; recording a sequence-wide normalization in the retained
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
# Configuration Collections Implementation Review Findings
|
||||
|
||||
Status: three findings open. Review completed 2026-10-17.
|
||||
Status: Finding 1 completed; Findings 2–3 open. Review completed 2026-10-17.
|
||||
|
||||
Reviewed implementation after `89af2a1` through `0221cd4` against the accepted
|
||||
[feature roadmap](configuration-collections.md), [implementation plan](implementation.md),
|
||||
@@ -35,7 +35,7 @@ addresses a retained public guarantee that no longer describes scoped views.
|
||||
|
||||
## Finding 1: Share Origin Metadata Across Collection Materialization
|
||||
|
||||
Priority: P2. Status: open.
|
||||
Priority: P2. Status: completed 2026-10-17.
|
||||
|
||||
Locations: [load.go](../../internal/configuration/load.go), lines 257–262, and
|
||||
[materializePrompts](../../internal/configuration/prompts.go), lines 84–95.
|
||||
@@ -78,6 +78,15 @@ one file-backed collection, and use a small scaling benchmark to verify the
|
||||
repeated full-snapshot cost is removed. Do not make exact allocations, elapsed
|
||||
times, or private helper call order a new contract.
|
||||
|
||||
Completion: prompt materialization now traverses root and collection resources
|
||||
within one construction-scoped pass sharing the lazy origin metadata snapshot.
|
||||
Extended the imported/inherited-content test across distinct collection resource
|
||||
contexts and added a scaling benchmark without allocation or timing assertions.
|
||||
At 64 collections the same benchmark dropped from 104,574,200 to 21,239,712
|
||||
allocated bytes per construction in single-iteration local measurements.
|
||||
Validation passed: focused origin tests and benchmark, race-enabled configuration
|
||||
and public API tests, configuration vet, and `git diff --check`.
|
||||
|
||||
## Finding 2: Detach The Optional Label In Internal Prepared Snapshots
|
||||
|
||||
Priority: P3. Status: open.
|
||||
|
||||
@@ -126,6 +126,40 @@ func layeredDefinitions(count int) configurationShape {
|
||||
}
|
||||
}
|
||||
|
||||
func BenchmarkCollectionFileBackedConstruction(b *testing.B) {
|
||||
for _, count := range []int{4, 16, 64} {
|
||||
b.Run(fmt.Sprintf("collections_%d", count), func(b *testing.B) {
|
||||
collections := make(map[string]any, count)
|
||||
for index := range count {
|
||||
collections[fmt.Sprintf("collection-%03d", index)] = map[string]any{
|
||||
"prompts": map[string]any{
|
||||
"example": map[string]any{
|
||||
"messages": []any{map[string]any{"role": "user", "content_file": "body.md"}},
|
||||
"output": map[string]any{"format": "text", "validation_mode": "none"},
|
||||
},
|
||||
},
|
||||
}
|
||||
}
|
||||
namespace := configkit.NewFilesystem("collection benchmark", fstest.MapFS{
|
||||
"body.md": {Data: []byte("Hello")},
|
||||
})
|
||||
main := configkit.Input{
|
||||
Source: configkit.Mapping("collections", map[string]any{"collections": collections}),
|
||||
Filesystem: &configkit.FilesystemContext{FS: &namespace, Root: ".", BaseDir: "."},
|
||||
}
|
||||
b.ReportAllocs()
|
||||
for b.Loop() {
|
||||
result, err := load(main, nil)
|
||||
if err != nil {
|
||||
b.Fatal(err)
|
||||
}
|
||||
constructionResult = result
|
||||
}
|
||||
b.ReportMetric(float64(count), "collections/op")
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func BenchmarkConfigurationConstruction(b *testing.B) {
|
||||
for _, scenario := range []struct {
|
||||
name string
|
||||
|
||||
@@ -255,10 +255,8 @@ func load(main configkit.Input, underlays []configkit.Underlay) (*loaded, error)
|
||||
return nil, fmt.Errorf("load configuration: %w", err)
|
||||
}
|
||||
state.result = result
|
||||
for _, resources := range resourceSets(&result.Config) {
|
||||
if err := state.materializePrompts(resources.prompts, resources.bases.prompts, resources.namespace); err != nil {
|
||||
return nil, fmt.Errorf("load configuration: %w", err)
|
||||
}
|
||||
if err := state.materializePrompts(); err != nil {
|
||||
return nil, fmt.Errorf("load configuration: %w", err)
|
||||
}
|
||||
if err := state.buildExecutionProfiles(); err != nil {
|
||||
return nil, fmt.Errorf("load configuration: %w", err)
|
||||
|
||||
@@ -81,10 +81,8 @@ func resolvePrompts(prompts map[string]promptConfig, basePath configkit.Path, re
|
||||
}
|
||||
}
|
||||
|
||||
func (state *loaded) materializePrompts(prompts map[string]promptConfig, basePath configkit.Path, namespace string) error {
|
||||
if state.prompts == nil {
|
||||
state.prompts = make(map[string]*promptEntry)
|
||||
}
|
||||
func (state *loaded) materializePrompts() error {
|
||||
state.prompts = make(map[string]*promptEntry)
|
||||
var metadata *configkit.Metadata
|
||||
lookupOrigin := func(path configkit.Path) (configkit.FieldInfo, bool) {
|
||||
if metadata == nil {
|
||||
@@ -93,17 +91,20 @@ func (state *loaded) materializePrompts(prompts map[string]promptConfig, basePat
|
||||
}
|
||||
return metadata.Lookup(path)
|
||||
}
|
||||
for _, id := range sortedKeys(prompts) {
|
||||
promptBase := basePath.Key(id)
|
||||
if strings.TrimSpace(id) == "" {
|
||||
return fmt.Errorf("%s: prompt ID must not be blank", promptBase)
|
||||
for _, resources := range resourceSets(&state.result.Config) {
|
||||
for _, id := range sortedKeys(resources.prompts) {
|
||||
promptBase := resources.bases.prompts.Key(id)
|
||||
if strings.TrimSpace(id) == "" {
|
||||
return fmt.Errorf("%s: prompt ID must not be blank", promptBase)
|
||||
}
|
||||
fullID := indexedResourceID(resources.namespace, id)
|
||||
raw := resources.prompts[id]
|
||||
entry, err := state.materializePrompt(fullID, raw.Version, raw, promptBase, lookupOrigin)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
state.prompts[fullID] = entry
|
||||
}
|
||||
fullID := indexedResourceID(namespace, id)
|
||||
entry, err := state.materializePrompt(fullID, prompts[id].Version, prompts[id], promptBase, lookupOrigin)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
state.prompts[fullID] = entry
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -16,9 +16,11 @@ import (
|
||||
|
||||
func TestLoadCapturesInheritedImportedContent(t *testing.T) {
|
||||
files := fstest.MapFS{
|
||||
"app/main.yaml": {Data: []byte("configkit:\n imports:\n - path: ../catalog/base.yaml\ncollections:\n docs:\n prompts:\n child:\n extends: base\n")},
|
||||
"catalog/base.yaml": {Data: []byte("collections:\n docs:\n release_version: r1\n prompts:\n base:\n version: \"v1\"\n messages:\n - role: ' USER '\n cache_control:\n type: ' ephemeral '\n ttl: ' 1h '\n content_file: ./body.md\n output:\n format: text\n validation_mode: basic\n")},
|
||||
"catalog/body.md": {Data: []byte("Captured {{.name}}")},
|
||||
"app/main.yaml": {Data: []byte("configkit:\n imports:\n - path: ../catalog/base.yaml\n - path: ../other/definition.yaml\ncollections:\n docs:\n prompts:\n child:\n extends: base\n")},
|
||||
"catalog/base.yaml": {Data: []byte("collections:\n docs:\n release_version: r1\n prompts:\n base:\n version: \"v1\"\n messages:\n - role: ' USER '\n cache_control:\n type: ' ephemeral '\n ttl: ' 1h '\n content_file: ./body.md\n output:\n format: text\n validation_mode: basic\n")},
|
||||
"catalog/body.md": {Data: []byte("Captured {{.name}}")},
|
||||
"other/definition.yaml": {Data: []byte("collections:\n other:\n prompts:\n child:\n messages:\n - role: user\n content_file: ./body.md\n output:\n format: text\n validation_mode: none\n")},
|
||||
"other/body.md": {Data: []byte("Other collection {{.name}}")},
|
||||
}
|
||||
namespace := configkit.NewFilesystem("embedded", files)
|
||||
state, err := load(configkit.Input{
|
||||
@@ -56,7 +58,12 @@ func TestLoadCapturesInheritedImportedContent(t *testing.T) {
|
||||
if originPath != "catalog/base.yaml" {
|
||||
t.Fatalf("content origin = %q, want catalog/base.yaml", originPath)
|
||||
}
|
||||
other, otherInspection, err := state.prompt("other/child")
|
||||
if err != nil || other.Templates[0].Content != "Other collection {{.name}}" {
|
||||
t.Fatalf("other collection did not retain its source context: %+v, %v", other, err)
|
||||
}
|
||||
files["catalog/body.md"].Data = []byte("Changed")
|
||||
files["other/body.md"].Data = []byte("Changed other collection")
|
||||
again, againInspection, err := state.prompt("docs/child")
|
||||
if err != nil || again.Templates[0].Content != "Captured {{.name}}" || againInspection.PromptHash != inspection.PromptHash {
|
||||
t.Fatalf("captured definition changed after source mutation: %+v, %+v, %v", again, againInspection, err)
|
||||
@@ -66,6 +73,10 @@ func TestLoadCapturesInheritedImportedContent(t *testing.T) {
|
||||
if err != nil || third.Templates[0].Content != "Captured {{.name}}" {
|
||||
t.Fatalf("caller mutation escaped into index: %+v, %v", third, err)
|
||||
}
|
||||
otherAgain, otherAgainInspection, err := state.prompt("other/child")
|
||||
if err != nil || otherAgain.Templates[0].Content != "Other collection {{.name}}" || otherAgainInspection.PromptHash != otherInspection.PromptHash {
|
||||
t.Fatalf("other captured definition changed after source mutation: %+v, %+v, %v", otherAgain, otherAgainInspection, err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestLoadUsesExplicitMappingContextAndClosesContent(t *testing.T) {
|
||||
|
||||
Reference in New Issue
Block a user