Finish profile comparison follow-up fixes
This commit is contained in:
@@ -30,7 +30,9 @@ construction to the prepared-report flow. It does not accept a notifier.
|
|||||||
Once the destination is resolved, the partial result retains its absolute
|
Once the destination is resolved, the partial result retains its absolute
|
||||||
output directory even when later preflight, debug initialization, inspection,
|
output directory even when later preflight, debug initialization, inspection,
|
||||||
collection, or preparation fails. Every initialized result is finalized with a
|
collection, or preparation fails. Every initialized result is finalized with a
|
||||||
finished timestamp. Artifact paths are added only after publication commits.
|
finished timestamp. If prompt inspection succeeds before a later profile
|
||||||
|
inspection fails, the partial result retains the resolved prompt ID, version,
|
||||||
|
and hash. Artifact paths are added only after publication commits.
|
||||||
|
|
||||||
The comparison execution core starts each inspected profile independently,
|
The comparison execution core starts each inspected profile independently,
|
||||||
keeps results in selection order, and waits for all started work. Independent
|
keeps results in selection order, and waits for all started work. Independent
|
||||||
|
|||||||
@@ -16,7 +16,7 @@ internally coherent.
|
|||||||
|
|
||||||
## Implementation Rules
|
## Implementation Rules
|
||||||
|
|
||||||
Apply these rules in every remaining stage:
|
The following rules governed every implementation stage:
|
||||||
|
|
||||||
- Read `docs/development.md`, the task-specific documents it identifies, all
|
- Read `docs/development.md`, the task-specific documents it identifies, all
|
||||||
files under `docs/policy/`, and the feature roadmap before changing code.
|
files under `docs/policy/`, and the feature roadmap before changing code.
|
||||||
|
|||||||
@@ -65,12 +65,17 @@ type ComparisonProfileResult struct {
|
|||||||
Error *comparison.SafeError
|
Error *comparison.SafeError
|
||||||
}
|
}
|
||||||
|
|
||||||
var publishComparison = comparison.Publish
|
type comparisonPublisher func(context.Context, comparison.DestinationPlan, comparison.LogicalBundle) (comparison.PublicationResult, error)
|
||||||
|
|
||||||
// CompareDetailed assembles, executes, and atomically publishes a comparison
|
// CompareDetailed assembles, executes, and atomically publishes a comparison
|
||||||
// bundle. Profile failures publish a complete partial bundle; all other
|
// bundle. Profile failures publish a complete partial bundle. Failures before
|
||||||
// failures leave the destination untouched.
|
// commit leave the destination untouched; a post-commit cleanup failure leaves
|
||||||
|
// the new bundle installed and returns its artifact paths with an error.
|
||||||
func CompareDetailed(ctx context.Context, req ComparisonRequest) (*ComparisonResult, error) {
|
func CompareDetailed(ctx context.Context, req ComparisonRequest) (*ComparisonResult, error) {
|
||||||
|
return compareDetailed(ctx, req, comparison.Publish)
|
||||||
|
}
|
||||||
|
|
||||||
|
func compareDetailed(ctx context.Context, req ComparisonRequest, publish comparisonPublisher) (*ComparisonResult, error) {
|
||||||
if err := comparison.ValidateProfileIDs(req.ProfileIDs); err != nil {
|
if err := comparison.ValidateProfileIDs(req.ProfileIDs); err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
@@ -117,10 +122,10 @@ func CompareDetailed(ctx context.Context, req ComparisonRequest) (*ComparisonRes
|
|||||||
inspection, err := InspectComparisonExecution(ctx, ComparisonInspectionRequest{
|
inspection, err := InspectComparisonExecution(ctx, ComparisonInspectionRequest{
|
||||||
Resolved: resolved, ProfileIDs: req.ProfileIDs, Executor: req.Executor, LookupEnv: os.LookupEnv,
|
Resolved: resolved, ProfileIDs: req.ProfileIDs, Executor: req.Executor, LookupEnv: os.LookupEnv,
|
||||||
})
|
})
|
||||||
|
result.PromptID, result.PromptVersion, result.PromptHash = inspection.PromptID, inspection.PromptVersion, inspection.PromptHash
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return result, err
|
return result, err
|
||||||
}
|
}
|
||||||
result.PromptID, result.PromptVersion, result.PromptHash = inspection.PromptID, inspection.PromptVersion, inspection.PromptHash
|
|
||||||
|
|
||||||
collection, err := collectWeather(ctx, req.Config, req.Collector)
|
collection, err := collectWeather(ctx, req.Config, req.Collector)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -148,7 +153,7 @@ func CompareDetailed(ctx context.Context, req ComparisonRequest) (*ComparisonRes
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return result, fmt.Errorf("re-preflight comparison destination: %w", err)
|
return result, fmt.Errorf("re-preflight comparison destination: %w", err)
|
||||||
}
|
}
|
||||||
publication, err := publishComparison(ctx, publicationPlan, bundle)
|
publication, err := publish(ctx, publicationPlan, bundle)
|
||||||
if publication.Committed {
|
if publication.Committed {
|
||||||
result.OutputDirectory = publicationPlan.Target
|
result.OutputDirectory = publicationPlan.Target
|
||||||
result.ManifestPath = filepath.Join(publicationPlan.Target, comparison.ManifestFilename)
|
result.ManifestPath = filepath.Join(publicationPlan.Target, comparison.ManifestFilename)
|
||||||
|
|||||||
@@ -90,18 +90,16 @@ func TestCompareDetailedRetainsCommittedPathsWhenBackupCleanupFails(t *testing.T
|
|||||||
bundle := generationBundle(t)
|
bundle := generationBundle(t)
|
||||||
backupPath := filepath.Join(t.TempDir(), ".comparison-daily.backup-retained")
|
backupPath := filepath.Join(t.TempDir(), ".comparison-daily.backup-retained")
|
||||||
cleanupCause := errors.New("backup cleanup failed")
|
cleanupCause := errors.New("backup cleanup failed")
|
||||||
originalPublish := publishComparison
|
publish := func(context.Context, comparison.DestinationPlan, comparison.LogicalBundle) (comparison.PublicationResult, error) {
|
||||||
publishComparison = func(context.Context, comparison.DestinationPlan, comparison.LogicalBundle) (comparison.PublicationResult, error) {
|
|
||||||
return comparison.PublicationResult{Committed: true, RetainedBackupPath: backupPath}, &comparison.PublicationCleanupError{RetainedBackupPath: backupPath, Err: cleanupCause}
|
return comparison.PublicationResult{Committed: true, RetainedBackupPath: backupPath}, &comparison.PublicationCleanupError{RetainedBackupPath: backupPath, Err: cleanupCause}
|
||||||
}
|
}
|
||||||
t.Cleanup(func() { publishComparison = originalPublish })
|
|
||||||
|
|
||||||
result, err := CompareDetailed(context.Background(), ComparisonRequest{
|
result, err := compareDetailed(context.Background(), ComparisonRequest{
|
||||||
Config: comparisonConfig(), Report: ReportDaily, ProfileIDs: []string{"weather-light", "weather-deep"},
|
Config: comparisonConfig(), Report: ReportDaily, ProfileIDs: []string{"weather-light", "weather-deep"},
|
||||||
WorkingDir: t.TempDir(), Date: generationTime("2026-05-29T12:00:00-05:00"),
|
WorkingDir: t.TempDir(), Date: generationTime("2026-05-29T12:00:00-05:00"),
|
||||||
Clock: timeutil.FixedClock{Time: generationTime("2026-05-29T08:30:00-05:00")},
|
Clock: timeutil.FixedClock{Time: generationTime("2026-05-29T08:30:00-05:00")},
|
||||||
Collector: &generationCollector{bundle: &bundle}, Executor: &generationExecutor{},
|
Collector: &generationCollector{bundle: &bundle}, Executor: &generationExecutor{},
|
||||||
})
|
}, publish)
|
||||||
var cleanupErr *comparison.PublicationCleanupError
|
var cleanupErr *comparison.PublicationCleanupError
|
||||||
if result == nil || !errors.As(err, &cleanupErr) || !errors.Is(err, cleanupCause) || cleanupErr.RetainedBackupPath != backupPath || !filepath.IsAbs(result.ManifestPath) || !filepath.IsAbs(result.DataPackagePath) {
|
if result == nil || !errors.As(err, &cleanupErr) || !errors.Is(err, cleanupCause) || cleanupErr.RetainedBackupPath != backupPath || !filepath.IsAbs(result.ManifestPath) || !filepath.IsAbs(result.DataPackagePath) {
|
||||||
t.Fatalf("CompareDetailed() result/error = %#v/%v", result, err)
|
t.Fatalf("CompareDetailed() result/error = %#v/%v", result, err)
|
||||||
@@ -139,6 +137,7 @@ func TestCompareDetailedFinalizesUnpublishedFailures(t *testing.T) {
|
|||||||
executor *generationExecutor
|
executor *generationExecutor
|
||||||
collector *generationCollector
|
collector *generationCollector
|
||||||
wantPrompt bool
|
wantPrompt bool
|
||||||
|
wantCollection bool
|
||||||
}{
|
}{
|
||||||
{
|
{
|
||||||
name: "destination preflight",
|
name: "destination preflight",
|
||||||
@@ -162,17 +161,27 @@ func TestCompareDetailedFinalizesUnpublishedFailures(t *testing.T) {
|
|||||||
collector: &generationCollector{},
|
collector: &generationCollector{},
|
||||||
wantPrompt: false,
|
wantPrompt: false,
|
||||||
},
|
},
|
||||||
|
{
|
||||||
|
name: "profile preflight",
|
||||||
|
executor: &generationExecutor{profileInspectErrors: map[string]error{
|
||||||
|
"weather-deep": promptexec.NewError(promptexec.MissingCredential, "profile credential is unavailable", errors.New("unsafe cause")),
|
||||||
|
}},
|
||||||
|
collector: &generationCollector{},
|
||||||
|
wantPrompt: true,
|
||||||
|
},
|
||||||
{
|
{
|
||||||
name: "collection",
|
name: "collection",
|
||||||
executor: &generationExecutor{},
|
executor: &generationExecutor{},
|
||||||
collector: &generationCollector{err: errors.New("collection failed")},
|
collector: &generationCollector{err: errors.New("collection failed")},
|
||||||
wantPrompt: true,
|
wantPrompt: true,
|
||||||
|
wantCollection: true,
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "preparation",
|
name: "preparation",
|
||||||
executor: &generationExecutor{},
|
executor: &generationExecutor{},
|
||||||
collector: &generationCollector{bundle: &weatherdata.Bundle{}},
|
collector: &generationCollector{bundle: &weatherdata.Bundle{}},
|
||||||
wantPrompt: true,
|
wantPrompt: true,
|
||||||
|
wantCollection: true,
|
||||||
},
|
},
|
||||||
} {
|
} {
|
||||||
t.Run(test.name, func(t *testing.T) {
|
t.Run(test.name, func(t *testing.T) {
|
||||||
@@ -199,6 +208,9 @@ func TestCompareDetailedFinalizesUnpublishedFailures(t *testing.T) {
|
|||||||
if (result.PromptID != "") != test.wantPrompt || (result.PromptHash != "") != test.wantPrompt {
|
if (result.PromptID != "") != test.wantPrompt || (result.PromptHash != "") != test.wantPrompt {
|
||||||
t.Fatalf("prompt identity = %q/%q, want resolved=%t", result.PromptID, result.PromptHash, test.wantPrompt)
|
t.Fatalf("prompt identity = %q/%q, want resolved=%t", result.PromptID, result.PromptHash, test.wantPrompt)
|
||||||
}
|
}
|
||||||
|
if collector.called != test.wantCollection || test.executor.executeCalls != 0 {
|
||||||
|
t.Fatalf("collection/execution = %t/%d, want collection=%t and no execution", collector.called, test.executor.executeCalls, test.wantCollection)
|
||||||
|
}
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -40,6 +40,7 @@ type generationExecutor struct {
|
|||||||
promptInspections int
|
promptInspections int
|
||||||
profileInspections int
|
profileInspections int
|
||||||
inspectErr error
|
inspectErr error
|
||||||
|
profileInspectErrors map[string]error
|
||||||
executeErr error
|
executeErr error
|
||||||
executeErrors map[string]error
|
executeErrors map[string]error
|
||||||
beforeExecute func(promptexec.ExecuteRequest)
|
beforeExecute func(promptexec.ExecuteRequest)
|
||||||
@@ -65,6 +66,9 @@ func (e *generationExecutor) InspectProfile(_ context.Context, id string) (promp
|
|||||||
generationExecutorMu.Lock()
|
generationExecutorMu.Lock()
|
||||||
defer generationExecutorMu.Unlock()
|
defer generationExecutorMu.Unlock()
|
||||||
e.profileInspections++
|
e.profileInspections++
|
||||||
|
if err := e.profileInspectErrors[id]; err != nil {
|
||||||
|
return promptexec.ProfileInspection{}, err
|
||||||
|
}
|
||||||
return promptexec.ProfileInspection{ProfileID: id, BackendID: "fixture", ModelName: "fixture-model"}, nil
|
return promptexec.ProfileInspection{ProfileID: id, BackendID: "fixture", ModelName: "fixture-model"}, nil
|
||||||
}
|
}
|
||||||
func (e *generationExecutor) Execute(_ context.Context, req promptexec.ExecuteRequest, callback promptexec.PreparationCallback) (*promptexec.Execution, error) {
|
func (e *generationExecutor) Execute(_ context.Context, req promptexec.ExecuteRequest, callback promptexec.PreparationCallback) (*promptexec.Execution, error) {
|
||||||
|
|||||||
@@ -120,7 +120,8 @@ func InspectPromptExecutions(ctx context.Context, req PromptExecutionsInspection
|
|||||||
|
|
||||||
// InspectComparisonExecution validates one exact prompt and every explicitly
|
// InspectComparisonExecution validates one exact prompt and every explicitly
|
||||||
// requested profile before collection or model execution. Profiles are
|
// requested profile before collection or model execution. Profiles are
|
||||||
// inspected sequentially in request order.
|
// inspected sequentially in request order. If a profile fails, the returned
|
||||||
|
// partial result retains the prompt identity and successfully inspected prefix.
|
||||||
func InspectComparisonExecution(ctx context.Context, req ComparisonInspectionRequest) (ComparisonInspectionResult, error) {
|
func InspectComparisonExecution(ctx context.Context, req ComparisonInspectionRequest) (ComparisonInspectionResult, error) {
|
||||||
if err := comparison.ValidateProfileIDs(req.ProfileIDs); err != nil {
|
if err := comparison.ValidateProfileIDs(req.ProfileIDs); err != nil {
|
||||||
return ComparisonInspectionResult{}, promptexec.NewError(promptexec.InvalidRequest, "comparison profile selection is invalid", err)
|
return ComparisonInspectionResult{}, promptexec.NewError(promptexec.InvalidRequest, "comparison profile selection is invalid", err)
|
||||||
@@ -142,7 +143,7 @@ func InspectComparisonExecution(ctx context.Context, req ComparisonInspectionReq
|
|||||||
for _, profileID := range req.ProfileIDs {
|
for _, profileID := range req.ProfileIDs {
|
||||||
profile, err := inspectPromptProfile(ctx, req.Executor, profileID, req.LookupEnv)
|
profile, err := inspectPromptProfile(ctx, req.Executor, profileID, req.LookupEnv)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return ComparisonInspectionResult{}, comparisonInspectionError("comparison profile inspection failed", err)
|
return result, comparisonInspectionError("comparison profile inspection failed", err)
|
||||||
}
|
}
|
||||||
result.Profiles = append(result.Profiles, ComparisonProfileInspection{
|
result.Profiles = append(result.Profiles, ComparisonProfileInspection{
|
||||||
ProfileID: profile.ProfileID,
|
ProfileID: profile.ProfileID,
|
||||||
|
|||||||
@@ -195,7 +195,7 @@ func TestInspectComparisonExecutionStopsAtFirstProfileFailure(t *testing.T) {
|
|||||||
"weather-deep": {ProfileID: "weather-deep", BackendID: "cloud", ModelName: "deep-model"},
|
"weather-deep": {ProfileID: "weather-deep", BackendID: "cloud", ModelName: "deep-model"},
|
||||||
},
|
},
|
||||||
}
|
}
|
||||||
_, err := InspectComparisonExecution(context.Background(), ComparisonInspectionRequest{
|
result, err := InspectComparisonExecution(context.Background(), ComparisonInspectionRequest{
|
||||||
Resolved: resolved, ProfileIDs: []string{"weather-light", "missing-key", "weather-deep"}, Executor: executor,
|
Resolved: resolved, ProfileIDs: []string{"weather-light", "missing-key", "weather-deep"}, Executor: executor,
|
||||||
LookupEnv: func(string) (string, bool) { return "", false },
|
LookupEnv: func(string) (string, bool) { return "", false },
|
||||||
})
|
})
|
||||||
@@ -205,6 +205,9 @@ func TestInspectComparisonExecutionStopsAtFirstProfileFailure(t *testing.T) {
|
|||||||
if !reflect.DeepEqual(executor.profileRequests, []string{"weather-light", "missing-key"}) || len(executor.promptRequests) != 1 || executor.executeRequests != 0 {
|
if !reflect.DeepEqual(executor.profileRequests, []string{"weather-light", "missing-key"}) || len(executor.promptRequests) != 1 || executor.executeRequests != 0 {
|
||||||
t.Fatalf("prompt/profile/execute requests = %#v/%#v/%d", executor.promptRequests, executor.profileRequests, executor.executeRequests)
|
t.Fatalf("prompt/profile/execute requests = %#v/%#v/%d", executor.promptRequests, executor.profileRequests, executor.executeRequests)
|
||||||
}
|
}
|
||||||
|
if result.PromptID != resolved.Definition.PromptID || result.PromptVersion != resolved.Definition.PromptVersion || result.PromptHash == "" || len(result.Profiles) != 1 || result.Profiles[0].ProfileID != "weather-light" {
|
||||||
|
t.Fatalf("partial inspection result = %#v", result)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestInspectComparisonExecutionStopsBeforeProfileInspectionWhenPromptFails(t *testing.T) {
|
func TestInspectComparisonExecutionStopsBeforeProfileInspectionWhenPromptFails(t *testing.T) {
|
||||||
|
|||||||
Reference in New Issue
Block a user