test: rank individual and ensemble candidates with synthetic workload costs - #742
Conversation
milindsrivastava1997
left a comment
milindsrivastava1997
left a comment
There was a problem hiding this comment.
Automated code review: 5 findings, posted inline.
| if err := l.runCompose(ctx, environment, "wait", "planner"); err != nil { | ||
| return fmt.Errorf("derive workload cost evidence: %w", err) | ||
| } | ||
| if err := l.runCompose(ctx, environment, "up", "-d", "data-plane"); err != nil { |
There was a problem hiding this comment.
Stale backend image (high). prometheus/planner get --build above, but data-plane is brought up without it, so Compose reuses the existing asapquery-backend-promql-compliance-data-plane image. After the first make run-all, subsequent runs on the same machine test the old data_plane binary (e.g. edit data_plane/src/..., rerun, and results still reflect the first build). Suggest adding --build here or building all three services in the first up.
| c := caseCard{Dataset: r.Dataset, Suite: r.Suite, Passed: r.Passed, Queries: len(r.Queries)} | ||
| for _, q := range r.Queries { | ||
| for _, i := range q.Instant { | ||
| if i.Responses.Backend.ServedBy != "" { |
There was a problem hiding this comment.
"Prometheus fallback" column is always 0 (medium). query.go sets ServedBy = "prometheus_fallback" when the provenance header is absent, so ServedBy is never empty for backend responses and this check counts fallback answers as ASAPQuery answers. The loop also only looks at q.Instant, skipping range responses. summary.md/summary.json therefore overstate local coverage.
| func main() { | ||
| dir := flag.String("reports-dir", "/tmp/asapquery-backend-promql-reports", "report directory") | ||
| flag.Parse() | ||
| files, err := filepath.Glob(filepath.Join(*dir, "*.json")) |
There was a problem hiding this comment.
Picks up stale reports (medium). *.json in REPORT_DIR (a fixed /tmp path by default) also matches the summary.json written by the previous run; it decodes as a runner.Report with empty dataset/suite and the previous overall passed, adding a blank row and possibly pinning the result to false. Likewise, a case that fails before writing its report (build error, health timeout) leaves the previous run's <case>.json counted as current. Suggest excluding summary.json and clearing/scoping REPORT_DIR per run.
| # not need credentials for ASAPCollector or Sketchlib while this dependency | ||
| # remains private. | ||
| RUN mkdir -p ASAPQuery-backend/.cargo && \ | ||
| printf '%s\n' \ |
There was a problem hiding this comment.
Overwrites .cargo/config.toml and patches all builds (medium). (Same block in control_plane/Dockerfile.) > replaces the checked-in config, dropping [net] git-fetch-with-cli = true. And since this applies to every image built from these Dockerfiles, not just the compliance suite, asap-precompute-rs/asap_sketchlib now compile from whatever sibling working trees are passed as build contexts rather than the revisions pinned in Cargo.lock, so production images can pick up uncommitted/unpinned code. Suggest appending (>>) and gating the patch behind a build arg used only by the compliance build.
| asap-precompute-rs: ${ASAP_PRECOMPUTE_RS_CONTEXT} | ||
| asap-sketchlib: ${ASAP_SKETCHLIB_CONTEXT} | ||
| asap-gorilla-rust: ${ASAP_GORILLA_RUST_CONTEXT} | ||
| command: ["--profile", "asapquery", "--planning-snapshot", "/config/planning-snapshot.json", "--prometheus-server", "http://prometheus:9090", "--forward-unsupported-queries", "--http-port", "9091", "--output-dir", "/tmp/asap"] |
There was a problem hiding this comment.
Fallback enabled but always fails the suite (low). --forward-unsupported-queries is on, but report.go:104 fails every response served by Prometheus fallback, so any query routed to the exact/fallback path fails rather than being compared. The design doc also says fallback should be disabled, which would make the validate_profile change in data_plane/src/main.rs pointless as written. Either drop the flag or accept fallback responses for those nodes.
1501e44 to
0b40465
Compare
zzylol
changed the title
9c84758 to
00195cb
Compare
zzylol
changed the title
zzylol
changed the title
2 participants
Alignment with updated #737
The complete synthetic-ranking test passes. Besides making each admitted candidate win by changing prices, it rejects otherwise matching quotes from another dataset. Costs remain synthetic; this does not establish production-optimal selection.
Verify Backend candidate ranking independently of real cost calibration or execution correctness.
Before this PR: structural tests also selected winners with fixture costs, while #742 mixed in the differential execution harness.
After this PR: Level 2 uses the same ten-query workload and Planner-exposed candidate inventory as #728. Each unique admissible manifest becomes the cheapest candidate in turn. Assertions cover selection reversal, minimum reported cost, exactly one winner, inventory-order stability, and exclusion of missing/infeasible quotes. No Planner logical winner is forced to construct the comparison.
Validation:
issue754_level2passes across all ten queries; strict target Clippy passes. All quotes are explicitly synthetic and establish ranking logic only. The final diff against #728 contains the ranking integration test; the execution harness is preserved in #775.Current review order: #728 → #742 → #775. Workload + synthetic costs → selected Deployment Plan → data-plane execution is the current milestone. #776/#777/#778/#759 are deferred follow-ups, not prerequisites. Online ERP collection, feedback and runtime replanning are not required.
Query ensemble coverage
The fixtures now include each individual query, the shared-rate ensemble
(temporal-rate, grouped-rate, topk-rate), the shared-quantile ensemble
(temporal-quantile, quantile-ratio), and the full ten-query workload.
Level 2 makes each admitted workload candidate cheapest in turn using complete
synthetic quotes, asserts the exact selected manifest, reverses candidate order,
and rejects missing or infeasible cheapest quotes. The complete ranking test passed
for the ten single-query workloads and all three ensembles.