Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

145 changes: 127 additions & 18 deletions asap-planner-rs/src/bin/optimizer_cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,12 @@
use std::path::PathBuf;

use asap_planner::optimizer::{
load_optional_selected_atomic_cost_table, run_greedy_pipeline, AtomicCostTable, LabelSetFacts,
build_milp_workload, load_optional_selected_atomic_cost_table, load_workload_facts,
run_greedy_pipeline, solve_milp, AtomicCostTable, LabelSetFacts,
};
use asap_planner::ControllerConfig;
use clap::Parser;
use rqe_optimizer::milp::Objective;

#[derive(Parser, Debug)]
#[command(
Expand All @@ -27,26 +29,53 @@ struct Args {
#[arg(long = "data-ingestion-interval-ms", value_parser = clap::value_parser!(u64).range(1..))]
data_ingestion_interval_ms: u64,

/// YAML label-set facts: `series_count` per (metric, spatial filter) and
/// `cardinality` per (metric, spatial filter, grouping labels).
#[arg(long = "label-set-facts")]
label_set_facts: PathBuf,

/// Path to the versioned atomic-cost document sketch-bench's `atomic-costs`
/// subcommand exports. Requires --atomic-cost-workload to select exactly
/// one measured workload profile. Omitted: every
/// benchmarked-family candidate (CMS/HLL/KLL) is dropped, since there is
/// no data to cost it at — only trivial accumulators and EXACT remain
/// selectable.
#[arg(long = "atomic-costs")]
/// Greedy only. YAML label-set facts: `series_count` per (metric, spatial
/// filter) and `cardinality` per (metric, spatial filter, grouping labels).
#[arg(long = "label-set-facts", required_unless_present = "milp")]

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.

--label-set-facts is silently ignored with --milp. Clap accepts the flag but the file is never read in MILP mode. Add conflicts_with = "milp", as --atomic-cost-workload already has.

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.

Fixed in 5561781. --label-set-facts now has conflicts_with = "milp".

label_set_facts: Option<PathBuf>,

/// Greedy: the versioned atomic-cost document sketch-bench's
/// `atomic-costs` subcommand exports; requires --atomic-cost-workload.
/// Omitted: every benchmarked-family candidate (CMS/HLL/KLL) is dropped,
/// leaving only trivial accumulators and EXACT.
/// MILP: the flat cost table `export_rqe_optimizer_costs.sh` writes
/// (`rqe_atomic_costs.json`); required.
#[arg(long = "atomic-costs", required_if_eq("milp", "true"))]
atomic_costs: Option<PathBuf>,

/// JSON `profiles[].workload` value copied from the sketch-bench atomic-cost
/// document. This makes the empirical workload profile explicit and avoids
/// mixing costs from different traces or time windows.
#[arg(long = "atomic-cost-workload", requires = "atomic_costs")]
/// Greedy only. JSON `profiles[].workload` value copied from the

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: --label-set-facts has no conflicts_with = "milp", while --atomic-cost-workload does, so it is silently ignored under --milp.

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.

Fixed in 5561781. --label-set-facts now has conflicts_with = "milp".

/// sketch-bench atomic-cost document. This makes the empirical workload
/// profile explicit and avoids mixing costs from different traces or time
/// windows.
#[arg(
long = "atomic-cost-workload",
requires = "atomic_costs",
conflicts_with = "milp"
)]
atomic_cost_workload: Option<PathBuf>,

/// Plan with sketch-bench's rqe-optimizer MILP and print the plan; writes
/// no configs yet.
#[arg(long)]
milp: bool,

/// MILP only. YAML workload facts: per metric, `cardinality` per label
/// set, including the set of all its labels (the series count).
#[arg(
long = "workload-facts",
required_if_eq("milp", "true"),
requires = "milp"
)]
workload_facts: Option<PathBuf>,

/// MILP only. Objective weight on CPU-sec/sec. Default: rqe-optimizer's.
#[arg(long = "w-cpu", requires = "milp")]
w_cpu: Option<f64>,

/// MILP only. Objective weight on memory GiB. Default: rqe-optimizer's.
#[arg(long = "w-mem", requires = "milp")]
w_mem: Option<f64>,

#[arg(short, long, action = clap::ArgAction::Count)]
verbose: u8,
}
Expand All @@ -64,7 +93,14 @@ fn main() -> anyhow::Result<()> {

let yaml_str = std::fs::read_to_string(&args.input_config)?;
let config: ControllerConfig = serde_yaml::from_str(&yaml_str)?;
let facts = LabelSetFacts::from_path(&args.label_set_facts)?;
if args.milp {
return run_milp(&args, &config);
}
let facts = LabelSetFacts::from_path(
args.label_set_facts
.as_deref()
.expect("clap requires --label-set-facts without --milp"),
)?;

let atomic_cost_table = match load_optional_selected_atomic_cost_table(
args.atomic_costs.as_deref(),
Expand Down Expand Up @@ -108,3 +144,76 @@ fn main() -> anyhow::Result<()> {

Ok(())
}

fn run_milp(args: &Args, config: &ControllerConfig) -> anyhow::Result<()> {

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: workload facts load against config.metrics.unwrap_or_default() before the MissingMetricHints check runs. A config with no metrics: section fails with "metric X has facts but no metrics: hint" instead of the intended MissingMetricHints 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.

Fixed in 5561781. A missing metrics: section now returns MissingMetricHints before facts are loaded, and the MILP path calls config.warn_default_slas().

let hints = config.metrics.as_deref().unwrap_or_default();

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.

Missing metrics: hints are silently defaulted. config.metrics.as_deref().unwrap_or_default() turns missing hints into an empty slice, which hides the intended MissingMetricHints error (code-design-review §1).

Failure: a config with no metrics: block later fails with metric "..." has facts but no metrics: hint. That points the user at the facts file instead of the config.

Separately, the MILP path never calls config.warn_default_slas(). A group without controller_options gets accuracy_sla = 0.0 (max error 1.0, or precision_at_k >= 0 for TopK), so the coarsest sketch qualifies with no warning.

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.

Fixed in 5561781. A missing metrics: section now returns MissingMetricHints before facts are loaded, and the MILP path calls config.warn_default_slas().

let facts = load_workload_facts(
args.workload_facts
.as_deref()
.expect("clap requires --workload-facts with --milp"),
hints,
args.data_ingestion_interval_ms,
)?;
let costs_path = args
.atomic_costs
.as_deref()
.expect("clap requires --atomic-costs with --milp");

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.

The MILP path deserializes the cost table without row validation. The greedy loader drops non-finite or negative rows via valid_cost_entry, and rqe-optimizer doesn't check them either. A NaN or negative cost then reaches minimize, where HiGHS either fails with an opaque error or picks the negative-cost deployment. Suggest reusing valid_cost_entry.

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.

Fixed in 5561781. New load_flat_atomic_cost_table reuses valid_cost_entry and errors on any non-finite or negative row instead of dropping it (flat_loader_rejects_non_finite_or_negative_costs).

let costs: AtomicCostTable = serde_json::from_str(&std::fs::read_to_string(costs_path)?)
.map_err(|e| anyhow::anyhow!("parsing cost table {}: {e}", costs_path.display()))?;
let Objective::AUCCost { w_cpu, w_mem } = Objective::default();

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.

--w-cpu and --w-mem accept any f64. A negative value (for example --w-mem -1) rewards memory or makes the problem unbounded, and NaN makes every objective coefficient NaN. Suggest rejecting values that aren't finite and >= 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.

Fixed in 5561781. --w-cpu/--w-mem go through parse_weight: finite and >= 0 only (weights_must_be_finite_and_non_negative).

let objective = Objective::AUCCost {
w_cpu: args.w_cpu.unwrap_or(w_cpu),
w_mem: args.w_mem.unwrap_or(w_mem),
};
tracing::debug!(?objective, cost_rows = costs.len(), "milp: inputs loaded");

let workload = build_milp_workload(config, &facts, args.data_ingestion_interval_ms)?;
let (deployments, solution) = solve_milp(&workload, &facts, &costs, objective)?;

let mut active: Vec<usize> = solution.mapping.clone();
active.sort();
active.dedup();
println!("=== Deployments: {} ===", active.len());
for &d in &active {
let dep = &deployments[d];
println!(
" [{d}] {:?} {} config={} metric={} grouping={:?} window={}ms slide={}ms",
dep.capability,
dep.config.sketch,
dep.config.sketch_config,
dep.metric,
dep.grouping_labels,
dep.window_ms,
dep.slide_ms,
);
}
println!("\n=== Raqes: {} ===", workload.raqes.len());
for ((raqe, &d), latency_ms) in workload
.raqes
.iter()
.zip(&solution.mapping)
.zip(&solution.plan_cost.query_latency_ms)
{
println!(" {} -> [{d}] latency={latency_ms:.3e}ms", raqe.id);
}
let cost = &solution.plan_cost;
println!(
"\nobjective={:.6e} cpu={:.6e} cpu-sec/sec memory={:.3} MB",
objective.value(cost),
cost.cpu_secs_per_sec(),
cost.memory_bytes() / 1e6,
);
for (phase, c) in [
("ingest", &cost.ingest),
("merge", &cost.merge),
("query", &cost.query),
("storage", &cost.storage),
] {
println!(
" {phase}: cpu={:.6e} cpu-sec/sec memory={:.3} MB",
c.cpu_secs_per_sec,
c.memory_bytes / 1e6
);
}
Ok(())
}
53 changes: 11 additions & 42 deletions asap-planner-rs/src/optimizer/atomic_costs.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,8 @@
//! The atomic-cost table exported by sketch-bench (sketch-bench#30,
//! `scripts/export_atomic_costs.sh`), and the (sketch_type, params) lookup
//! that resolves a candidate's [`AtomicCosts`] from it.
//!
//! `AtomicCostEntry`/`AtomicCostTable` are a deliberate duplicate of
//! sketch-bench's `aqpbm_core::atomic_costs` types, not a shared dependency —
//! see ASAPQuery#524 and sketch-bench#30 for why. Keep the two in sync by
//! hand; `atomic_cost_entry_deserializes_sketch_benchs_documented_shape`
//! below is a canary for drift.

use std::collections::{BTreeMap, HashMap};

use std::collections::HashMap;
use std::path::Path;

use promql_utilities::query_logics::enums::AggregationType;
Expand Down Expand Up @@ -73,25 +67,7 @@ pub struct ExternalWorkload {
pub timestamp_unit: String,
}

#[derive(Debug, Clone, Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct AtomicCostEntry {
pub sketch: String,
pub sketch_config: Value,
pub mem_bytes_per_instance: f64,
pub insert_cpu_secs: f64,
pub merge_cpu_secs: f64,
pub query_cpu_secs: f64,
pub query_accuracy: BTreeMap<String, f64>,
/// The conditions sketch-bench measured the row under (items, keys and
/// value range per instance, merge operand size, distribution;
/// sketch-bench#147). Carried opaquely, like a synthetic workload
/// description; absent in tables written before it existed.
#[serde(default, skip_serializing_if = "Option::is_none")]
pub measured_at: Option<Value>,
}

pub type AtomicCostTable = Vec<AtomicCostEntry>;
pub use rqe_optimizer::{AtomicCostEntry, AtomicCostTable};

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.

Re-exporting rqe_optimizer's AtomicCostEntry drops the local #[serde(deny_unknown_fields)]. Unknown or misspelled cost fields (for example a new subtract_cpu_secs) used to fail to parse and are now silently accepted, and the canary test that guarded this was removed.

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.

Agreed that the strictness is lost, but it belongs to sketch-bench's type now that ASAPQuery uses it directly. Tracked in ProjectASAP/sketch-bench#161 (add deny_unknown_fields to AtomicCostEntry). On the typed measured_at: sketch-bench's reduce_one always writes the full block, so the typed form matches every table it emits. Tables are only produced by sketch-bench.

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.

Re-exporting sketch-bench's AtomicCostEntry drops #[serde(deny_unknown_fields)] and makes measured_at a typed MeasuredAt.

Failure: a misspelled field such as merge_acuracy now loads silently with the data dropped, so merged accuracy is never checked. Conversely, a profile whose measured_at lacks items_per_instance (accepted as opaque in #796) now fails to load.

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.

Agreed that the strictness is lost, but it belongs to sketch-bench's type now that ASAPQuery uses it directly. Tracked in ProjectASAP/sketch-bench#161 (add deny_unknown_fields to AtomicCostEntry). On the typed measured_at: sketch-bench's reduce_one always writes the full block, so the typed form matches every table it emits. Tables are only produced by sketch-bench.

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 also changes the greedy loader's parsing. Re-exporting rqe_optimizer's AtomicCostEntry makes measured_at typed (MeasuredAt with a required items_per_instance) where it was opaque JSON, and it drops deny_unknown_fields. A greedy cost document that #796 accepted can now fail to parse, and misspelled optional fields (merge_acuracy, measured-at) are now silently ignored instead of rejected. Was that intended?

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.

Intended. ASAPQuery now uses sketch-bench's type directly. sketch-bench's reduce_one always writes the full measured_at block, so the typed form matches every table it emits. Unknown-field strictness is tracked in ProjectASAP/sketch-bench#161 §2.

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.

Correctness: Re-exporting sketch-bench's AtomicCostEntry loses deny_unknown_fields, and the rejection test was removed. A typo such as merge_acuracy now parses silently with merge_accuracy empty, so accuracy_at falls back to single-instance query_accuracy. KLL/HLL rows can then pass accuracy_ok_for at L/W merges while their merged error exceeds the SLA.


/// Parse a standalone JSON workload selector. The selector is the exact
/// `profiles[].workload` value copied from the benchmark artifact, making the
Expand Down Expand Up @@ -472,6 +448,8 @@ fn valid_cost_entry(entry: &AtomicCostEntry) -> bool {

#[cfg(test)]
mod tests {
use std::collections::BTreeMap;

use super::*;

#[test]
Expand Down Expand Up @@ -578,21 +556,17 @@ mod tests {
assert!(serde_json::from_str::<AtomicCostEntry>(json).is_err());
}

/// sketch-bench#147 adds `measured_at`; it loads, and older rows without
/// it still do. Other unknown fields are still refused.
/// Rows with `measured_at` load, and older rows without it still do.
#[test]
fn atomic_cost_entry_accepts_optional_measured_at() {
let base = r#""sketch":"kll-percall","sketch_config":null,"mem_bytes_per_instance":1.0,"insert_cpu_secs":1.0,"merge_cpu_secs":1.0,"query_cpu_secs":1.0,"query_accuracy":{}"#;
let with = format!(
r#"{{{base},"measured_at":{{"items_per_instance":1000000,"keys_per_instance":100000,"value_range":[1.0,100000.0],"merge_operand_items":62500,"distribution":{{"kind":"zipf","skewness":1.1,"population_size":100000,"seed":42}}}}}}"#
);
let entry: AtomicCostEntry = serde_json::from_str(&with).unwrap();
assert_eq!(entry.measured_at.unwrap()["items_per_instance"], 1_000_000);
assert_eq!(entry.measured_at.unwrap().items_per_instance, 1_000_000);
let without: AtomicCostEntry = serde_json::from_str(&format!("{{{base}}}")).unwrap();
assert!(without.measured_at.is_none());
assert!(
serde_json::from_str::<AtomicCostEntry>(&format!(r#"{{{base},"other":1}}"#)).is_err()
);
}

#[test]
Expand All @@ -617,6 +591,7 @@ mod tests {
merge_cpu_secs: 4.5e-4,
query_cpu_secs: 7.8e-8,
query_accuracy: BTreeMap::new(),
merge_accuracy: BTreeMap::new(),
measured_at: None,
}
}
Expand All @@ -640,6 +615,7 @@ mod tests {
merge_cpu_secs: 4.0,
query_cpu_secs: 8.0,
query_accuracy: BTreeMap::new(),
merge_accuracy: BTreeMap::new(),
measured_at: None,
}
}
Expand All @@ -652,15 +628,6 @@ mod tests {
])
}

#[test]
fn atomic_cost_entry_deserializes_sketch_benchs_documented_shape() {
// Pinned against the current sketch-bench atomic-cost entry shape.
let json = r#"{"sketch":"cms-fastpath-vector2d","sketch_config":{"algorithm":"cms-fastpath-vector2d","params":{"cols":1024,"rows":3}},"mem_bytes_per_instance":12288.0,"insert_cpu_secs":8.484689139741214e-9,"merge_cpu_secs":0.00045364040539336466,"query_cpu_secs":7.799774697708031e-8,"query_accuracy":{"relative_error":0.01}}"#;
let entry: AtomicCostEntry = serde_json::from_str(json).expect("documented shape parses");
assert_eq!(entry.sketch, "cms-fastpath-vector2d");
assert_eq!(entry.mem_bytes_per_instance, 12288.0);
}

#[test]
fn cms_candidate_resolves_by_exact_key_regardless_of_value_type() {
// ASAPQuery's grid stores depth/width as u64; sketch-bench's exported
Expand Down Expand Up @@ -817,6 +784,7 @@ mod tests {
merge_cpu_secs: 2.76e-4,
query_cpu_secs: 1.23e-4,
query_accuracy: BTreeMap::new(),
merge_accuracy: BTreeMap::new(),
measured_at: None,
}];
let hll_params = HashMap::from([("precision".to_string(), Value::from(14u64))]);
Expand All @@ -830,6 +798,7 @@ mod tests {
merge_cpu_secs: 1.0e-3,
query_cpu_secs: 1.6e-4,
query_accuracy: BTreeMap::new(),
merge_accuracy: BTreeMap::new(),
measured_at: None,
}];
let kll_params = HashMap::from([("K".to_string(), Value::from(200u64))]);
Expand Down
2 changes: 2 additions & 0 deletions asap-planner-rs/src/optimizer/greedy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ mod tests {
merge_cpu_secs: 0.0,
query_cpu_secs: 0.0,
query_accuracy: std::collections::BTreeMap::new(),
merge_accuracy: std::collections::BTreeMap::new(),
measured_at: None,
}];
let aqe = make_aqe(Statistic::Topk, 60_000, 60_000, 1.0 / 60.0);
Expand Down Expand Up @@ -276,6 +277,7 @@ mod tests {
merge_cpu_secs: 0.0,
query_cpu_secs: 0.0,
query_accuracy: std::collections::BTreeMap::new(),
merge_accuracy: std::collections::BTreeMap::new(),
measured_at: None,
}];
let mut aqe = make_aqe(Statistic::Sum, 60_000, 60_000, 1.0 / 60.0);
Expand Down
Loading
Loading