Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
27 changes: 27 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,33 @@ changed:

Per-side env vars override shared env vars when both set the same key.

### What ran: the Run configuration block

Every comment the runner posts β€” running, completed, and failed β€” states what it
is benchmarking: a `Comparing <changed> (<sha>) to <baseline> [diff]` line, and a
collapsed **Run configuration** block holding the refs and env vars in the same
YAML the trigger accepts.

It is re-rendered from the job rather than quoted from the trigger comment,
because the two differ: one `run benchmark tpch clickbench_1` fans out into one
job (and one comment) per name, and a bare `run benchmarks` expands to the repo's
default suite. The block shows what *this* run did, so pasting it back into a PR
comment reproduces that run on its own.

```yaml
run benchmark tpch
env:
CARGO_BUILD_JOBS: "1"
baseline:
ref: "v45.0.0"
changed:
env:
DATAFUSION_RUNTIME_MEMORY_LIMIT: "2G"
```

Sections with nothing to report are omitted, so an unconfigured run renders as
just its trigger line.

### Peak memory pool reservation

DataFusion records the high-water mark of `MemoryPool::reserved()` per query and
Expand Down
11 changes: 10 additions & 1 deletion controller/src/bin/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use tracing::{error, info};
use benchmark_controller::github;
use benchmark_controller::runner::config::{BenchType, RunnerConfig};
use benchmark_controller::runner::poster::CommentPoster;
use benchmark_controller::runner::{bench_arrow, bench_datafusion, shell};
use benchmark_controller::runner::{bench_arrow, bench_datafusion, shell, trigger};

#[tokio::main]
async fn main() {
Expand Down Expand Up @@ -68,9 +68,18 @@ async fn run_benchmark(config: &RunnerConfig, poster: &CommentPoster) -> Result<
async fn post_error_comment(config: &RunnerConfig, poster: &CommentPoster) {
let tail = shell::tail_log(20).await;

// A failure can happen before either checkout resolves, so there is no
// comparison line here β€” just the requested configuration.
let bench_names = match config.bench_type {
BenchType::ArrowCriterion => config.bench_name.as_str(),
_ => config.benchmarks.as_str(),
};
let config_block = trigger::config_block(config, bench_names);

let footer = github::issues_footer(config.runner_repo_url.as_deref());
let body = format!(
"Benchmark for [this request]({}) failed.\n\n\
{config_block}\
Last 20 lines of output:\n\
<details><summary>Click to expand</summary>\n\n\
```\n\
Expand Down
8 changes: 8 additions & 0 deletions controller/src/job_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,14 @@ async fn create_k8s_job(
env.push(env_var(k, v));
}

// Also pass the shared block as a JSON map. The pod already has these set
// individually (above); the map lets the runner report them in PR comments
// without having to guess which of its env vars came from the trigger.
env.push(env_var(
"SHARED_ENV_VARS",
serde_json::to_string(&shared_env_vars)?,
));

// Pass per-side env vars as JSON maps for the runner to apply
env.push(env_var("BASELINE_ENV_VARS", &job.baseline_env_vars));
env.push(env_var("CHANGED_ENV_VARS", &job.changed_env_vars));
Expand Down
48 changes: 43 additions & 5 deletions controller/src/runner/bench_arrow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use crate::runner::git;
use crate::runner::monitor;
use crate::runner::poster::CommentPoster;
use crate::runner::shell;
use crate::runner::trigger;

/// Run an arrow-rs criterion benchmark comparing a PR branch to its merge-base.
pub async fn run(config: &RunnerConfig, poster: &CommentPoster) -> Result<()> {
Expand Down Expand Up @@ -75,6 +76,15 @@ pub async fn run(config: &RunnerConfig, poster: &CommentPoster) -> Result<()> {
} else {
format!("{} (merge-base)", &base_sha[..7.min(base_sha.len())])
};
let comparison = trigger::Comparison {
repo: &config.repo,
changed_display,
changed_sha: &changed_sha,
baseline_label: &baseline_label,
base_sha: &base_sha,
};
let config_block = trigger::config_block(config, bench_name);

let footer = github::issues_footer(config.runner_repo_url.as_deref());
let running_body = format!(
"\u{1f916} Arrow criterion benchmark running (GKE) | [trigger]({})\n\
Expand All @@ -84,14 +94,12 @@ pub async fn run(config: &RunnerConfig, poster: &CommentPoster) -> Result<()> {
{lscpu}\n\
```\n\n\
</details>\n\n\
Comparing {changed_display} ({changed_sha}) to {baseline_label} \
[diff](https://github.com/{repo}/compare/{base_sha}..{changed_sha})\n\
BENCH_NAME={bench_name}\n\
{comparison}\n\n\
{config_block}\
BENCH_COMMAND={bench_command_display}\n\
BENCH_FILTER={bench_filter}\n\
Results will be posted here when complete{footer}",
config.comment_url,
repo = config.repo,
comparison = comparison.line(),
);
let pr_number = config.pr_number()?;
poster
Expand Down Expand Up @@ -193,6 +201,8 @@ pub async fn run(config: &RunnerConfig, poster: &CommentPoster) -> Result<()> {
);
format_result_comment(
&config.comment_url,
&comparison.line(),
&config_block,
&report,
&resource_section,
&instance_type,
Expand All @@ -209,6 +219,8 @@ pub async fn run(config: &RunnerConfig, poster: &CommentPoster) -> Result<()> {
monitor::format_resource_comment("branch", &branch_stats).to_string();
format_branch_only_result_comment(
&config.comment_url,
&comparison.line(),
&config_block,
&report,
&resource_section,
&instance_type,
Expand Down Expand Up @@ -253,8 +265,14 @@ async fn copy_criterion_baselines(base_dir: &Path, branch_dir: &Path) {
}

/// Format the result comment body.
///
/// `comparison` and `config_block` restate what was run, so the result reads on
/// its own rather than only linking back to the trigger comment.
#[allow(clippy::too_many_arguments)]
fn format_result_comment(
comment_url: &str,
comparison: &str,
config_block: &str,
report: &str,
resource_section: &str,
instance_type: &str,
Expand All @@ -265,6 +283,8 @@ fn format_result_comment(
format!(
"\u{1f916} Arrow criterion benchmark completed (GKE) | [trigger]({comment_url})\n\n\
**Instance:** `{instance_type}` ({pod_resources})\n\n\
{comparison}\n\n\
{config_block}\
<details><summary>CPU Details (lscpu)</summary>\n\n\
```\n\
{lscpu}\n\
Expand All @@ -285,8 +305,11 @@ fn format_result_comment(
}

/// Format the result comment body for branch-only runs (no baseline comparison).
#[allow(clippy::too_many_arguments)]
fn format_branch_only_result_comment(
comment_url: &str,
comparison: &str,
config_block: &str,
report: &str,
resource_section: &str,
instance_type: &str,
Expand All @@ -297,6 +320,8 @@ fn format_branch_only_result_comment(
format!(
"\u{1f916} Arrow criterion benchmark completed (GKE) | [trigger]({comment_url})\n\n\
**Instance:** `{instance_type}` ({pod_resources})\n\n\
{comparison}\n\n\
{config_block}\
<details><summary>CPU Details (lscpu)</summary>\n\n\
```\n\
{lscpu}\n\
Expand Down Expand Up @@ -340,10 +365,17 @@ mod tests {
);
}

/// Stand-ins for the two "what was run" sections the runner passes in.
const COMPARISON: &str =
"Comparing my-branch (aaa) to bbb (merge-base) [diff](https://example.com/diff)";
const CONFIG_BLOCK: &str = "<details><summary>Run configuration</summary>\n\n```yaml\nrun benchmark concatenate_kernel\n```\n\n</details>\n\n";

#[test]
fn result_comment_format() {
let comment = format_result_comment(
"https://example.com/comment",
COMPARISON,
CONFIG_BLOCK,
"test report\n",
"resources\n",
"c4a-standard-48",
Expand All @@ -358,12 +390,16 @@ mod tests {
assert!(comment.contains("c4a-standard-48"));
assert!(comment.contains("12 vCPU / 65 GiB"));
assert!(comment.contains("lscpu output"));
assert!(comment.contains("Comparing my-branch (aaa)"));
assert!(comment.contains("run benchmark concatenate_kernel"));
}

#[test]
fn branch_only_result_comment_format() {
let comment = format_branch_only_result_comment(
"https://example.com/comment",
COMPARISON,
CONFIG_BLOCK,
"branch report\n",
"branch resources\n",
"c4a-standard-48",
Expand All @@ -372,6 +408,8 @@ mod tests {
"",
);
assert!(comment.contains("Arrow criterion benchmark completed"));
assert!(comment.contains("Comparing my-branch (aaa)"));
assert!(comment.contains("run benchmark concatenate_kernel"));
assert!(comment.contains("New benchmark β€” branch-only results"));
assert!(comment.contains("[trigger](https://example.com/comment)"));
assert!(comment.contains("branch report"));
Expand Down
54 changes: 50 additions & 4 deletions controller/src/runner/bench_datafusion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ use crate::runner::monitor::{self, ResourceStats};
use crate::runner::pool_peak::{self, BenchPeaks};
use crate::runner::poster::CommentPoster;
use crate::runner::shell;
use crate::runner::trigger;

/// Run DataFusion benchmarks comparing a PR branch to its merge-base.
pub async fn run(config: &RunnerConfig, poster: &CommentPoster) -> Result<()> {
Expand Down Expand Up @@ -121,6 +122,15 @@ pub async fn run(config: &RunnerConfig, poster: &CommentPoster) -> Result<()> {
format!("{} (merge-base)", &base_sha[..7.min(base_sha.len())])
};

let comparison = trigger::Comparison {
repo: &config.repo,
changed_display,
changed_sha: &changed_sha,
baseline_label: &baseline_label,
base_sha: &base_sha,
};
let config_block = trigger::config_block(config, benchmarks);

let footer = github::issues_footer(config.runner_repo_url.as_deref());
let running_body = format!(
"\u{1f916} Benchmark running (GKE) | [trigger]({})\n\
Expand All @@ -130,12 +140,11 @@ pub async fn run(config: &RunnerConfig, poster: &CommentPoster) -> Result<()> {
{lscpu}\n\
```\n\n\
</details>\n\n\
Comparing {changed_display} ({changed_sha}) to {baseline_label} \
[diff](https://github.com/{repo}/compare/{base_sha}..{changed_sha}) \
using: {benchmarks}\n\
{comparison}\n\n\
{config_block}\
Results will be posted here when complete{footer}",
config.comment_url,
repo = config.repo,
comparison = comparison.line(),
);
poster
.post_comment(&config.repo, pr_number, &running_body)
Expand Down Expand Up @@ -339,6 +348,8 @@ pub async fn run(config: &RunnerConfig, poster: &CommentPoster) -> Result<()> {
);
let result_body = format_result_comment(
&config.comment_url,
&comparison.line(),
&config_block,
&report,
&resource_section,
&pool_section,
Expand Down Expand Up @@ -722,13 +733,19 @@ fn format_resource_section(

/// Format the result comment body.
///
/// `comparison` and `config_block` restate what was run β€” the same two lines
/// the "running" comment opened with β€” so the result stands on its own instead
/// of only linking back to the trigger.
///
/// `pool_section` is empty whenever no side recorded a `pool_peak_bytes` β€” the
/// default, since runs set no memory limit unless the trigger comment asks for
/// one. Its `<details>` block is then omitted entirely, leaving the comment as
/// it was before the section existed.
#[allow(clippy::too_many_arguments)]
fn format_result_comment(
comment_url: &str,
comparison: &str,
config_block: &str,
report: &str,
resource_section: &str,
pool_section: &str,
Expand All @@ -749,6 +766,8 @@ fn format_result_comment(
format!(
"\u{1f916} Benchmark completed (GKE) | [trigger]({comment_url})\n\n\
**Instance:** `{instance_type}` ({pod_resources})\n\n\
{comparison}\n\n\
{config_block}\
<details><summary>CPU Details (lscpu)</summary>\n\n\
```\n\
{lscpu}\n\
Expand Down Expand Up @@ -777,6 +796,8 @@ mod tests {
fn result_comment_format() {
let comment = format_result_comment(
"https://example.com/comment",
"Comparing my-branch (aaa) to bbb (merge-base) [diff](https://example.com/diff)",
"<details><summary>Run configuration</summary>\n\n```yaml\nrun benchmark tpch\n```\n\n</details>\n\n",
"test report\n",
"resources\n",
"",
Expand All @@ -798,10 +819,35 @@ mod tests {
assert!(!comment.contains("Memory Pool Peaks"));
}

#[test]
fn result_comment_restates_what_was_run() {
// A result comment should be readable without opening the trigger.
let comment = format_result_comment(
"https://example.com/comment",
"Comparing my-branch (aaa) to bbb (merge-base) [diff](https://example.com/diff)",
"<details><summary>Run configuration</summary>\n\n```yaml\nrun benchmark tpch\nbaseline:\n ref: \"v45.0.0\"\n```\n\n</details>\n\n",
"test report\n",
"resources\n",
"",
"c4a-standard-48",
"12 vCPU / 65 GiB",
"lscpu output",
"",
);
assert!(comment.contains("Comparing my-branch (aaa) to bbb (merge-base)"));
assert!(comment.contains("<details><summary>Run configuration</summary>"));
assert!(comment.contains("run benchmark tpch"));
assert!(comment.contains("ref: \"v45.0.0\""));
// Stated up front, before the results themselves.
assert!(comment.find("Run configuration").unwrap() < comment.find("test report").unwrap());
}

#[test]
fn result_comment_includes_pool_section_when_recorded() {
let comment = format_result_comment(
"https://example.com/comment",
"Comparing my-branch (aaa) to bbb (merge-base) [diff](https://example.com/diff)",
"",
"test report\n",
"resources\n",
"pool peaks table\n",
Expand Down
12 changes: 11 additions & 1 deletion controller/src/runner/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
//! The controller passes these env vars to PR-triggered runner pods (see
//! `job_manager.rs`): `PR_URL`, `COMMENT_ID`, `BENCHMARKS`, `BENCH_TYPE`,
//! `BENCH_NAME`, `BENCH_FILTER`, `REPO`, `JOB_ID`, `RUNNER_TOKEN`,
//! `CONTROLLER_URL`, `RUNNER_REPO_URL`. The scheduled main-tracking
//! `CONTROLLER_URL`, `RUNNER_REPO_URL`, `SHARED_ENV_VARS`,
//! `BASELINE_ENV_VARS`, `CHANGED_ENV_VARS`. The scheduled main-tracking
//! workflow instead supplies `GITHUB_TOKEN` directly (no PR author to
//! distrust).

Expand Down Expand Up @@ -67,6 +68,10 @@ pub struct RunnerConfig {
pub poster_mode: PosterMode,
pub sccache_gcs_bucket: Option<String>,
pub data_cache_bucket: Option<String>,
/// Env vars from the trigger comment's shared `env:` block. These are
/// already set on the pod (so they reach the build too); the map is kept
/// only so comments can report what was requested.
pub shared_env_vars: HashMap<String, String>,
pub baseline_env_vars: HashMap<String, String>,
pub changed_env_vars: HashMap<String, String>,
pub baseline_ref: Option<String>,
Expand All @@ -83,6 +88,10 @@ impl RunnerConfig {
let comment_url = format!("{pr_url}#issuecomment-{comment_id}");
let bench_type_str = env_required("BENCH_TYPE")?;

let shared_env_vars: HashMap<String, String> = std::env::var("SHARED_ENV_VARS")
.ok()
.and_then(|s| serde_json::from_str(&s).ok())
.unwrap_or_default();
let baseline_env_vars: HashMap<String, String> = std::env::var("BASELINE_ENV_VARS")
.ok()
.and_then(|s| serde_json::from_str(&s).ok())
Expand All @@ -104,6 +113,7 @@ impl RunnerConfig {
poster_mode: parse_poster_mode()?,
sccache_gcs_bucket: std::env::var("SCCACHE_GCS_BUCKET").ok(),
data_cache_bucket: std::env::var("DATA_CACHE_BUCKET").ok(),
shared_env_vars,
baseline_env_vars,
changed_env_vars,
baseline_ref: std::env::var("BASELINE_REF").ok(),
Expand Down
1 change: 1 addition & 0 deletions controller/src/runner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,4 @@ pub mod monitor;
pub mod pool_peak;
pub mod poster;
pub mod shell;
pub mod trigger;
Loading
Loading