Skip to content

feat(planner): asap-planner --planner saved writes a sketch-bench eval plan without re-solving - #828

Open
milindsrivastava1997 wants to merge 3 commits into
mainfrom
824-planner-saved
Open

milindsrivastava1997 wants to merge 3 commits into
mainfrom
824-planner-saved

Conversation

@milindsrivastava1997

@milindsrivastava1997 milindsrivastava1997 commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Before this PR

The only way to get streaming and inference configs from an rqe-optimizer plan was --planner milp, which solves the plan itself. The #777 eval's plans were solved in sketch-bench (autosketch_vs_asap, saved in sketch-bench#203), and re-solving gives a different plan: a different solver objective, accuracy source and arrival rates. experiment_run_e2e.py also always runs the planner.

After this PR

B1 + B2: asap-planner --planner saved

asap-planner --planner saved --streaming_engine precompute --data-ingestion-interval-ms 1000 \
  --plan <sketch-bench>/rqe-optimizer/results/autosketch-vs-asap-hydra/plans/synthetic-templatesclassic-shared1.plans.json.gz \
  --workload-table <sketch-bench>/rqe-optimizer/results/autosketch-vs-asap-hydra/inputs/tables/synthetic-templatesclassic-shared1.json \
  --method asap --weights cpu [--sla-ms 1000] --output_dir DIR
  • The table's e2e block (sketch-bench#208) becomes a ControllerConfig: one query group per interval, with metrics hints from its labels. workload_from_config (the facts-free half of build_milp_workload) turns it into Raqes.
  • Each leaf query maps to its plan RQE through e2e.queries[].rqes, and the RQE to its chosen deployment, sketch, params, window and slide. The result goes through deployments_to_planner_output, the existing converter with MilpSolution replaced by (deployments, Raqe → deployment). plan_to_planner_output is now a thin wrapper around it, so --planner milp output is unchanged.
  • Without --sla-ms it picks the cheapest plan, and with it the sla plan at that SLA. A .gz plan file is read directly (flate2).
  • It refuses:
    • roll-ups (deployment_grouping becomes the deployment's grouping, so the shared roll-up check catches it)
    • an RQE the plan doesn't serve, or picks twice
    • a pick that breaks any of sketch-bench's eligibility rules that need no workload facts (window and slide, mergeability, heap size, key tracker), which means a plan from another table
    • a plan deployment whose RQEs would need different sketches
    • one query served by two deployments (the existing check, which also covers the same PromQL at two intervals on different deployments)
    • avg and any other shape the engine can't serve (the existing checks, run before the leaf split)
  • --planner milp and saved share one helper for their flag checks.

B3: controller.prebuilt_output_dir in experiment_run_e2e.py skips controller_service.start. It copies that directory's streaming_config.yaml and inference_config.yaml into a cleared controller output dir, then syncs them. It checks that both files exist at startup. Before copying, it refuses source and output dirs that contain one another, a query of this run the configs don't plan (whole, or as arithmetic over planned leaves), and slides that aren't multiples of this run's scrape interval.

Departures from the #824 plan:

  • B1 passes deployments instead of building a MilpSolution, because PlanCost can't be built honestly from a saved plan.
  • B2 takes --sla-ms instead of --kind, because the file has one SLA plan per SLA.

Blocked: 12 classic queries (#827)

On the real classic base table (ASAP cheapest at CPU weights), 12 of the 54 queries are shapes ASAPQuery can't serve: sum by (job) (rate(...)) and topk(32, sum by (label_0) (...)) over rate and sum_over_time. So --planner saved refuses the table:

Error: cannot optimize leaf 'topk(32, sum by (label_0) (sum_over_time(data[15m])))' ...: no supported query pattern matched

With those 12 removed from a copy of the table, it writes 5 aggregations. Their windows, slides and params match the plan: dd 0.02 1s; dd 0.05 300s/60s sliding; exact-increase 60s; exact-sum 1s and 60s. The other 3 plan deployments serve the removed queries. #827 tracks engine support, and the e2e run waits on it.

Validation

  • cargo test -p asap_planner: all pass, including 12 new tests in saved_plan.rs. They cover:
    • a ratio whose legs share one sliding DDSketch
    • --sla-ms selection, plus no matching plan
    • roll-up refusal
    • one deployment needing two sketches (this test caught leaves that merge into one item, such as p90 and p50, not being checked)
    • an RQE missing from the plan, or picked twice
    • a pick whose window doesn't divide the query's lookback, or that needs a key tracker
    • a gzipped plan file
    • a query naming fewer RQEs than it has leaves
    • one query's leaves on two plan deployments
    • avg refused before the leaf split
  • tests/test_prebuilt_controller_output.py: stale files are cleared; refused are a missing config, nested dirs, an unplanned query and another scrape interval; arithmetic over planned leaves is accepted.
  • cargo clippy --all-targets -D warnings, cargo fmt, black, flake8: clean.
  • The CLI ran on sketch-bench's committed plan file: the refusal above, and the 42-query conversion for all 12 ASAP plans (cpu and fargate weights, cheapest and five SLAs). The new prebuilt checks accept the cpu cheapest output with the 42 queries at a 1 s scrape, and refuse it at 15 s.
  • B3: the copy-and-sync path was run on its own with a local provider, before the review changes. A full e2e run waits on feat(planner): serve the #777 eval's sum-of-rate and nested topk queries #827 and B4.

Part of #824 (B1, B2, B3).

🤖 Generated with Claude Code

…l plan without re-solving

Reads a plan from sketch-bench's autosketch_vs_asap plan file and the eval
table's `e2e` block (PromQL queries, the plan RQEs each reads, metric
labels), and writes streaming and inference YAML through the same
converter as --planner milp. experiment_run_e2e.py gains
controller.prebuilt_output_dir to deploy those configs instead of running
the planner.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@milindsrivastava1997 milindsrivastava1997 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review: 10 findings, posted inline.

pick.deployment_grouping.is_none(),
"RQE {rqe:?} is served by a roll-up, which the engine can't read yet"
);
let deployment = Deployment {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plan pick never checked against the RAQE's window/slide. The synthesized Deployment takes capability, metric, filter and grouping from the RAQE, but sketch, params, window and slide from the plan. Nothing checks pick.window_secs/slide_secs against the RAQE's lookback_ms/interval_ms. If --plan doesn't match --workload-table (wrong table, reused RQE ids like q0/q1, mis-ordered e2e.queries[].rqes), a 5m quantile_over_time leaf can bind to a 1s-window or sum-sketch pick. The planner then writes the wrong config and exits 0.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing: each pick is now refused unless its window divides the RAQE's lookback (the rule sketch-bench uses for merged_shards) and its slide fits the repeat interval.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e7a49eb.

CONTROLLER_LOCAL_OUTPUT_DIR,
node_offset=args.node_offset,
)
if args.controller_prebuilt_output_dir:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prebuilt configs aren't checked against the experiment. The client sends queries from controller_client_config, not the table's e2e block, and data_ingestion_interval_ms, streaming_engine and punting are ignored here. If the planner ran with --data-ingestion-interval-ms 1000 but Prometheus scrapes at 15s, or the query groups differ, the engine gets queries or windows the config doesn't cover. The run then finishes silently with fallback or wrong results.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferring to B4 (#824). B4 builds the client config and scrape interval from the same table e2e block the planner reads, which closes most of this. A full cross-check in the runner isn't worth it on its own.

f"{args.controller_prebuilt_output_dir}"
)
shutil.copytree(
args.controller_prebuilt_output_dir,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

copytree(..., dirs_exist_ok=True): same-path error and stale merge. If prebuilt_output_dir points at <experiment_outputs>/<same name>/controller_output, copy2 raises SameFileError and copytree raises shutil.Error. With different paths, stale files from an earlier run with the same name (e.g. controller.log, other YAML) are kept and rsynced to the remote.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing: a source equal to the destination is refused, and controller_output is cleared before the copy.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e7a49eb.

let mut rqe_of: HashMap<(String, u64), &str> = HashMap::new();
for q in &e2e.queries {
let leaves = decompose_to_leaves(&q.promql);
ensure!(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avg queries hit the leaf-count error, not the AvgQuery refusal. reject_unwritable_queries only runs inside deployments_to_planner_output, after this check. avg_over_time(data[5m]) with one RQE is split into sum and count leaves, so the user sees "has 2 leaves but names 1 RQEs" instead of the intended AvgQuery error.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing: reject_unwritable_queries now runs before the leaf loop.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e7a49eb.

selector.sla_ms,
);
};
let chosen: HashMap<&str, &Chosen> = plan.chosen.iter().map(|c| (c.rqe.as_str(), c)).collect();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A duplicate rqe in chosen silently keeps the last entry. collect() into a HashMap drops earlier picks for the same RQE without warning, so the plan is only partly reproduced. Consider refusing duplicates.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing: duplicate RQEs in chosen are refused. (The committed classic plan files have none.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e7a49eb.

p.method == selector.method
&& p.weights == selector.weights
&& p.kind == kind
&& p.sla_ms == selector.sla_ms

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact f64 equality on sla_ms; cheapest requires a null sla_ms. If sketch-bench writes a non-null sla_ms on cheapest plans, or a value like 999.9999 after unit conversion, --sla-ms 1000 (or no --sla-ms) gets "0 plans match".

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't fix. sketch-bench leaves sla_ms out of cheapest plans and writes the SLA values as integers (100, 300, 1000, 3000, 10000), so a parsed --sla-ms 1000 matches exactly. If it ever drifts, the error is loud ("0 plans match"), not silent.

let deployment = &solution.deployments[planned.deployment].deployment;
for (raqe, &index) in workload.raqes.iter().zip(served_by) {
let deployment = &deployments[index];
if raqe.grouping_labels != deployment.grouping_labels {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This roll-up guard can't fire on the saved path. saved_plan copies grouping_labels from the RAQE into the synthesized Deployment, so roll-up detection there depends only on Chosen.deployment_grouping.is_none(). If sketch-bench changes how it records roll-ups (e.g. always emitting deployment_grouping, or emitting null), neither check catches it and configs are written at the RAQE grouping. Building the Deployment's grouping from the plan's deployment_grouping when present would let this shared check do the work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing: the Deployment's grouping now comes from deployment_grouping when it's present, and the saved-path check is gone, so the shared guard catches roll-ups and Hydra grids.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e7a49eb.

Comment thread asap-planner-rs/src/main.rs Outdated
anyhow::ensure!(
matches!(args.query_language, QueryLanguage::promql)
&& args.prometheus_url.is_none()
&& !args.enable_punting

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flag validation is duplicated. run_saved repeats run_milp's PromQL, hints-only and flag-exclusion checks, plus the MILP-only-flags ensure from main's legacy branch. A new flag now has to be added in three places. A shared helper (e.g. ensure_promql_hint_only(args, name)) plus hoisting the MILP-only ensure to planner != Milp would cover all of them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing: the shared checks now live in one helper.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e7a49eb.

let mut position: HashMap<usize, usize> = HashMap::new();
let mut deployments: Vec<Deployment> = Vec::new();
let mut served_by = Vec::with_capacity(workload.raqes.len());
for (raqe, &item) in workload.raqes.iter().zip(&workload.raqe_items) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit (efficiency): Resolution runs once per RAQE, so a query repeated across N dashboards rebuilds N identical Deployments, each with a cost_entry JSON round-trip. Resolving once per workload.items index and expanding through raqe_items gives the same result with less work.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't fix: the workloads have 54 queries, so the repeated work is negligible.

# Controller configuration
self.controller_punting = cfg.controller.punting
self.controller_auto_discover_labels = cfg.controller.auto_discover_labels
self.controller_prebuilt_output_dir = cfg.controller.get("prebuilt_output_dir")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scope: this PR bundles the Rust --planner saved feature with an unrelated asap-tools e2e-runner change (prebuilt_output_dir), and the runner change has no test. AGENTS.md: "Keep PRs small and single-purpose." Consider splitting it out.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping it bundled: B1 to B3 ship together on purpose (#824), since B3 is only useful with --planner saved. Adding a test for the prebuilt_output_dir checks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e7a49eb.

Check saved picks against the RAQE's window rules, refuse duplicate
picks, reject avg queries before the leaf split, and take a roll-up's
grouping from the plan so the shared guard catches it. Share the flag
checks of --planner milp and saved. Copy only the two prebuilt configs
into a cleared output dir, refusing the dir itself.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@zzylol

zzylol commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

@milindsrivastava1997 I looked at extending --planner saved to sketch-bench's multigroup and all tables (sketch-bench#209). Their multigroup e2e queries are structured (table, value, capability, grouping, window_secs, interval_secs, rqes, no promql), since PromQL can't express pooled p99 or distinct counts of a value field, so ASAPQuery would run them through its SQL frontend. Findings, no changes to this PR for now:

  • SQL needs feat(planner): --planner milp for SQL #838. Building SQL Raqes and writing SQL configs needs feat(planner): --planner milp for SQL #838's MilpConfig::Sql, extract_sql_aqes and SQL output path. Here promql is required (saved_plan.rs:40), workload_from_config is PromQL-only, and deployments_to_planner_output writes table_name: None. I'd add SQL support to saved after feat(planner): --planner milp for SQL #838 merges rather than duplicate it here. The two PRs conflict mechanically in main.rs, milp.rs, milp_output.rs and optimizer/mod.rs.
  • Mixed tables. The engine runs one query language per process (engine_config.rs:140-146), so the all tables (classic PromQL + multigroup SQL) would need two engines or a refusal that says so.
  • Roll-ups. Every multigroup query in the current asap plans is served by a roll-up (52 picks per shared1 plan), so saved refuses those plans anyway until the engine supports roll-ups.
  • Classic tables now convert. sketch-bench is dropping templates 7/9 (sum by (job) (rate(...)), topk(32, sum by (label_0) (rate(...)))) and rendering template 2 as topk(32, sum_over_time(data[w])). label_0 identifies a series, so the sum by (label_0) only dropped job, and the RQE is unchanged. With that edit, all 12 asap plans of classic-shared1 (cpu and fargate × cheapest and SLA 100–10000 ms) convert with the unchanged plan files, into 6–11 aggregations each. Template 2 maps to a CountMinSketchWithHeap (depth 3, width 1024, heap 15360, 180 s window sliding every 60 s) with no grouping/key check firing. Tested on CloudLab.

Related: the e2e wiring (feeder + aligned query client) is #841, stacked on this PR.

@milindsrivastava1997 milindsrivastava1997 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review. Process note: this PR combines two independent objectives, the Rust --planner saved mode (saved_plan.rs/main.rs) and the asap-tools controller.prebuilt_output_dir feature (experiment_run_e2e.py/config.py). AGENTS.md asks for single-purpose PRs, and the planner title scope doesn't cover the asap-tools change. Consider splitting.

pick.deployment,
deployments[at]
);
ensure!(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid plans get refused when the leaves of one item are on different deployments. sketch-bench plans per RQE, but extract_aqes merges, for example, both leaves of quantile_over_time(0.99, x[5m]) / quantile_over_time(0.5, x[5m]) into one OptimizerItem. If the saved plan serves p99 from DD alpha=0.01 and p50 from DD alpha=0.05 (legal in sketch-bench), this bails with "served by two deployments" and the whole table fails, even though the plan could be deployed as two aggregations.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't fix in this PR. It's possible in principle, but it doesn't happen in the plans we run: all 12 ASAP plans in the classic file (cpu and fargate weights, cheapest and all five SLAs) convert without error. It also fails loudly rather than writing wrong configs. Supporting it means splitting optimizer items in the shared converter, so I'll open an issue if a plan ever needs it.

CONTROLLER_LOCAL_OUTPUT_DIR,
node_offset=args.node_offset,
)
if args.controller_prebuilt_output_dir:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prebuilt_output_dir never checks the copied configs against this experiment. Nothing compares inference_config.yaml with this run's query_groups, or the plan's ingestion interval with this run's Prometheus scrape interval. With a plan from another table, or one built with --data-ingestion-interval-ms 1000 for a 15s run, queries silently fall through to the fallback backend. The latency and accuracy numbers then look valid but don't measure the saved plan. Please fail loud on a mismatch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing (partially): at startup, every query in the run's client config must appear in the prebuilt inference_config.yaml, and every slideIntervalMs must be a multiple of the run's scrape interval. That catches both a plan from another table and a 1s plan on a 15s run.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in c9c48e3. Binary queries are planned as their leaves, so a query counts as planned when it is whole in inference_config.yaml or is arithmetic over planned leaves. This is checked against the actual planner output for the classic table.

// sketch-bench's `is_eligible` window rules: a pick that breaks
// them belongs to another workload table.
ensure!(
slide_ms != 0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This eligibility check copies only part of sketch-bench's is_eligible. It leaves out mergeable_across_windows || lookback == window, heap sufficiency (heap_needed(lookback, k)) and the key-tracker rule. A plan from another table with a top-k CMS-heap sized for a shorter lookback passes and gets written with a heap that is too small. Calling rqe_optimizer::candidates::is_eligible on the built Deployment (or at least d.properties() + heap_needed) would keep one copy of the rules, so they can't drift apart.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing (partially). is_eligible needs workload facts and accuracy data, which the saved path doesn't have, so I'm adding the rules that don't need them: mergeable or lookback == window, heap sufficiency through Deployment::heap_needed and heap_capacity, and the key-tracker rule (only Hydra needs a tracker, and Hydra is refused).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in c9c48e3.

raise ValueError(
f"controller.prebuilt_output_dir is the experiment's own output dir: {dest}"
)
shutil.rmtree(dest, ignore_errors=True)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A source inside the destination gets deleted before it is copied. Only src == dest is refused. With prebuilt_output_dir=.../controller_output/saved (a natural staging spot in local mode), rmtree(dest) deletes saved/ and copy2 then raises FileNotFoundError, with the prebuilt configs already gone. Refuse src.is_relative_to(dest) (and probably the reverse too).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing: the copy is now refused when either path is inside the other, not only when they're equal.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in c9c48e3.

)?)
}

fn same_sketch(a: &Deployment, b: &Deployment) -> bool {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: same_sketch compares fields by hand, but rqe_optimizer::Deployment already derives PartialEq. Both sides come from the same template (zeroed cost_entry, key_tracker: None), so deployments[at] == deployment is equivalent and doesn't need updating whenever Deployment gains a field.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing: replaced with ==.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in c9c48e3.

Comment thread asap-planner-rs/src/main.rs Outdated
--planner milp"
);
}
if args.planner != PlannerArg::Legacy {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: wrapping the match in if planner != Legacy forces an unreachable!() arm. A single match args.planner { Milp => ..., Saved => ..., Legacy => {} } drops it and needs one edit, not two, when a planner variant is added.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixing: it's one match now.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in c9c48e3.

}
let config = e2e_config(e2e)?;
// Before the leaf split, which turns an avg into two leaves.
reject_unwritable_queries(&config)?;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: reject_unwritable_queries also runs at the top of deployments_to_planner_output, so it runs twice on the same config. Drop one copy, or note which call is the source of truth. Separately, e2e_config builds ControllerConfig through a json! round trip, which turns type errors into runtime errors; it could build the struct directly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't fix. The early call is deliberate: it reports avg before the leaf split turns it into a leaf-count error. The converter keeps its own call because --planner milp relies on it too. Building ControllerConfig directly would mean naming every field, where the json! route takes serde's defaults.

Ok(serde_json::from_value(config)?)
}

fn read_json<T: serde::de::DeserializeOwned>(path: &Path) -> anyhow::Result<T> {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: read_json holds the compressed bytes, the full decompressed String and the parsed tree in memory at the same time. Peeking at the gzip magic bytes and then using serde_json::from_reader(GzDecoder::new(BufReader::new(file))) streams it instead.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't fix: the plan files are about 28 KB gzipped.

self.controller_punting = cfg.controller.punting
self.controller_auto_discover_labels = cfg.controller.auto_discover_labels
self.controller_prebuilt_output_dir = cfg.controller.get("prebuilt_output_dir")
if self.controller_prebuilt_output_dir:

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No tests for the new failure paths and path resolution. Nothing covers resolving a relative prebuilt_output_dir under Hydra's changed cwd (plus the startup check), reading .plans.json.gz, a mismatch between leaf and RQE counts, or the "served by two deployments" refusal. The Rust tests only write plain JSON, and the Python tests only call copy_prebuilt_controller_output.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly fixing: adding Rust tests for reading a gzipped plan, a leaf/RQE count mismatch and the two-deployments refusal, plus Python tests for the new checks. I'm skipping a test of Hydra resolving a relative path: hydra.utils.to_absolute_path is Hydra's own function, and testing it would need a full Hydra config.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in c9c48e3.

@milindsrivastava1997

Copy link
Copy Markdown
Contributor Author

Re the process note in the review: B1 to B3 are in one PR on purpose, as decided in #824. The asap-tools part (B3) only makes sense together with --planner saved.

Check saved picks against every eligibility rule that needs no workload
facts (mergeability, heap size, key tracker), compare deployments with
PartialEq, and dispatch planners in one match. Before copying prebuilt
configs, refuse nested source and output dirs, queries the configs
don't plan, and slides off the run's scrape interval.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@zzylol

zzylol commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

@milindsrivastava1997 Two engine findings from an end-to-end check (paced feeder → ASAPQuery --planner saved configs vs Prometheus 2.53 and ClickHouse, classic table, CloudLab):

  1. IncreaseAccumulator::update treats an out-of-order sample as a counter reset (asap-query-engine/src/precompute_operators/increase_accumulator.rs:74). The worker processes a series' samples in arrival order. With remote write delivering one series' samples within a second in two concurrent requests, a later sample sometimes arrived first, and rate(data_total[15m]) came out 150–520× too high. Fed in order, it matched ClickHouse to 1e-10. The feeder now keeps each series' samples for a tick in one request (so the e2e runs are fine), but real remote-write senders can reorder within the allowed lateness too. Sorting a series' samples by timestamp within the lateness before updating, or ignoring out-of-order samples for counters, would make it robust.
  2. Sliding and tumbling windows behave differently on partial data. With 25 minutes of data, the 1h/6h/24h quantile_over_time queries (served by a 300 s window sliding every 60 s) returned no answer: sliding windows answer only when every sub-window is present (plan_exact_cover, engines/sliding_window_composition.rs:38). The tumbling aggregations answered from whatever data existed. It's expected that partial data gives partial answers, but the two window types should probably agree, either both answering from what exists or both refusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants