From 77f78b7dfe4e51090450d83dce76bc4365f8cb9c Mon Sep 17 00:00:00 2001 From: Adam Wildavsky Date: Tue, 21 Jul 2026 21:23:23 -0500 Subject: [PATCH 1/8] Port dtest benchmark.sh to Python with unit tests. Replace the shell implementation with python/tests/benchmark.py, keep ./benchmark.sh as a thin wrapper, and move the dirty-tree git-prep checks into //python:benchmark_test. Co-authored-by: Cursor --- BUILD.bazel | 15 - MODULE.bazel | 3 - benchmark.sh | 798 +---------------------------- benchmark_git_prep_test.sh | 137 ----- python/BUILD.bazel | 28 +- python/tests/benchmark.py | 904 +++++++++++++++++++++++++++++++++ python/tests/benchmark_test.py | 401 +++++++++++++++ 7 files changed, 1335 insertions(+), 951 deletions(-) delete mode 100755 benchmark_git_prep_test.sh create mode 100755 python/tests/benchmark.py create mode 100644 python/tests/benchmark_test.py diff --git a/BUILD.bazel b/BUILD.bazel index 2c3ed73d..1799dab3 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -86,21 +86,6 @@ config_setting( load("@rules_cc//cc:defs.bzl", "cc_library") -load("@rules_shell//shell:sh_test.bzl", "sh_test") - -# benchmark.sh --branch dirty-tree gate (clean tree only when switching commits). -# Needs bash + git; skipped on Windows like other host-tool shell checks. -sh_test( - name = "benchmark_git_prep_test", - size = "small", - srcs = ["benchmark_git_prep_test.sh"], - data = ["benchmark.sh"], - deps = ["@rules_shell//shell/runfiles"], - target_compatible_with = select({ - "//:build_windows": ["@platforms//:incompatible"], - "//conditions:default": [], - }), -) genrule( name = "doxygen_docs", diff --git a/MODULE.bazel b/MODULE.bazel index 1503870c..796ce3eb 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -9,9 +9,6 @@ bazel_dep(name = "platforms", version = "1.1.0") # Pin to the version the graph resolves (via rules_jvm_external) to avoid a # direct-dependency mismatch warning on every invocation. bazel_dep(name = "bazel_skylib", version = "1.9.0") -# sh_test for //:benchmark_git_prep_test (already pulled in transitively; -# pin directly so the load path stays stable). -bazel_dep(name = "rules_shell", version = "0.6.1") bazel_dep(name = "googletest", version = "1.17.0.bcr.2") bazel_dep(name = "pybind11_bazel", version = "3.0.1") bazel_dep(name = "rules_python", version = "2.0.1") diff --git a/benchmark.sh b/benchmark.sh index 2bd1638d..6373c409 100755 --- a/benchmark.sh +++ b/benchmark.sh @@ -1,797 +1,5 @@ #!/usr/bin/env bash -# Benchmark dtest performance across one or more binaries. -# -# Runs all combinations of solver (solve, calc) and hand file -# (list100/1000/…/1), largest files first. Always prints a summary. Per-run -# timing rows and build (git/bazel) output are shown transiently, then hidden -# unless the --details flag is set. Passes dtest options if given after -# "--" (see below). -# -# Usage: -# ./benchmark.sh -# ./benchmark.sh --build -# ./benchmark.sh -- -n 8 -r -# ./benchmark.sh --build --binary /path/to/other/dtest -# ./benchmark.sh --branch develop -- -n 8 -# ./benchmark.sh --binary /path/to/other/dtest --epsilon 1 -# ./benchmark.sh --repeats 5 -- -n 4 -# REPEATS=3 ./benchmark.sh -# -# Environment: -# BRANCH Path to the baseline dtest (default: bazel-bin under the current dir) -# BINARY Optional extra dtest binary to benchmark (like a trailing --binary) -# HANDS_DIR Directory containing list*.txt files (default: ./hands) -# REPEATS Runs per combination per binary (default: 1) -# MAX_DEALS Include list10^n.txt files with 10^n <= N (default: 100) -# DRY_RUN If 1, print commands only -# DETAILS If 1, keep per-run rows and build output (default: 0, summary only) -# EPSILON For a two-binary comparison, max % diff treated as equal (default: 0.5) - +# Thin wrapper around python/tests/benchmark.py for ./benchmark.sh invocation. set -euo pipefail - -# Operate on the current working directory (the repo you invoke this from), -# not the directory the script happens to live in. -ROOT="$(pwd)" - -# Fail fast if ROOT is not the root of a DDS checkout: the git/bazel operations -# and the default branch-binary and hands paths are all relative to it. -if [[ ! -f "$ROOT/MODULE.bazel" || ! -f "$ROOT/library/tests/BUILD.bazel" ]]; then - echo "error: '$ROOT' is not a DDS checkout root" >&2 - echo " cd to the root of the dds repository before running benchmark.sh" >&2 - exit 1 -fi -BRANCH="${BRANCH:-$ROOT/bazel-bin/library/tests/dtest}" -HANDS_DIR="${HANDS_DIR:-$ROOT/hands}" -REPEATS="${REPEATS:-1}" -MAX_DEALS="${MAX_DEALS:-100}" -DRY_RUN="${DRY_RUN:-0}" -DETAILS="${DETAILS:-0}" -EPSILON="${EPSILON:-0.5}" -BUILD=0 -REVERSE=0 -# Ordered list of binaries to benchmark, captured in command-line order. Each -# --branch/--binary appends one spec; the first spec is the baseline. -SPEC_KINDS=() # parallel: "branch" (git ref) or "binary" (path to a binary) -SPEC_VALS=() -CLI_BINARY_GIVEN=0 -REPEATS_GIVEN=0 -DTEST_EXTRA=() - -# Cleanup state (set later). The EXIT trap restores the original git branch if -# --branch switched away, and removes temp files. -RESULTS="" -TIME_FILE="" -ORIG_BRANCH="" -TMP_BINS=() -BUILD_LOG="" - -cleanup() { - # Leave the alternate screen first so any restore/error messages and the shell - # prompt land on the normal screen. - if [[ "${ALT_SCREEN_ACTIVE:-0}" == "1" ]]; then - printf '\033[?1049l' >/dev/tty 2>/dev/null || true - ALT_SCREEN_ACTIVE=0 - fi - if [[ -n "$ORIG_BRANCH" ]]; then - local cur - cur="$(git -C "$ROOT" symbolic-ref --quiet --short HEAD 2>/dev/null \ - || git -C "$ROOT" rev-parse HEAD 2>/dev/null || true)" - if [[ -n "$cur" && "$cur" != "$ORIG_BRANCH" ]]; then - echo "Restoring git branch '$ORIG_BRANCH'..." >&2 - git -C "$ROOT" checkout "$ORIG_BRANCH" >/dev/null 2>&1 || true - fi - fi - if ((${#TMP_BINS[@]} > 0)); then - rm -f "${TMP_BINS[@]}" - fi - [[ -n "$BUILD_LOG" ]] && rm -f "$BUILD_LOG" - [[ -n "$RESULTS" ]] && rm -f "$RESULTS" - [[ -n "$TIME_FILE" ]] && rm -f "$TIME_FILE" - return 0 -} -trap cleanup EXIT - -SOLVERS=(solve calc) - -usage() { - cat <&2 - exit 1 - fi - REPEATS_GIVEN=1 - shift - REPEATS="${1:?missing value for --repeats}" - shift - ;; - --branch) - shift - val="${1:?missing value for --branch}" - if [[ "$val" == -* ]]; then - echo "error: --branch ref must not start with '-': $val" >&2 - exit 1 - fi - SPEC_KINDS+=("branch") - SPEC_VALS+=("$val") - shift - ;; - --binary) - shift - SPEC_KINDS+=("binary") - SPEC_VALS+=("${1:?missing value for --binary}") - CLI_BINARY_GIVEN=1 - shift - ;; - --max-deals|--max_deals|-max-deals|-max_deals) - shift - MAX_DEALS="${1:?missing value for --max-deals}" - shift - ;; - --build) - BUILD=1 - shift - ;; - --reverse) - REVERSE=1 - shift - ;; - --details) - DETAILS=1 - shift - ;; - --epsilon) - shift - EPSILON="${1:?missing value for --epsilon}" - shift - ;; - --) - shift - DTEST_EXTRA=("$@") - break - ;; - *) - echo "Unknown option: $1" >&2 - usage >&2 - exit 1 - ;; - esac -done - -if ! [[ "$MAX_DEALS" =~ ^[0-9]+$ ]] || (( MAX_DEALS < 1 )); then - echo "error: max_deals must be a positive integer (got: $MAX_DEALS)" >&2 - exit 1 -fi - -if ! [[ "$REPEATS" =~ ^[0-9]+$ ]] || (( REPEATS < 1 )); then - echo "error: repeats must be a positive integer (got: $REPEATS)" >&2 - exit 1 -fi - -if ! [[ "$EPSILON" =~ ^[0-9]+(\.[0-9]+)?$ ]]; then - echo "error: epsilon must be a non-negative number (got: $EPSILON)" >&2 - exit 1 -fi - -# A binary supplied via the BINARY env var behaves like a trailing -# --binary PATH, unless --binary was already given on the command line. -if [[ "$CLI_BINARY_GIVEN" == "0" && -n "${BINARY:-}" ]]; then - SPEC_KINDS+=("binary") - SPEC_VALS+=("$BINARY") -fi - -nspecs=${#SPEC_KINDS[@]} -nbranch=0 -if (( nspecs > 0 )); then - for k in "${SPEC_KINDS[@]}"; do - [[ "$k" == "branch" ]] && nbranch=$((nbranch + 1)) - done -fi - -if [[ "$REVERSE" == "1" && "$nspecs" -lt 2 ]]; then - echo "error: --reverse requires at least two binaries (two or more --branch/--binary)" >&2 - exit 1 -fi - -# Check out $1, build dtest, and copy the binary to $2. -# Build output (git checkout + bazel) is noise unless --details was given. -# Trying to show it live and erase it afterward with ANSI does not work: bazel -# drives its own cursor save/restore for its progress display, clobbering any -# saved position, so a restore+clear erases nothing. Instead, capture the -# output to a log and surface it only on failure (or with --details). bazel sees -# a non-tty here and emits plain, line-based output. The short "Building..." -# labels are kept as progress markers. -bazel_dtest() { ( cd "$ROOT" && bazel build //library/tests:dtest ); } -checkout_and_build() { git -C "$ROOT" checkout "$1" && bazel_dtest; } - -run_build() { - if [[ "$DETAILS" == "1" ]]; then - "$@" - return - fi - if [[ -z "$BUILD_LOG" ]]; then - BUILD_LOG="$(mktemp "${TMPDIR:-/tmp}/dds-dtest-build.XXXXXX")" - fi - if ! "$@" >"$BUILD_LOG" 2>&1; then - cat "$BUILD_LOG" >&2 - return 1 - fi -} - -build_branch_binary() { - local name="$1" dest="$2" - local dtest_rel="bazel-bin/library/tests/dtest" - if [[ "$DRY_RUN" == "1" ]]; then - echo "DRY_RUN: git -C $ROOT checkout $name" >&2 - echo "DRY_RUN: (cd $ROOT && bazel build //library/tests:dtest)" >&2 - echo "DRY_RUN: cp -L $ROOT/$dtest_rel $dest" >&2 - return 0 - fi - echo "Building dtest from '$name'..." >&2 - run_build checkout_and_build "$name" - cp -L "$ROOT/$dtest_rel" "$dest" - chmod +x "$dest" -} - -# Display label for a binary given by path: its basename, or the raw -# path if basename is somehow empty. -label_for_path() { - local b - b="$(basename -- "$1")" - [[ -n "$b" ]] && printf '%s' "$b" || printf '%s' "$1" -} - -# Label for the current checkout's binary: the git branch, else "branch". -current_label() { - if [[ -n "$git_branch" && "$git_branch" != "unknown" ]]; then - printf '%s' "$git_branch" - else - printf 'branch' - fi -} - -# Validate the git work tree and branch refs before any checkout. Only called -# when at least one --branch spec is present. -git_prep_for_branches() { - if ! git -C "$ROOT" rev-parse --is-inside-work-tree >/dev/null 2>&1; then - echo "error: --branch requires a git work tree at $ROOT" >&2 - exit 1 - fi - ORIG_BRANCH="$(git -C "$ROOT" symbolic-ref --quiet --short HEAD 2>/dev/null \ - || git -C "$ROOT" rev-parse HEAD)" - - local i - # "." is shorthand for the current branch. - for i in "${!SPEC_VALS[@]}"; do - if [[ "${SPEC_KINDS[$i]}" == "branch" && "${SPEC_VALS[$i]}" == "." ]]; then - SPEC_VALS[$i]="$ORIG_BRANCH" - fi - done - for i in "${!SPEC_VALS[@]}"; do - # Require a commit-ish: plain rev-parse accepts trees/blobs, but later - # ^{commit} peels (and checkout) need a commit. - if [[ "${SPEC_KINDS[$i]}" == "branch" ]] \ - && ! git -C "$ROOT" rev-parse --verify --quiet \ - "${SPEC_VALS[$i]}^{commit}" >/dev/null; then - echo "error: --branch: unknown git ref '${SPEC_VALS[$i]}'" >&2 - exit 1 - fi - done - # A dirty tree only blocks when a --branch ref would leave HEAD's commit - # (git checkout of a different commit can refuse or overwrite local changes). - # Benchmarking the current branch / HEAD commit itself needs no switch, so - # uncommitted work is fine. Untracked files can also block a foreign checkout. - local head_commit needs_switch=0 ref_commit - head_commit="$(git -C "$ROOT" rev-parse HEAD)" - for i in "${!SPEC_VALS[@]}"; do - if [[ "${SPEC_KINDS[$i]}" == "branch" ]]; then - ref_commit="$(git -C "$ROOT" rev-parse --verify --quiet "${SPEC_VALS[$i]}^{commit}")" - if [[ "$ref_commit" != "$head_commit" ]]; then - needs_switch=1 - break - fi - fi - done - if (( needs_switch )) \ - && [[ -n "$(git -C "$ROOT" status --porcelain --untracked-files=normal)" ]]; then - echo "error: working tree not clean; commit, stash, or remove changes (tracked or untracked) before using --branch with a different commit" >&2 - exit 1 - fi -} - -# Restore the original branch. Pass 1 to also rebuild dtest (needed only when -# the current checkout's bazel-bin binary is used as a baseline). -restore_branch() { - local rebuild="$1" - if [[ "$DRY_RUN" == "1" ]]; then - echo "DRY_RUN: git -C $ROOT checkout $ORIG_BRANCH" >&2 - [[ "$rebuild" == "1" ]] && \ - echo "DRY_RUN: (cd $ROOT && bazel build //library/tests:dtest)" >&2 - return 0 - fi - if [[ "$rebuild" == "1" ]]; then - echo "Restoring '$ORIG_BRANCH' and rebuilding..." >&2 - run_build checkout_and_build "$ORIG_BRANCH" - else - echo "Restoring '$ORIG_BRANCH'..." >&2 - run_build git -C "$ROOT" checkout "$ORIG_BRANCH" - fi -} - -new_tmp_bin() { - local t - t="$(mktemp "${TMPDIR:-/tmp}/dds-dtest-bin.XXXXXX")" - TMP_BINS+=("$t") - printf '%s' "$t" -} - -# Build the ordered list of binaries to benchmark into BIN_LABELS / BIN_PATHS. -# Binary-set selection: -# (no spec) : one binary, the current checkout (the default) -# one or more specs : exactly those specs, in order; the checkout is ignored -# The first binary is the baseline. --branch specs are built from git; --binary -# specs are existing binary paths. -build_binaries() { - BIN_LABELS=() - BIN_PATHS=() - - if (( nspecs == 0 )); then - BIN_PATHS=("$BRANCH") - BIN_LABELS=("$(current_label)") - return 0 - fi - - if (( nbranch > 0 )); then - git_prep_for_branches - fi - - # Benchmark exactly the specified binaries, in order; the current checkout is - # only used when no spec is given at all. - local i kind val t - for i in "${!SPEC_KINDS[@]}"; do - kind="${SPEC_KINDS[$i]}" - val="${SPEC_VALS[$i]}" - if [[ "$kind" == "branch" ]]; then - t="$(new_tmp_bin)" - build_branch_binary "$val" "$t" - BIN_PATHS+=("$t") - BIN_LABELS+=("$val") - else - BIN_PATHS+=("$val") - BIN_LABELS+=("$(label_for_path "$val")") - fi - done - if (( nbranch > 0 )); then - restore_branch 0 - fi -} - -git_branch="unknown" -if git -C "$ROOT" rev-parse --is-inside-work-tree >/dev/null 2>&1; then - git_branch="$(git -C "$ROOT" rev-parse --abbrev-ref HEAD 2>/dev/null || echo unknown)" -fi - -build_binaries -num_bins=${#BIN_PATHS[@]} -if (( nspecs == 0 || nbranch == nspecs )); then - RUN_LABEL_COL="branch" -elif (( nbranch == 0 )); then - RUN_LABEL_COL="binary" -else - RUN_LABEL_COL="label" -fi -if (( nbranch > 0 )); then - BUILD=0 # build already done as part of the branch workflow -fi - -select_hand_files() { - is_power_of_10() { - local n="$1" - (( n >= 1 )) || return 1 - while (( n > 1 )); do - (( n % 10 == 0 )) || return 1 - n=$(( n / 10 )) - done - return 0 - } - - local -a candidates=() - local path base count - - shopt -s nullglob - for path in "$HANDS_DIR"/list*.txt; do - base="${path##*/}" - if [[ "$base" =~ ^list([0-9]+)\.txt$ ]]; then - count="${BASH_REMATCH[1]}" - if is_power_of_10 "$count" && (( count <= MAX_DEALS )); then - candidates+=("${count}:${base}") - fi - fi - done - shopt -u nullglob - - if ((${#candidates[@]} == 0)); then - echo "error: no list10^n.txt files with 10^n <= $MAX_DEALS in $HANDS_DIR" >&2 - exit 1 - fi - - FILES=() - local item - while IFS= read -r item; do - FILES+=("${item#*:}") - done < <(printf '%s\n' "${candidates[@]}" | sort -t: -k1,1rn) -} - -select_hand_files - -if [[ "$BUILD" == "1" ]]; then - if [[ "$DRY_RUN" == "1" ]]; then - echo "DRY_RUN: (cd $ROOT && bazel build //library/tests:dtest)" >&2 - else - echo "Building //library/tests:dtest..." >&2 - run_build bazel_dtest - fi -fi - -if [[ "$DRY_RUN" != "1" ]]; then - for i in "${!BIN_PATHS[@]}"; do - if [[ ! -x "${BIN_PATHS[$i]}" ]]; then - echo "error: binary not found or not executable: ${BIN_PATHS[$i]} (${BIN_LABELS[$i]})" >&2 - (( i == 0 )) && echo "hint: bazel build //library/tests:dtest" >&2 - exit 1 - fi - done -fi - -# Dispatch order of the binaries within each (solver, file, repeat). The -# baseline (index 0) leads by default; --reverse flips it. The summary keys on -# the stored binary index, so order never affects the reported results. -RUN_ORDER=() -for (( i = 0; i < num_bins; i++ )); do - RUN_ORDER+=("$i") -done -if [[ "$REVERSE" == "1" ]]; then - rev=() - for (( i = num_bins - 1; i >= 0; i-- )); do - rev+=("${RUN_ORDER[$i]}") - done - RUN_ORDER=("${rev[@]}") -fi - -for f in "${FILES[@]}"; do - if [[ ! -f "$HANDS_DIR/$f" ]]; then - echo "error: hand file not found: $HANDS_DIR/$f" >&2 - exit 1 - fi -done - -RESULTS="$(mktemp "${TMPDIR:-/tmp}/dds-benchmark.XXXXXX")" -TIME_FILE="$(mktemp "${TMPDIR:-/tmp}/dds-benchmark-time.XXXXXX")" -# Removal handled by the cleanup() EXIT trap installed near the top. - -parse_dtest_output() { - awk ' - /^Number of hands/ { hands = $NF } - /^User time \(ms\)/ { user = ($NF == "zero" ? 0 : $NF) } - /^Sys time \(ms\)/ { sys = ($NF == "zero" ? 0 : $NF) } - /^Avg user time \(ms\)/ { avg = ($NF == "zero" ? 0 : $NF) } - /^Ratio[[:space:]]/ { ratio = $NF } - END { - if (user == "") user = "NA" - if (sys == "") sys = "NA" - if (avg == "") { - if (user == 0) avg = 0 - else if (hands != "" && user != "NA" && hands > 0) avg = user / hands - else avg = "NA" - } - if (ratio == "") ratio = "NA" - print user, sys, avg, ratio - } - ' -} - -run_dtest() { - local binary="$1" - local solver="$2" - local hands="$3" - local -a cmd=("$binary" -f "$hands" -s "$solver") - if ((${#DTEST_EXTRA[@]} > 0)); then - cmd+=("${DTEST_EXTRA[@]}") - fi - - if [[ "$DRY_RUN" == "1" ]]; then - echo "DRY_RUN: ${cmd[*]}" >&2 - return 0 - fi - - # Bash time keyword (TIMEFORMAT=%R) captures wall-clock seconds on a side - # channel; dtest stdout/stderr stay separate for parse_dtest_output. - local out wall - local TIMEFORMAT='%R' - if ! { time out="$("${cmd[@]}" 2>&1)"; } 2>"$TIME_FILE"; then - echo "error: dtest failed: ${cmd[*]}" >&2 - echo "$out" >&2 - exit 1 - fi - local parsed - parsed="$(parse_dtest_output <<<"$out")" - local parsed_user parsed_sys - read -r parsed_user parsed_sys _ _ <<<"$parsed" - if [[ "$parsed_user" == "NA" || "$parsed_sys" == "NA" ]]; then - echo "warning: incomplete dtest timing output: ${cmd[*]}" >&2 - fi - wall="$(<"$TIME_FILE")" - [[ -z "$wall" ]] && wall="NA" - echo "$parsed $wall" -} - -show_run_lines=1 - -print_run_table_header() { - printf "%-6s %-13s %-12s %8s %8s %10s %6s %s\n" \ - "solver" "file" "$RUN_LABEL_COL" "user_ms" "sys_ms" "avg_user" "ratio" "run" - printf "%-6s %-13s %-12s %8s %8s %10s %6s %s\n" \ - "------" "-------------" "------------" "--------" "--------" "----------" "------" "---" -} - -print_run_row() { - printf "%-6s %-13s %-12s %8s %8s %10s %6s %s\n" \ - "$1" "$2" "$3" "$4" "$5" "$6" "$7" "$8" -} - -echo "DDS dtest benchmark" -echo "===================" -# One line per binary, baseline first. The label is the branch name (--branch), -# the binary's basename (--binary), or the current git branch (checkout). -for i in "${!BIN_PATHS[@]}"; do - tag="binary $((i + 1)):" - (( i == 0 )) && tag="baseline:" - printf "%-12s %s\n" "$tag" "${BIN_LABELS[$i]} (${BIN_PATHS[$i]})" -done -if (( num_bins >= 2 )); then - if [[ "$DETAILS" == "1" ]]; then - printf "%-12s %s\n" "details:" "on (per-run rows + build output)" - else - printf "%-12s %s\n" "details:" "off (summary only)" - fi - order_str="" - for idx in "${RUN_ORDER[@]}"; do - order_str+="${order_str:+, }${BIN_LABELS[$idx]}" - done - printf "%-12s %s\n" "run order:" "interleaved $order_str" - # The note column (and thus epsilon) only applies to a two-binary comparison. - if (( num_bins == 2 )); then - printf "%-12s %s\n" "epsilon:" "${EPSILON}%" - fi -fi -printf "%-12s %s\n" "hands dir:" "$HANDS_DIR" -printf "%-12s %s\n" "max_deals:" "$MAX_DEALS" -printf "%-12s %s\n" "files:" "${FILES[*]}" -printf "%-12s %s\n" "git branch:" "$git_branch" -printf "%-12s %s\n" "repeats:" "$REPEATS" -if ((${#DTEST_EXTRA[@]} > 0)); then - printf "%-12s %s\n" "dtest args:" "${DTEST_EXTRA[*]}" -fi -echo - -# The per-run rows are the script's live progress. With --details they are kept -# in the final output. Without --details, on a tty, they are shown on the -# alternate screen so the user sees progress, then discarded when we switch back -# to the main screen just before the summary; off a tty they are suppressed -# (summary only), since there is nothing to hide them. -show_run_lines=0 -ALT_SCREEN=0 -if [[ "$DRY_RUN" != "1" ]]; then - if [[ "$DETAILS" == "1" ]]; then - show_run_lines=1 - elif [[ -t 1 ]]; then - show_run_lines=1 - ALT_SCREEN=1 - fi -fi - -if [[ "$ALT_SCREEN" == "1" ]]; then - printf '\033[?1049h\033[H\033[2J' # enter alt screen, home, clear - ALT_SCREEN_ACTIVE=1 -fi - -if [[ "$DRY_RUN" != "1" && "$show_run_lines" == "1" ]]; then - print_run_table_header -fi - -total_runs=$(( ${#SOLVERS[@]} * ${#FILES[@]} * num_bins * REPEATS )) -run_no=0 - -for solver in "${SOLVERS[@]}"; do - for file in "${FILES[@]}"; do - hands="$HANDS_DIR/$file" - - for (( rep = 1; rep <= REPEATS; rep++ )); do - if [[ "$REPEATS" -gt 1 ]]; then - run_label="${rep}/${REPEATS}" - else - run_label="1/1" - fi - - for idx in "${RUN_ORDER[@]}"; do - bin="${BIN_PATHS[$idx]}" - run_no=$((run_no + 1)) - - if [[ "$DRY_RUN" == "1" ]]; then - run_dtest "$bin" "$solver" "$hands" - continue - fi - - read -r user sys avg ratio wall < <(run_dtest "$bin" "$solver" "$hands") - - if [[ "$show_run_lines" == "1" ]]; then - print_run_row "$solver" "$file" "${BIN_LABELS[$idx]:0:12}" \ - "$user" "$sys" "$avg" "$ratio" "$run_label" - fi - - # Column 3 is the binary index; the summary keys on it. - printf "%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\t%s\n" \ - "$solver" "$file" "$idx" "$rep" "$user" "$sys" "$avg" "$ratio" "$wall" \ - >>"$RESULTS" - done - done - done -done - -# Return to the normal screen, discarding the live dtest output, before the -# summary so the final output is just the header and the summary. -if [[ "$ALT_SCREEN" == "1" ]]; then - printf '\033[?1049l' - ALT_SCREEN_ACTIVE=0 -fi - -if [[ "$DRY_RUN" != "1" ]]; then - echo - echo "Summary (avg user ms)" - echo "==============================================================================" - - # One avg column per binary (baseline first). A ratio + note column are added - # only for a two-binary comparison; with three or more binaries the note has - # no single meaning and is dropped, leaving just the per-binary averages. - # Labels go through the environment (ENVIRON) rather than -v: BSD awk rejects - # newlines in a -v value, and labels are newline-joined. - labels_joined="$(printf '%s\n' "${BIN_LABELS[@]}")" - - LABELS="$labels_joined" awk -F'\t' -v nb="$num_bins" \ - -v files="${FILES[*]}" -v eps="$EPSILON" ' - function within_epsilon(a, b, e, hi, lo) { - e = eps / 100 - if (a > b) { hi = a; lo = b } else { hi = b; lo = a } - return (hi <= 0 || (hi - lo) / hi <= e) - } - function L(b) { return substr(lab[b + 1], 1, 12) } # truncated, for columns - function Lf(b) { return lab[b + 1] } # full, for notes - function pdash( b) { - printf "%-6s %-13s", "------", "-------------" - for (b = 0; b < nb; b++) printf " %12s", "------------" - if (nb == 2) printf " %10s %-15s", "----------", "---------------" - printf "\n" - } - BEGIN { split(ENVIRON["LABELS"], lab, "\n") } # lab[1..nb]; trailing empty ignored - { - base = $1 SUBSEP $2 - b = $3 + 0 - s[base, b] += $7 - c[base, b]++ - if ($9 != "NA") tw[b] += $9 # total wall-clock elapsed, per binary - } - END { - printf "%-6s %-13s", "solver", "file" - for (b = 0; b < nb; b++) printf " %12s", L(b) - if (nb == 2) printf " %10s %-15s", "ratio", "note" - printf "\n" - pdash() - - split("solve calc", solvers, " ") - nfiles = split(files, farr, " ") - for (si = 1; si <= 2; si++) { - for (fi = 1; fi <= nfiles; fi++) { - base = solvers[si] SUBSEP farr[fi] - ok = 1 - for (b = 0; b < nb; b++) if (!((base, b) in c)) ok = 0 - if (!ok) continue - # We intentionally do not test for averages that round to 0. - # Every average should be positive; a zero is rounding and means - # TestTimer.cpp should accumulate microseconds, not milliseconds. - printf "%-6s %-13s", solvers[si], farr[fi] - for (b = 0; b < nb; b++) { u[b] = s[base, b] / c[base, b]; printf " %12.2f", u[b] } - if (nb == 2) { - r = u[1] / u[0] - if (within_epsilon(u[0], u[1])) note = "equal" - else if (r >= 1) note = Lf(0) " faster" - else note = Lf(1) " faster" - printf " %10s %-15s", sprintf("%.2fx", r), note - } - printf "\n" - } - } - - pdash() - # Total elapsed (wall-clock seconds) summed per binary across all runs. - printf "%-6s %-13s", "TOTAL", "elapsed (s)" - allpos = 1 - for (b = 0; b < nb; b++) { printf " %12.2f", tw[b] + 0; if (!(tw[b] > 0)) allpos = 0 } - if (nb == 2) { - if (allpos) { - r = tw[1] / tw[0] - if (within_epsilon(tw[0], tw[1])) tnote = "equal" - else if (r >= 1) tnote = Lf(0) " faster" - else tnote = Lf(1) " faster" - printf " %10s %-15s", sprintf("%.2fx", r), tnote - } else { - printf " %10s %-15s", "", "" - } - } - printf "\n" - } - ' "$RESULTS" -fi - -echo -if [[ "$DRY_RUN" == "1" ]]; then - echo "DRY_RUN: $total_runs dtest invocations (not run)." -else - echo "Completed $run_no runs ($total_runs expected)." -fi +SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" +exec python3 "$SCRIPT_DIR/python/tests/benchmark.py" "$@" diff --git a/benchmark_git_prep_test.sh b/benchmark_git_prep_test.sh deleted file mode 100755 index f2f4db48..00000000 --- a/benchmark_git_prep_test.sh +++ /dev/null @@ -1,137 +0,0 @@ -#!/usr/bin/env bash -# Exercises benchmark.sh's --branch dirty-tree gate: a clean tree is required -# only when a --branch ref would check out a different commit than HEAD. -set -euo pipefail - -fail() { echo "FAIL: $*" >&2; exit 1; } -pass() { echo "PASS: $*"; } - -resolve_benchmark() { - # Under bazel test, locate via runfiles. Otherwise, use the script directory. - if [[ -n "${TEST_SRCDIR:-}${RUNFILES_DIR:-}${RUNFILES_MANIFEST_FILE:-}" ]]; then - # --- begin runfiles.bash initialization v3 --- - set -uo pipefail; set +e; f=bazel_tools/tools/bash/runfiles/runfiles.bash - # shellcheck disable=SC1090 - source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \ - source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \ - source "$0.runfiles/$f" 2>/dev/null || \ - source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \ - source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \ - { echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e - # --- end runfiles.bash initialization v3 --- - local path - path="$(rlocation "_main/benchmark.sh" || true)" - if [[ -n "$path" && -f "$path" ]]; then - printf '%s' "$path" - return 0 - fi - fi - local script_dir - script_dir="$(cd "$(dirname "$0")" && pwd)" - printf '%s' "$script_dir/benchmark.sh" -} - -BENCHMARK="$(resolve_benchmark)" -[[ -f "$BENCHMARK" ]] || fail "benchmark.sh not found at $BENCHMARK" - -command -v git >/dev/null 2>&1 || fail "git is required" - -TMP="$(mktemp -d "${TEST_TMPDIR:-${TMPDIR:-/tmp}}/dds-benchmark-git-prep.XXXXXX")" -trap 'rm -rf "$TMP"' EXIT - -git_q() { - git -c core.fsmonitor=false -c advice.detachedHead=false "$@" -} - -# Minimal two-branch repo at $1. Caller adds dirtiness (tracked and/or untracked). -setup_repo_base() { - local repo="$1" - mkdir -p "$repo/library/tests" "$repo/hands" - # Minimal DDS root markers that benchmark.sh requires. - printf 'module(name = "dds")\n' >"$repo/MODULE.bazel" - printf '# test BUILD\n' >"$repo/library/tests/BUILD.bazel" - printf 'hand\n' >"$repo/hands/list100.txt" - - git_q -C "$repo" init -q -b main - git_q -C "$repo" config user.email "test@example.com" - git_q -C "$repo" config user.name "Test" - git_q -C "$repo" add MODULE.bazel library/tests/BUILD.bazel hands/list100.txt - git_q -C "$repo" commit -q -m "initial" - - # Second commit on a side branch so --branch other differs from HEAD. - git_q -C "$repo" checkout -q -b other - echo "other" >"$repo/other.txt" - git_q -C "$repo" add other.txt - git_q -C "$repo" commit -q -m "other" - git_q -C "$repo" checkout -q main - - # Tag at HEAD so --branch can name the same commit without switching. - git_q -C "$repo" tag head-tag -} - -run_bench() { - local repo="$1" - shift - # Invoke via bash: Bazel runfiles may be read-only (no chmod +x). - ( cd "$repo" && DRY_RUN=1 bash "$BENCHMARK" "$@" ) 2>&1 -} - -# --- Tracked dirty tree --- -setup_repo_base "$TMP/repo" -echo "dirty" >>"$TMP/repo/MODULE.bazel" - -# Dirty tree + --branch for the current branch (by name) must succeed: no switch. -out="$(run_bench "$TMP/repo" --branch main)" || fail "dirty + --branch main exited $?" -[[ "$out" != *"working tree not clean"* ]] || fail "dirty + --branch main wrongly rejected: $out" -pass "dirty + --branch main allowed" - -# Dirty tree + --branch . (current branch shorthand) must succeed. -out="$(run_bench "$TMP/repo" --branch .)" || fail "dirty + --branch . exited $?" -[[ "$out" != *"working tree not clean"* ]] || fail "dirty + --branch . wrongly rejected: $out" -pass "dirty + --branch . allowed" - -# Dirty tree + tag at HEAD (same commit, peeled via ^{commit}) must succeed. -out="$(run_bench "$TMP/repo" --branch head-tag)" || fail "dirty + --branch head-tag exited $?" -[[ "$out" != *"working tree not clean"* ]] || fail "dirty + --branch head-tag wrongly rejected: $out" -pass "dirty + --branch head-tag allowed" - -# Dirty tree + --branch other (different commit) must fail with the clean-tree error. -set +e -out="$(run_bench "$TMP/repo" --branch other 2>&1)" -rc=$? -set -e -[[ "$rc" -ne 0 ]] || fail "dirty + --branch other should fail" -[[ "$out" == *"working tree not clean"* ]] || fail "dirty + --branch other missing clean-tree error: $out" -pass "dirty + --branch other rejected" - -# Non-commit revspec must fail at validation with a clear error (not a later ^{commit} crash). -set +e -out="$(run_bench "$TMP/repo" --branch 'HEAD^{tree}' 2>&1)" -rc=$? -set -e -[[ "$rc" -ne 0 ]] || fail "HEAD^{tree} should fail" -[[ "$out" == *"unknown git ref"* ]] || fail "HEAD^{tree} missing unknown-ref error: $out" -pass "non-commit revspec rejected" - -# --- Untracked-only dirty tree (status --porcelain --untracked-files=normal) --- -setup_repo_base "$TMP/repo_untracked" -echo "untracked" >"$TMP/repo_untracked/scratch.txt" - -# Untracked + --branch main (no switch) must succeed. -out="$(run_bench "$TMP/repo_untracked" --branch main)" \ - || fail "untracked + --branch main exited $?" -[[ "$out" != *"working tree not clean"* ]] \ - || fail "untracked + --branch main wrongly rejected: $out" -pass "untracked + --branch main allowed" - -# Untracked + --branch other (switch) must fail: untracked can block checkout. -set +e -out="$(run_bench "$TMP/repo_untracked" --branch other 2>&1)" -rc=$? -set -e -[[ "$rc" -ne 0 ]] || fail "untracked + --branch other should fail" -[[ "$out" == *"working tree not clean"* ]] \ - || fail "untracked + --branch other missing clean-tree error: $out" -pass "untracked + --branch other rejected" - -echo "All benchmark git-prep tests passed." diff --git a/python/BUILD.bazel b/python/BUILD.bazel index df169761..8804e4b6 100644 --- a/python/BUILD.bazel +++ b/python/BUILD.bazel @@ -1,6 +1,6 @@ load("//:CPPVARIABLES.bzl", "DDS_CPPOPTS") load("@pybind11_bazel//:build_defs.bzl", "pybind_extension") -load("@rules_python//python:defs.bzl", "py_library", "py_test") +load("@rules_python//python:defs.bzl", "py_binary", "py_library", "py_test") load("@rules_python//python:packaging.bzl", "py_wheel", "py_wheel_dist") pybind_extension( @@ -55,6 +55,32 @@ py_library( srcs = ["test_utils.py"], ) +py_library( + name = "benchmark_lib", + srcs = ["tests/benchmark.py"], + imports = ["tests"], +) + +py_binary( + name = "benchmark", + srcs = ["tests/benchmark.py"], + main = "tests/benchmark.py", + deps = [":benchmark_lib"], +) + +# Unit + git-prep tests for benchmark.py (needs git on the host). +py_test( + name = "benchmark_test", + size = "small", + main = "tests/benchmark_test.py", + srcs = ["tests/benchmark_test.py"], + deps = [":benchmark_lib"], + target_compatible_with = select({ + "//:build_windows": ["@platforms//:incompatible"], + "//conditions:default": [], + }), +) + py_test( name = "python_interface_smoke_test", size = "small", diff --git a/python/tests/benchmark.py b/python/tests/benchmark.py new file mode 100755 index 00000000..8306a889 --- /dev/null +++ b/python/tests/benchmark.py @@ -0,0 +1,904 @@ +#!/usr/bin/env python3 +"""Benchmark dtest performance across one or more binaries. + +Runs all combinations of solver (solve, calc) and hand file +(list100/1000/…/1), largest files first. Always prints a summary. Per-run +timing rows and build (git/bazel) output are shown transiently, then hidden +unless the --details flag is set. Passes dtest options if given after "--". + +Usage: + python/tests/benchmark.py + python/tests/benchmark.py --build + python/tests/benchmark.py -- -n 8 -r + python/tests/benchmark.py --build --binary /path/to/other/dtest + python/tests/benchmark.py --branch develop -- -n 8 + python/tests/benchmark.py --binary /path/to/other/dtest --epsilon 1 + python/tests/benchmark.py --repeats 5 -- -n 4 + REPEATS=3 python/tests/benchmark.py + ./benchmark.sh # equivalent wrapper at repo root + +Environment: + BRANCH Path to the baseline dtest (default: bazel-bin under the current dir) + BINARY Optional extra dtest binary to benchmark (like a trailing --binary) + HANDS_DIR Directory containing list*.txt files (default: ./hands) + REPEATS Runs per combination per binary (default: 1) + MAX_DEALS Include list10^n.txt files with 10^n <= N (default: 100) + DRY_RUN If 1, print commands only + DETAILS If 1, keep per-run rows and build output (default: 0, summary only) + EPSILON For a two-binary comparison, max % diff treated as equal (default: 0.5) +""" + +from __future__ import annotations + +import atexit +import os +import re +import shutil +import subprocess +import sys +import tempfile +import time +from dataclasses import dataclass, field +from pathlib import Path +from typing import Mapping, Sequence, TextIO + +SOLVERS = ("solve", "calc") +DTEST_REL = Path("bazel-bin/library/tests/dtest") +ALT_ENTER = "\033[?1049h\033[H\033[2J" +ALT_LEAVE = "\033[?1049l" + + +class BenchmarkError(Exception): + """User-facing configuration or validation error.""" + + +@dataclass(frozen=True) +class DtestTiming: + user_ms: float | None + sys_ms: float | None + avg_user: float | None + ratio: float | None + + +@dataclass(frozen=True) +class ResultRow: + solver: str + file: str + bin_idx: int + rep: int + user_ms: float | None + sys_ms: float | None + avg_user: float | None + ratio: float | None + wall_s: float | None + + +@dataclass +class Config: + repeats: int = 1 + max_deals: int = 100 + epsilon: float = 0.5 + dry_run: bool = False + details: bool = False + build: bool = False + reverse: bool = False + branch_binary: Path | None = None # BRANCH env / default dtest path + hands_dir: Path | None = None + specs: list[tuple[str, str]] = field(default_factory=list) + dtest_extra: list[str] = field(default_factory=list) + + +def is_power_of_10(n: int) -> bool: + if n < 1: + return False + while n > 1: + if n % 10 != 0: + return False + n //= 10 + return True + + +def is_dds_root(root: Path) -> bool: + return (root / "MODULE.bazel").is_file() and ( + root / "library" / "tests" / "BUILD.bazel" + ).is_file() + + +def label_for_path(path: str | Path) -> str: + name = Path(path).name + return name if name else str(path) + + +def run_order(num_bins: int, *, reverse: bool) -> list[int]: + order = list(range(num_bins)) + if reverse: + order.reverse() + return order + + +def within_epsilon(a: float, b: float, eps_pct: float) -> bool: + e = eps_pct / 100.0 + hi, lo = (a, b) if a > b else (b, a) + return hi <= 0 or (hi - lo) / hi <= e + + +def select_hand_files(hands_dir: Path, max_deals: int) -> list[str]: + candidates: list[tuple[int, str]] = [] + for path in hands_dir.glob("list*.txt"): + m = re.fullmatch(r"list([0-9]+)\.txt", path.name) + if not m: + continue + count = int(m.group(1)) + if is_power_of_10(count) and count <= max_deals: + candidates.append((count, path.name)) + if not candidates: + raise BenchmarkError( + f"no list10^n.txt files with 10^n <= {max_deals} in {hands_dir}" + ) + candidates.sort(key=lambda t: t[0], reverse=True) + return [name for _, name in candidates] + + +def _parse_ms(token: str) -> float | None: + if token == "zero": + return 0.0 + try: + return float(token) + except ValueError: + return None + + +def parse_dtest_output(text: str) -> DtestTiming: + hands: float | None = None + user: float | None = None + sys_ms: float | None = None + avg: float | None = None + ratio: float | None = None + for line in text.splitlines(): + if line.startswith("Number of hands"): + hands = _parse_ms(line.split()[-1]) + elif line.startswith("User time (ms)"): + user = _parse_ms(line.split()[-1]) + elif line.startswith("Sys time (ms)"): + sys_ms = _parse_ms(line.split()[-1]) + elif line.startswith("Avg user time (ms)"): + avg = _parse_ms(line.split()[-1]) + elif line.startswith("Ratio"): + # Match awk: /^Ratio[[:space:]]/ + if len(line) > 5 and line[5].isspace(): + ratio = _parse_ms(line.split()[-1]) + if avg is None: + if user == 0: + avg = 0.0 + elif hands is not None and user is not None and hands > 0: + avg = user / hands + return DtestTiming(user_ms=user, sys_ms=sys_ms, avg_user=avg, ratio=ratio) + + +def _fmt_timing(v: float | None) -> str: + if v is None: + return "NA" + if v == int(v): + return str(int(v)) + return str(v) + + +def format_summary( + rows: Sequence[ResultRow], + *, + labels: Sequence[str], + files: Sequence[str], + epsilon: float, +) -> str: + nb = len(labels) + sums: dict[tuple[str, str, int], float] = {} + counts: dict[tuple[str, str, int], int] = {} + total_wall: list[float] = [0.0] * nb + wall_seen: list[bool] = [False] * nb + + for row in rows: + key = (row.solver, row.file, row.bin_idx) + if row.avg_user is not None: + sums[key] = sums.get(key, 0.0) + row.avg_user + counts[key] = counts.get(key, 0) + 1 + if row.wall_s is not None: + total_wall[row.bin_idx] += row.wall_s + wall_seen[row.bin_idx] = True + + def L(b: int) -> str: + return labels[b][:12] + + def Lf(b: int) -> str: + return labels[b] + + lines: list[str] = [] + header = f"{'solver':<6} {'file':<13}" + for b in range(nb): + header += f" {L(b):>12}" + if nb == 2: + header += f" {'ratio':>10} {'note':<15}" + lines.append(header) + + def dash() -> None: + d = f"{'------':<6} {'-------------':<13}" + for _ in range(nb): + d += f" {'------------':>12}" + if nb == 2: + d += f" {'----------':>10} {'---------------':<15}" + lines.append(d) + + dash() + + for solver in SOLVERS: + for fname in files: + if not all((solver, fname, b) in counts for b in range(nb)): + continue + avgs = [sums[(solver, fname, b)] / counts[(solver, fname, b)] for b in range(nb)] + line = f"{solver:<6} {fname:<13}" + for u in avgs: + line += f" {u:12.2f}" + if nb == 2: + if avgs[0] > 0: + # If avgs[0] ever becomes zero, we should switch dtest timing from + # milliseconds to microseconds. + r = avgs[1] / avgs[0] + if within_epsilon(avgs[0], avgs[1], epsilon): + note = "equal" + elif r >= 1: + note = f"{Lf(0)} faster" + else: + note = f"{Lf(1)} faster" + line += f" {r:9.2f}x {note:<15}" + else: + line += f" {'':>10} {'':<15}" + lines.append(line) + + dash() + tot = f"{'TOTAL':<6} {'elapsed (s)':<13}" + allpos = True + for b in range(nb): + tw = total_wall[b] if wall_seen[b] else 0.0 + tot += f" {tw:12.2f}" + if not (tw > 0): + allpos = False + if nb == 2: + if allpos: + r = total_wall[1] / total_wall[0] + if within_epsilon(total_wall[0], total_wall[1], epsilon): + tnote = "equal" + elif r >= 1: + tnote = f"{Lf(0)} faster" + else: + tnote = f"{Lf(1)} faster" + tot += f" {r:9.2f}x {tnote:<15}" + else: + tot += f" {'':>10} {'':<15}" + lines.append(tot) + return "\n".join(lines) + + +def _env_truthy(env: Mapping[str, str], key: str) -> bool: + return env.get(key, "0") == "1" + + +def _parse_positive_int(value: str, name: str) -> int: + if not re.fullmatch(r"[0-9]+", value) or int(value) < 1: + raise BenchmarkError(f"{name} must be a positive integer (got: {value})") + return int(value) + + +def _parse_nonneg_float(value: str, name: str) -> float: + if not re.fullmatch(r"[0-9]+(\.[0-9]+)?", value): + raise BenchmarkError(f"{name} must be a non-negative number (got: {value})") + return float(value) + + +def parse_args(argv: Sequence[str], env: Mapping[str, str] | None = None) -> Config: + env = dict(os.environ if env is None else env) + cfg = Config( + repeats=_parse_positive_int(env.get("REPEATS", "1"), "repeats"), + max_deals=_parse_positive_int(env.get("MAX_DEALS", "100"), "max_deals"), + epsilon=_parse_nonneg_float(env.get("EPSILON", "0.5"), "epsilon"), + dry_run=_env_truthy(env, "DRY_RUN"), + details=_env_truthy(env, "DETAILS"), + ) + if env.get("HANDS_DIR"): + cfg.hands_dir = Path(env["HANDS_DIR"]) + if env.get("BRANCH"): + cfg.branch_binary = Path(env["BRANCH"]) + + repeats_given = False + cli_binary_given = False + args = list(argv) + i = 0 + while i < len(args): + a = args[i] + if a in ("-h", "--help"): + print(usage_text()) + raise SystemExit(0) + if a == "--repeats": + if repeats_given: + raise BenchmarkError("--repeats may be given only once") + repeats_given = True + i += 1 + if i >= len(args): + raise BenchmarkError("missing value for --repeats") + cfg.repeats = _parse_positive_int(args[i], "repeats") + elif a == "--branch": + i += 1 + if i >= len(args): + raise BenchmarkError("missing value for --branch") + val = args[i] + if val.startswith("-"): + raise BenchmarkError(f"--branch ref must not start with '-': {val}") + cfg.specs.append(("branch", val)) + elif a == "--binary": + i += 1 + if i >= len(args): + raise BenchmarkError("missing value for --binary") + cfg.specs.append(("binary", args[i])) + cli_binary_given = True + elif a in ("--max-deals", "--max_deals", "-max-deals", "-max_deals"): + i += 1 + if i >= len(args): + raise BenchmarkError("missing value for --max-deals") + cfg.max_deals = _parse_positive_int(args[i], "max_deals") + elif a == "--build": + cfg.build = True + elif a == "--reverse": + cfg.reverse = True + elif a == "--details": + cfg.details = True + elif a == "--epsilon": + i += 1 + if i >= len(args): + raise BenchmarkError("missing value for --epsilon") + cfg.epsilon = _parse_nonneg_float(args[i], "epsilon") + elif a == "--": + cfg.dtest_extra = args[i + 1 :] + break + else: + raise BenchmarkError(f"Unknown option: {a}\n{usage_text()}") + i += 1 + + if not cli_binary_given and env.get("BINARY"): + cfg.specs.append(("binary", env["BINARY"])) + + if cfg.reverse and len(cfg.specs) < 2: + raise BenchmarkError( + "--reverse requires at least two binaries (two or more --branch/--binary)" + ) + return cfg + + +def usage_text() -> str: + return """\ +Usage: benchmark.py [OPTIONS] + +Benchmark dtest across solver/file combinations. Always prints a summary; use +--details for per-run rows and build (git/bazel) output. + +Options: + -h, --help Show this help + --repeats N Runs per combination per binary (default: 1; env: REPEATS) + --max-deals N Include list10^n.txt files with 10^n <= N (default: 100; env: MAX_DEALS) + (alias: --max_deals) + --build Build dtest for the current checkout (bazel build //library/tests:dtest) + --branch NAME Git branch to build and benchmark ("." means the current branch). + Repeatable. Each named branch is checked out, dtest is built and + its binary saved, then the original branch is restored. A clean + tree is required only when a ref would switch away from HEAD. + --binary PATH Path to a prebuilt dtest binary to benchmark. Repeatable. + --details Keep per-run timing rows and build (git/bazel) output + --epsilon PCT For a two-binary comparison, treat timings within PCT% as equal + (default: 0.5; env: EPSILON) + --reverse Reverse the per-repeat dispatch order of the binaries + -- End benchmark options; remaining args are passed to dtest + (e.g. -- -n 8 -r for 8 threads and slow-board report) + +--branch and --binary may be given any number of times and combined freely; the +binaries are benchmarked in the order specified and the first is the baseline. The +current checkout is benchmarked by default only when neither flag is given. With two +binaries the summary adds a ratio and a "faster" note; with three or more it shows +only the per-binary averages (no note). + +Environment: + BRANCH, BINARY, HANDS_DIR, REPEATS, MAX_DEALS, DRY_RUN, DETAILS, EPSILON + +Examples: + python/tests/benchmark.py + python/tests/benchmark.py --build + python/tests/benchmark.py -- -n 8 + python/tests/benchmark.py --repeats 3 -- -n 4 -r + python/tests/benchmark.py --branch develop + python/tests/benchmark.py --branch develop --branch opus-two-percent + python/tests/benchmark.py --branch develop --branch opus-two-percent --branch fastest + python/tests/benchmark.py --branch opus-two-percent --binary /path/to/dtest + python/tests/benchmark.py --branch develop --repeats 3 -- -n 8 + python/tests/benchmark.py --binary /path/to/dtest + python/tests/benchmark.py --binary /path/to/dtest --details + python/tests/benchmark.py --binary /path/to/dtest --epsilon 1 + python/tests/benchmark.py --binary /path/to/dtest --reverse + python/tests/benchmark.py --repeats 5 --binary /path/to/dtest + DRY_RUN=1 python/tests/benchmark.py + ./benchmark.sh""" + + +def _git(root: Path, *args: str, check: bool = True) -> subprocess.CompletedProcess[str]: + return subprocess.run( + ["git", "-C", str(root), *args], + check=check, + text=True, + capture_output=True, + ) + + +def _git_out(root: Path, *args: str) -> str: + return _git(root, *args).stdout.strip() + + +def git_prep_for_branches( + root: Path, specs: list[tuple[str, str]] +) -> tuple[list[tuple[str, str]], str]: + """Validate git state and resolve '.' branch shorthand. + + Returns (resolved_specs, orig_branch). + """ + probe = _git(root, "rev-parse", "--is-inside-work-tree", check=False) + if probe.returncode != 0: + raise BenchmarkError(f"--branch requires a git work tree at {root}") + + sym = _git(root, "symbolic-ref", "--quiet", "--short", "HEAD", check=False) + if sym.returncode == 0 and sym.stdout.strip(): + orig_branch = sym.stdout.strip() + else: + orig_branch = _git_out(root, "rev-parse", "HEAD") + + resolved: list[tuple[str, str]] = [] + for kind, val in specs: + if kind == "branch" and val == ".": + resolved.append((kind, orig_branch)) + else: + resolved.append((kind, val)) + + for kind, val in resolved: + if kind != "branch": + continue + ok = _git(root, "rev-parse", "--verify", "--quiet", f"{val}^{{commit}}", check=False) + if ok.returncode != 0: + raise BenchmarkError(f"--branch: unknown git ref '{val}'") + + head_commit = _git_out(root, "rev-parse", "HEAD") + needs_switch = False + for kind, val in resolved: + if kind != "branch": + continue + ref_commit = _git_out(root, "rev-parse", "--verify", "--quiet", f"{val}^{{commit}}") + if ref_commit != head_commit: + needs_switch = True + break + + if needs_switch: + status = _git_out(root, "status", "--porcelain", "--untracked-files=normal") + if status: + raise BenchmarkError( + "working tree not clean; commit, stash, or remove changes " + "(tracked or untracked) before using --branch with a different commit" + ) + return resolved, orig_branch + + +class BenchmarkRunner: + def __init__( + self, + root: Path, + cfg: Config, + *, + err: TextIO = sys.stderr, + out: TextIO = sys.stdout, + ) -> None: + self.root = root + self.cfg = cfg + self.err = err + self.out = out + self.tmp_bins: list[Path] = [] + self.build_log: Path | None = None + self.orig_branch: str | None = None + self.alt_screen_active = False + self.git_branch = "unknown" + atexit.register(self.cleanup) + + def cleanup(self) -> None: + if self.alt_screen_active: + try: + sys.stdout.write(ALT_LEAVE) + sys.stdout.flush() + except OSError: + pass + self.alt_screen_active = False + if self.orig_branch: + cur = "" + try: + sym = _git( + self.root, + "symbolic-ref", + "--quiet", + "--short", + "HEAD", + check=False, + ) + cur = ( + sym.stdout.strip() + if sym.returncode == 0 + else _git_out(self.root, "rev-parse", "HEAD") + ) + except (subprocess.CalledProcessError, OSError): + cur = "" + if cur and cur != self.orig_branch: + print(f"Restoring git branch '{self.orig_branch}'...", file=self.err) + _git(self.root, "checkout", self.orig_branch, check=False) + for p in self.tmp_bins: + try: + p.unlink(missing_ok=True) + except OSError: + pass + self.tmp_bins.clear() + if self.build_log is not None: + try: + self.build_log.unlink(missing_ok=True) + except OSError: + pass + self.build_log = None + + def current_label(self) -> str: + if self.git_branch and self.git_branch != "unknown": + return self.git_branch + return "branch" + + def run_build(self, cmd: Sequence[str], *, cwd: Path | None = None) -> None: + if self.cfg.details: + subprocess.run(cmd, cwd=cwd or self.root, check=True) + return + if self.build_log is None: + fd, name = tempfile.mkstemp(prefix="dds-dtest-build.") + os.close(fd) + self.build_log = Path(name) + with open(self.build_log, "w", encoding="utf-8") as log: + proc = subprocess.run( + cmd, + cwd=cwd or self.root, + stdout=log, + stderr=subprocess.STDOUT, + check=False, + ) + if proc.returncode != 0: + sys.stderr.write(self.build_log.read_text(encoding="utf-8", errors="replace")) + raise subprocess.CalledProcessError(proc.returncode, cmd) + + def bazel_dtest(self) -> None: + self.run_build(["bazel", "build", "//library/tests:dtest"], cwd=self.root) + + def checkout_and_build(self, name: str) -> None: + self.run_build(["git", "-C", str(self.root), "checkout", name]) + self.bazel_dtest() + + def build_branch_binary(self, name: str, dest: Path) -> None: + if self.cfg.dry_run: + print(f"DRY_RUN: git -C {self.root} checkout {name}", file=self.err) + print( + f"DRY_RUN: (cd {self.root} && bazel build //library/tests:dtest)", + file=self.err, + ) + print(f"DRY_RUN: cp -L {self.root / DTEST_REL} {dest}", file=self.err) + return + print(f"Building dtest from '{name}'...", file=self.err) + self.checkout_and_build(name) + src = self.root / DTEST_REL + shutil.copy2(src, dest, follow_symlinks=True) + dest.chmod(dest.stat().st_mode | 0o111) + + def new_tmp_bin(self) -> Path: + fd, name = tempfile.mkstemp(prefix="dds-dtest-bin.") + os.close(fd) + path = Path(name) + self.tmp_bins.append(path) + return path + + def restore_branch(self, rebuild: bool) -> None: + assert self.orig_branch is not None + if self.cfg.dry_run: + print(f"DRY_RUN: git -C {self.root} checkout {self.orig_branch}", file=self.err) + if rebuild: + print( + f"DRY_RUN: (cd {self.root} && bazel build //library/tests:dtest)", + file=self.err, + ) + return + if rebuild: + print(f"Restoring '{self.orig_branch}' and rebuilding...", file=self.err) + self.checkout_and_build(self.orig_branch) + else: + print(f"Restoring '{self.orig_branch}'...", file=self.err) + self.run_build(["git", "-C", str(self.root), "checkout", self.orig_branch]) + + def build_binaries(self) -> tuple[list[str], list[Path]]: + specs = list(self.cfg.specs) + nspecs = len(specs) + nbranch = sum(1 for k, _ in specs if k == "branch") + + if nspecs == 0: + assert self.cfg.branch_binary is not None + return [self.current_label()], [self.cfg.branch_binary] + + if nbranch > 0: + specs, self.orig_branch = git_prep_for_branches(self.root, specs) + + labels: list[str] = [] + paths: list[Path] = [] + for kind, val in specs: + if kind == "branch": + t = self.new_tmp_bin() + self.build_branch_binary(val, t) + paths.append(t) + labels.append(val) + else: + paths.append(Path(val)) + labels.append(label_for_path(val)) + + if nbranch > 0: + self.restore_branch(False) + return labels, paths + + def run_dtest(self, binary: Path, solver: str, hands: Path) -> tuple[DtestTiming, float | None]: + cmd = [str(binary), "-f", str(hands), "-s", solver, *self.cfg.dtest_extra] + if self.cfg.dry_run: + print(f"DRY_RUN: {' '.join(cmd)}", file=self.err) + return DtestTiming(None, None, None, None), None + + t0 = time.perf_counter() + proc = subprocess.run(cmd, capture_output=True, text=True, check=False) + wall = time.perf_counter() - t0 + out = proc.stdout + proc.stderr + if proc.returncode != 0: + print(f"error: dtest failed: {' '.join(cmd)}", file=self.err) + print(out, file=self.err) + raise SystemExit(1) + parsed = parse_dtest_output(out) + if parsed.user_ms is None or parsed.sys_ms is None: + print(f"warning: incomplete dtest timing output: {' '.join(cmd)}", file=self.err) + return parsed, wall + + def detect_git_branch(self) -> None: + probe = _git(self.root, "rev-parse", "--is-inside-work-tree", check=False) + if probe.returncode != 0: + self.git_branch = "unknown" + return + abbrev = _git(self.root, "rev-parse", "--abbrev-ref", "HEAD", check=False) + self.git_branch = abbrev.stdout.strip() if abbrev.returncode == 0 else "unknown" + + +def main(argv: Sequence[str] | None = None, env: Mapping[str, str] | None = None) -> int: + argv = list(sys.argv[1:] if argv is None else argv) + env_map = dict(os.environ if env is None else env) + + root = Path.cwd() + if not is_dds_root(root): + print(f"error: '{root}' is not a DDS checkout root", file=sys.stderr) + print( + " cd to the root of the dds repository before running benchmark.py", + file=sys.stderr, + ) + return 1 + + try: + cfg = parse_args(argv, env=env_map) + except BenchmarkError as e: + print(f"error: {e}", file=sys.stderr) + return 1 + + if cfg.hands_dir is None: + cfg.hands_dir = root / "hands" + if cfg.branch_binary is None: + cfg.branch_binary = root / DTEST_REL + + runner = BenchmarkRunner(root, cfg) + runner.detect_git_branch() + + try: + labels, paths = runner.build_binaries() + except BenchmarkError as e: + print(f"error: {e}", file=sys.stderr) + return 1 + except subprocess.CalledProcessError: + return 1 + + num_bins = len(paths) + nspecs = len(cfg.specs) + nbranch = sum(1 for k, _ in cfg.specs if k == "branch") + # After git_prep, specs may have changed on the runner via build_binaries; + # recompute nbranch from original cfg (branch count is stable). + if nspecs == 0 or nbranch == nspecs: + run_label_col = "branch" + elif nbranch == 0: + run_label_col = "binary" + else: + run_label_col = "label" + if nbranch > 0: + cfg.build = False + + try: + files = select_hand_files(cfg.hands_dir, cfg.max_deals) + except BenchmarkError as e: + print(f"error: {e}", file=sys.stderr) + return 1 + + if cfg.build: + if cfg.dry_run: + print( + f"DRY_RUN: (cd {root} && bazel build //library/tests:dtest)", + file=sys.stderr, + ) + else: + print("Building //library/tests:dtest...", file=sys.stderr) + try: + runner.bazel_dtest() + except subprocess.CalledProcessError: + return 1 + + if not cfg.dry_run: + for i, p in enumerate(paths): + if not (p.is_file() and os.access(p, os.X_OK)): + print( + f"error: binary not found or not executable: {p} ({labels[i]})", + file=sys.stderr, + ) + if i == 0: + print("hint: bazel build //library/tests:dtest", file=sys.stderr) + return 1 + + order = run_order(num_bins, reverse=cfg.reverse) + + for f in files: + if not (cfg.hands_dir / f).is_file(): + print(f"error: hand file not found: {cfg.hands_dir / f}", file=sys.stderr) + return 1 + + print("DDS dtest benchmark") + print("===================") + for i, (lab, path) in enumerate(zip(labels, paths)): + tag = "baseline:" if i == 0 else f"binary {i + 1}:" + print(f"{tag:<12} {lab} ({path})") + if num_bins >= 2: + if cfg.details: + print(f"{'details:':<12} on (per-run rows + build output)") + else: + print(f"{'details:':<12} off (summary only)") + order_str = ", ".join(labels[idx] for idx in order) + print(f"{'run order:':<12} interleaved {order_str}") + if num_bins == 2: + print(f"{'epsilon:':<12} {cfg.epsilon}%") + print(f"{'hands dir:':<12} {cfg.hands_dir}") + print(f"{'max_deals:':<12} {cfg.max_deals}") + print(f"{'files:':<12} {' '.join(files)}") + print(f"{'git branch:':<12} {runner.git_branch}") + print(f"{'repeats:':<12} {cfg.repeats}") + if cfg.dtest_extra: + print(f"{'dtest args:':<12} {' '.join(cfg.dtest_extra)}") + print() + sys.stdout.flush() + + show_run_lines = False + alt_screen = False + if not cfg.dry_run: + if cfg.details: + show_run_lines = True + elif sys.stdout.isatty(): + show_run_lines = True + alt_screen = True + + if alt_screen: + sys.stdout.write(ALT_ENTER) + sys.stdout.flush() + runner.alt_screen_active = True + + def print_run_header() -> None: + print( + f"{'solver':<6} {'file':<13} {run_label_col:<12} " + f"{'user_ms':>8} {'sys_ms':>8} {'avg_user':>10} {'ratio':>6} run" + ) + print( + f"{'------':<6} {'-------------':<13} {'------------':<12} " + f"{'--------':>8} {'--------':>8} {'----------':>10} {'------':>6} ---" + ) + + def print_run_row( + solver: str, + file: str, + lab: str, + user: str, + sys_ms: str, + avg: str, + ratio: str, + run_label: str, + ) -> None: + print( + f"{solver:<6} {file:<13} {lab:<12} " + f"{user:>8} {sys_ms:>8} {avg:>10} {ratio:>6} {run_label}" + ) + + if not cfg.dry_run and show_run_lines: + print_run_header() + + total_runs = len(SOLVERS) * len(files) * num_bins * cfg.repeats + run_no = 0 + results: list[ResultRow] = [] + + for solver in SOLVERS: + for file in files: + hands = cfg.hands_dir / file + for rep in range(1, cfg.repeats + 1): + run_label = f"{rep}/{cfg.repeats}" if cfg.repeats > 1 else "1/1" + for idx in order: + bin_path = paths[idx] + run_no += 1 + if cfg.dry_run: + runner.run_dtest(bin_path, solver, hands) + continue + parsed, wall = runner.run_dtest(bin_path, solver, hands) + if show_run_lines: + print_run_row( + solver, + file, + labels[idx][:12], + _fmt_timing(parsed.user_ms), + _fmt_timing(parsed.sys_ms), + _fmt_timing(parsed.avg_user), + _fmt_timing(parsed.ratio), + run_label, + ) + results.append( + ResultRow( + solver, + file, + idx, + rep, + parsed.user_ms, + parsed.sys_ms, + parsed.avg_user, + parsed.ratio, + wall, + ) + ) + + if alt_screen: + sys.stdout.write(ALT_LEAVE) + sys.stdout.flush() + runner.alt_screen_active = False + + if not cfg.dry_run: + print() + print("Summary (avg user ms)") + print("==============================================================================") + print( + format_summary( + results, + labels=labels, + files=files, + epsilon=cfg.epsilon, + ) + ) + + print() + if cfg.dry_run: + print(f"DRY_RUN: {total_runs} dtest invocations (not run).") + else: + print(f"Completed {run_no} runs ({total_runs} expected).") + return 0 + + +if __name__ == "__main__": + try: + raise SystemExit(main()) + except BenchmarkError as e: + print(f"error: {e}", file=sys.stderr) + raise SystemExit(1) from e diff --git a/python/tests/benchmark_test.py b/python/tests/benchmark_test.py new file mode 100644 index 00000000..b9963bad --- /dev/null +++ b/python/tests/benchmark_test.py @@ -0,0 +1,401 @@ +#!/usr/bin/env python3 +"""Unit tests for benchmark.py (stdlib unittest).""" + +from __future__ import annotations + +import os +import subprocess +import tempfile +import unittest +from pathlib import Path +from unittest import mock + +import benchmark + + +class TestIsPowerOf10(unittest.TestCase): + def test_powers(self) -> None: + for n in (1, 10, 100, 1000, 10000): + self.assertTrue(benchmark.is_power_of_10(n), n) + + def test_non_powers(self) -> None: + for n in (0, -1, 2, 11, 99, 101, 300): + self.assertFalse(benchmark.is_power_of_10(n), n) + + +class TestSelectHandFiles(unittest.TestCase): + def test_selects_powers_of_10_descending(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + hands = Path(tmp) + for name in ( + "list1.txt", + "list10.txt", + "list100.txt", + "list1000.txt", + "list2.txt", + "list300.txt", + "list1000_with_dups.txt", + ): + (hands / name).write_text("x\n") + files = benchmark.select_hand_files(hands, max_deals=100) + self.assertEqual(files, ["list100.txt", "list10.txt", "list1.txt"]) + + def test_empty_raises(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + with self.assertRaises(benchmark.BenchmarkError) as ctx: + benchmark.select_hand_files(Path(tmp), max_deals=100) + self.assertIn("no list10^n.txt", str(ctx.exception)) + + +class TestParseDtestOutput(unittest.TestCase): + def test_full_output(self) -> None: + out = ( + "Number of hands 100\n" + "User time (ms) 250\n" + "Sys time (ms) 10\n" + "Avg user time (ms) 2.5\n" + "Ratio 1.23\n" + ) + parsed = benchmark.parse_dtest_output(out) + self.assertEqual(parsed.user_ms, 250.0) + self.assertEqual(parsed.sys_ms, 10.0) + self.assertEqual(parsed.avg_user, 2.5) + self.assertEqual(parsed.ratio, 1.23) + + def test_zero_tokens(self) -> None: + out = ( + "Number of hands 0\n" + "User time (ms) zero\n" + "Sys time (ms) zero\n" + "Avg user time (ms) zero\n" + ) + parsed = benchmark.parse_dtest_output(out) + self.assertEqual(parsed.user_ms, 0.0) + self.assertEqual(parsed.sys_ms, 0.0) + self.assertEqual(parsed.avg_user, 0.0) + + def test_avg_derived_from_user_and_hands(self) -> None: + out = ( + "Number of hands 100\n" + "User time (ms) 200\n" + "Sys time (ms) 5\n" + ) + parsed = benchmark.parse_dtest_output(out) + self.assertEqual(parsed.avg_user, 2.0) + + def test_missing_fields_are_na(self) -> None: + parsed = benchmark.parse_dtest_output("hello\n") + self.assertIsNone(parsed.user_ms) + self.assertIsNone(parsed.sys_ms) + self.assertIsNone(parsed.avg_user) + self.assertIsNone(parsed.ratio) + + +class TestWithinEpsilon(unittest.TestCase): + def test_equal_within(self) -> None: + self.assertTrue(benchmark.within_epsilon(100.0, 100.4, 0.5)) + self.assertFalse(benchmark.within_epsilon(100.0, 101.0, 0.5)) + + def test_zero_hi(self) -> None: + self.assertTrue(benchmark.within_epsilon(0.0, 0.0, 0.5)) + + +class TestLabelForPath(unittest.TestCase): + def test_basename(self) -> None: + self.assertEqual(benchmark.label_for_path("/tmp/foo/dtest"), "dtest") + + +class TestRunOrder(unittest.TestCase): + def test_default(self) -> None: + self.assertEqual(benchmark.run_order(3, reverse=False), [0, 1, 2]) + + def test_reverse(self) -> None: + self.assertEqual(benchmark.run_order(3, reverse=True), [2, 1, 0]) + + +class TestParseArgs(unittest.TestCase): + def test_defaults(self) -> None: + cfg = benchmark.parse_args([], env={}) + self.assertEqual(cfg.repeats, 1) + self.assertEqual(cfg.max_deals, 100) + self.assertEqual(cfg.epsilon, 0.5) + self.assertFalse(cfg.build) + self.assertFalse(cfg.details) + self.assertFalse(cfg.reverse) + self.assertEqual(cfg.specs, []) + self.assertEqual(cfg.dtest_extra, []) + + def test_env_overrides(self) -> None: + cfg = benchmark.parse_args( + [], + env={ + "REPEATS": "3", + "MAX_DEALS": "10", + "EPSILON": "1.5", + "DRY_RUN": "1", + "DETAILS": "1", + }, + ) + self.assertEqual(cfg.repeats, 3) + self.assertEqual(cfg.max_deals, 10) + self.assertEqual(cfg.epsilon, 1.5) + self.assertTrue(cfg.dry_run) + self.assertTrue(cfg.details) + + def test_binary_env_appended(self) -> None: + cfg = benchmark.parse_args([], env={"BINARY": "/tmp/other"}) + self.assertEqual(cfg.specs, [("binary", "/tmp/other")]) + + def test_cli_binary_suppresses_env(self) -> None: + cfg = benchmark.parse_args( + ["--binary", "/cli"], + env={"BINARY": "/env"}, + ) + self.assertEqual(cfg.specs, [("binary", "/cli")]) + + def test_repeatable_specs_and_dtest_extra(self) -> None: + cfg = benchmark.parse_args( + [ + "--branch", + "develop", + "--binary", + "/tmp/dtest", + "--repeats", + "2", + "--", + "-n", + "8", + "-r", + ], + env={}, + ) + self.assertEqual( + cfg.specs, + [("branch", "develop"), ("binary", "/tmp/dtest")], + ) + self.assertEqual(cfg.repeats, 2) + self.assertEqual(cfg.dtest_extra, ["-n", "8", "-r"]) + + def test_max_deals_aliases(self) -> None: + for flag in ("--max-deals", "--max_deals", "-max-deals", "-max_deals"): + cfg = benchmark.parse_args([flag, "10"], env={}) + self.assertEqual(cfg.max_deals, 10, flag) + + def test_repeats_only_once(self) -> None: + with self.assertRaises(benchmark.BenchmarkError) as ctx: + benchmark.parse_args(["--repeats", "2", "--repeats", "3"], env={}) + self.assertIn("--repeats may be given only once", str(ctx.exception)) + + def test_branch_must_not_start_with_dash(self) -> None: + with self.assertRaises(benchmark.BenchmarkError) as ctx: + benchmark.parse_args(["--branch", "-bad"], env={}) + self.assertIn("must not start with '-'", str(ctx.exception)) + + def test_reverse_requires_two_specs(self) -> None: + with self.assertRaises(benchmark.BenchmarkError) as ctx: + benchmark.parse_args(["--reverse", "--binary", "/x"], env={}) + self.assertIn("--reverse requires at least two", str(ctx.exception)) + + def test_invalid_repeats(self) -> None: + with self.assertRaises(benchmark.BenchmarkError): + benchmark.parse_args(["--repeats", "0"], env={}) + + def test_invalid_epsilon(self) -> None: + with self.assertRaises(benchmark.BenchmarkError): + benchmark.parse_args(["--epsilon", "-1"], env={}) + + +class TestSummary(unittest.TestCase): + def test_two_binary_ratio_and_note(self) -> None: + rows = [ + benchmark.ResultRow("solve", "list100.txt", 0, 1, 100.0, 1.0, 1.0, 1.0, 1.0), + benchmark.ResultRow("solve", "list100.txt", 1, 1, 50.0, 1.0, 0.5, 1.0, 0.5), + ] + text = benchmark.format_summary( + rows, + labels=["base", "fast"], + files=["list100.txt"], + epsilon=0.5, + ) + self.assertIn("base", text) + self.assertIn("fast", text) + self.assertIn("0.50x", text) + self.assertIn("fast faster", text) + self.assertIn("TOTAL", text) + + def test_equal_within_epsilon(self) -> None: + rows = [ + benchmark.ResultRow("solve", "list100.txt", 0, 1, 100.0, 1.0, 1.0, 1.0, 1.0), + benchmark.ResultRow("solve", "list100.txt", 1, 1, 100.2, 1.0, 1.002, 1.0, 1.0), + ] + text = benchmark.format_summary( + rows, + labels=["a", "b"], + files=["list100.txt"], + epsilon=0.5, + ) + self.assertIn("equal", text) + + def test_three_binaries_no_note(self) -> None: + rows = [ + benchmark.ResultRow("solve", "list1.txt", b, 1, 10.0, 0.0, 1.0, None, 0.1) + for b in (0, 1, 2) + ] + text = benchmark.format_summary( + rows, + labels=["a", "b", "c"], + files=["list1.txt"], + epsilon=0.5, + ) + self.assertNotIn("ratio", text) + self.assertNotIn("note", text) + + def test_zero_baseline_avg_skips_ratio(self) -> None: + rows = [ + benchmark.ResultRow("solve", "list1.txt", 0, 1, 0.0, 0.0, 0.0, None, 0.0), + benchmark.ResultRow("solve", "list1.txt", 1, 1, 1.0, 0.0, 1.0, None, 0.1), + ] + text = benchmark.format_summary( + rows, + labels=["a", "b"], + files=["list1.txt"], + epsilon=0.5, + ) + solve_line = next( + line for line in text.splitlines() if line.startswith("solve ") + ) + self.assertRegex(solve_line, r"\b0\.00\b") + self.assertRegex(solve_line, r"\b1\.00\b") + self.assertNotRegex(solve_line, r"\d+\.\d+x") + self.assertNotIn("faster", solve_line) + self.assertNotIn("equal", solve_line) + + +class TestIsDdsRoot(unittest.TestCase): + def test_valid(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + (root / "MODULE.bazel").write_text("module(name = \"dds\")\n") + (root / "library" / "tests").mkdir(parents=True) + (root / "library" / "tests" / "BUILD.bazel").write_text("#\n") + self.assertTrue(benchmark.is_dds_root(root)) + + def test_invalid(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + self.assertFalse(benchmark.is_dds_root(Path(tmp))) + + +def _git(repo: Path, *args: str) -> str: + return subprocess.check_output( + [ + "git", + "-c", + "core.fsmonitor=false", + "-c", + "advice.detachedHead=false", + "-C", + str(repo), + *args, + ], + text=True, + ).strip() + + +def _setup_repo(repo: Path) -> None: + (repo / "library" / "tests").mkdir(parents=True) + (repo / "hands").mkdir() + (repo / "MODULE.bazel").write_text('module(name = "dds")\n') + (repo / "library" / "tests" / "BUILD.bazel").write_text("# test BUILD\n") + (repo / "hands" / "list100.txt").write_text("hand\n") + _git(repo, "init", "-q", "-b", "main") + _git(repo, "config", "user.email", "test@example.com") + _git(repo, "config", "user.name", "Test") + _git(repo, "add", "MODULE.bazel", "library/tests/BUILD.bazel", "hands/list100.txt") + _git(repo, "commit", "-q", "-m", "initial") + _git(repo, "checkout", "-q", "-b", "other") + (repo / "other.txt").write_text("other\n") + _git(repo, "add", "other.txt") + _git(repo, "commit", "-q", "-m", "other") + _git(repo, "checkout", "-q", "main") + _git(repo, "tag", "head-tag") + + +class TestGitPrepForBranches(unittest.TestCase): + def test_dirty_same_commit_allowed(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + repo = Path(tmp) / "repo" + repo.mkdir() + _setup_repo(repo) + (repo / "MODULE.bazel").write_text( + (repo / "MODULE.bazel").read_text() + "dirty\n" + ) + for ref in ("main", ".", "head-tag"): + specs = [("branch", ref)] + resolved, orig = benchmark.git_prep_for_branches(repo, specs) + self.assertEqual(orig, "main") + self.assertNotEqual(resolved[0][1], ".") + + def test_dirty_other_commit_rejected(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + repo = Path(tmp) / "repo" + repo.mkdir() + _setup_repo(repo) + (repo / "MODULE.bazel").write_text( + (repo / "MODULE.bazel").read_text() + "dirty\n" + ) + with self.assertRaises(benchmark.BenchmarkError) as ctx: + benchmark.git_prep_for_branches(repo, [("branch", "other")]) + self.assertIn("working tree not clean", str(ctx.exception)) + + def test_untracked_other_commit_rejected(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + repo = Path(tmp) / "repo" + repo.mkdir() + _setup_repo(repo) + (repo / "scratch.txt").write_text("untracked\n") + with self.assertRaises(benchmark.BenchmarkError) as ctx: + benchmark.git_prep_for_branches(repo, [("branch", "other")]) + self.assertIn("working tree not clean", str(ctx.exception)) + + def test_untracked_same_commit_allowed(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + repo = Path(tmp) / "repo" + repo.mkdir() + _setup_repo(repo) + (repo / "scratch.txt").write_text("untracked\n") + benchmark.git_prep_for_branches(repo, [("branch", "main")]) + + def test_non_commit_revspec_rejected(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + repo = Path(tmp) / "repo" + repo.mkdir() + _setup_repo(repo) + with self.assertRaises(benchmark.BenchmarkError) as ctx: + benchmark.git_prep_for_branches(repo, [("branch", "HEAD^{tree}")]) + self.assertIn("unknown git ref", str(ctx.exception)) + + +class TestDryRunMain(unittest.TestCase): + def test_dry_run_prints_commands(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + (root / "library" / "tests").mkdir(parents=True) + (root / "hands").mkdir() + (root / "MODULE.bazel").write_text('module(name = "dds")\n') + (root / "library" / "tests" / "BUILD.bazel").write_text("#\n") + (root / "hands" / "list1.txt").write_text("x\n") + (root / "hands" / "list10.txt").write_text("x\n") + fake_bin = root / "fake_dtest" + fake_bin.write_text("#!/bin/sh\nexit 0\n") + fake_bin.chmod(0o755) + with mock.patch("benchmark.Path.cwd", return_value=root): + with mock.patch("sys.stdout"), mock.patch("sys.stderr"): + rc = benchmark.main( + ["--binary", str(fake_bin), "--max-deals", "10"], + env={"DRY_RUN": "1", "HANDS_DIR": str(root / "hands")}, + ) + self.assertEqual(rc, 0) + + +if __name__ == "__main__": + unittest.main() From 33aeebec501598fdfab5b8da42cedd68b20f3687 Mon Sep 17 00:00:00 2001 From: Adam Wildavsky Date: Thu, 23 Jul 2026 00:13:28 +0200 Subject: [PATCH 2/8] Reject --binary at checkout dtest when combined with --branch. Branch builds overwrite bazel-bin dtest, so that mix could benchmark the wrong executable after restore. Co-authored-by: Cursor --- python/tests/benchmark.py | 36 +++++++++++++++++++--- python/tests/benchmark_test.py | 56 ++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+), 5 deletions(-) diff --git a/python/tests/benchmark.py b/python/tests/benchmark.py index 8306a889..a3c7cde3 100755 --- a/python/tests/benchmark.py +++ b/python/tests/benchmark.py @@ -396,11 +396,13 @@ def usage_text() -> str: -- End benchmark options; remaining args are passed to dtest (e.g. -- -n 8 -r for 8 threads and slow-board report) ---branch and --binary may be given any number of times and combined freely; the -binaries are benchmarked in the order specified and the first is the baseline. The -current checkout is benchmarked by default only when neither flag is given. With two -binaries the summary adds a ratio and a "faster" note; with three or more it shows -only the per-binary averages (no note). +--branch and --binary may be given any number of times and combined; the binaries +are benchmarked in the order specified and the first is the baseline. --binary +must not point at the checkout's bazel-bin dtest when --branch is also used +(branch builds overwrite that path). The current checkout is benchmarked by +default only when neither flag is given. With two binaries the summary adds a +ratio and a "faster" note; with three or more it shows only the per-binary +averages (no note). Environment: BRANCH, BINARY, HANDS_DIR, REPEATS, MAX_DEALS, DRY_RUN, DETAILS, EPSILON @@ -437,6 +439,28 @@ def _git_out(root: Path, *args: str) -> str: return _git(root, *args).stdout.strip() +def reject_checkout_binary_with_branch( + root: Path, specs: Sequence[tuple[str, str]] +) -> None: + """Disallow --binary pointing at checkout dtest when --branch is used. + + --branch builds overwrite root/DTEST_REL, so an explicit --binary at that + path would run the wrong executable after the branch is restored. + """ + if not any(kind == "branch" for kind, _ in specs): + return + checkout_dtest = (root / DTEST_REL).resolve() + for kind, val in specs: + if kind != "binary": + continue + if Path(val).resolve() == checkout_dtest: + raise BenchmarkError( + f"--binary may not target the checkout's {DTEST_REL} when " + "--branch is used (branch builds overwrite that path); " + "copy the binary elsewhere or use --branch . for HEAD" + ) + + def git_prep_for_branches( root: Path, specs: list[tuple[str, str]] ) -> tuple[list[tuple[str, str]], str]: @@ -630,6 +654,8 @@ def build_binaries(self) -> tuple[list[str], list[Path]]: assert self.cfg.branch_binary is not None return [self.current_label()], [self.cfg.branch_binary] + reject_checkout_binary_with_branch(self.root, specs) + if nbranch > 0: specs, self.orig_branch = git_prep_for_branches(self.root, specs) diff --git a/python/tests/benchmark_test.py b/python/tests/benchmark_test.py index b9963bad..75848d81 100644 --- a/python/tests/benchmark_test.py +++ b/python/tests/benchmark_test.py @@ -320,6 +320,62 @@ def _setup_repo(repo: Path) -> None: _git(repo, "tag", "head-tag") +class TestRejectCheckoutBinaryWithBranch(unittest.TestCase): + def test_rejects_relative_checkout_dtest(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + old_cwd = Path.cwd() + try: + os.chdir(root) + with self.assertRaises(benchmark.BenchmarkError) as ctx: + benchmark.reject_checkout_binary_with_branch( + root, + [ + ("branch", "develop"), + ("binary", str(benchmark.DTEST_REL)), + ], + ) + finally: + os.chdir(old_cwd) + self.assertIn("checkout's", str(ctx.exception)) + self.assertIn(str(benchmark.DTEST_REL), str(ctx.exception)) + + def test_rejects_absolute_checkout_dtest(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + abs_path = root / benchmark.DTEST_REL + with self.assertRaises(benchmark.BenchmarkError) as ctx: + benchmark.reject_checkout_binary_with_branch( + root, + [("branch", "develop"), ("binary", str(abs_path))], + ) + self.assertIn("checkout's", str(ctx.exception)) + + def test_allows_external_binary_with_branch(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + benchmark.reject_checkout_binary_with_branch( + root, + [("branch", "develop"), ("binary", "/tmp/other-dtest")], + ) + + def test_allows_checkout_binary_without_branch(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + benchmark.reject_checkout_binary_with_branch( + root, + [("binary", str(root / benchmark.DTEST_REL))], + ) + + def test_allows_branch_only(self) -> None: + with tempfile.TemporaryDirectory() as tmp: + root = Path(tmp) + benchmark.reject_checkout_binary_with_branch( + root, + [("branch", "develop")], + ) + + class TestGitPrepForBranches(unittest.TestCase): def test_dirty_same_commit_allowed(self) -> None: with tempfile.TemporaryDirectory() as tmp: From 687bfd5cefe4ad39f73922d992974e6393b4e0ea Mon Sep 17 00:00:00 2001 From: Adam Wildavsky Date: Thu, 23 Jul 2026 07:57:24 +0200 Subject: [PATCH 3/8] Write alt-screen escapes to the injected out stream. Cleanup and main were hardcoding sys.stdout, so a redirected or custom output stream could miss the terminal teardown. Co-authored-by: Cursor --- python/tests/benchmark.py | 14 +++++++------- python/tests/benchmark_test.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 7 deletions(-) diff --git a/python/tests/benchmark.py b/python/tests/benchmark.py index a3c7cde3..e4cc1d5d 100755 --- a/python/tests/benchmark.py +++ b/python/tests/benchmark.py @@ -535,8 +535,8 @@ def __init__( def cleanup(self) -> None: if self.alt_screen_active: try: - sys.stdout.write(ALT_LEAVE) - sys.stdout.flush() + self.out.write(ALT_LEAVE) + self.out.flush() except OSError: pass self.alt_screen_active = False @@ -818,13 +818,13 @@ def main(argv: Sequence[str] | None = None, env: Mapping[str, str] | None = None if not cfg.dry_run: if cfg.details: show_run_lines = True - elif sys.stdout.isatty(): + elif runner.out.isatty(): show_run_lines = True alt_screen = True if alt_screen: - sys.stdout.write(ALT_ENTER) - sys.stdout.flush() + runner.out.write(ALT_ENTER) + runner.out.flush() runner.alt_screen_active = True def print_run_header() -> None: @@ -897,8 +897,8 @@ def print_run_row( ) if alt_screen: - sys.stdout.write(ALT_LEAVE) - sys.stdout.flush() + runner.out.write(ALT_LEAVE) + runner.out.flush() runner.alt_screen_active = False if not cfg.dry_run: diff --git a/python/tests/benchmark_test.py b/python/tests/benchmark_test.py index 75848d81..5bdf81b0 100644 --- a/python/tests/benchmark_test.py +++ b/python/tests/benchmark_test.py @@ -3,6 +3,7 @@ from __future__ import annotations +import io import os import subprocess import tempfile @@ -105,6 +106,36 @@ def test_basename(self) -> None: self.assertEqual(benchmark.label_for_path("/tmp/foo/dtest"), "dtest") +class TestRunnerCleanup(unittest.TestCase): + def test_alt_leave_goes_to_injected_out(self) -> None: + out = io.StringIO() + err = io.StringIO() + with tempfile.TemporaryDirectory() as tmp: + runner = benchmark.BenchmarkRunner( + Path(tmp), + benchmark.Config(), + out=out, + err=err, + ) + runner.alt_screen_active = True + with mock.patch("sys.stdout", new_callable=io.StringIO) as fake_stdout: + runner.cleanup() + self.assertEqual(out.getvalue(), benchmark.ALT_LEAVE) + self.assertEqual(fake_stdout.getvalue(), "") + self.assertFalse(runner.alt_screen_active) + + def test_cleanup_noop_when_alt_screen_inactive(self) -> None: + out = io.StringIO() + with tempfile.TemporaryDirectory() as tmp: + runner = benchmark.BenchmarkRunner( + Path(tmp), + benchmark.Config(), + out=out, + ) + runner.cleanup() + self.assertEqual(out.getvalue(), "") + + class TestRunOrder(unittest.TestCase): def test_default(self) -> None: self.assertEqual(benchmark.run_order(3, reverse=False), [0, 1, 2]) From 7b95821929b23d5a80a7a6519165d13d8273d84d Mon Sep 17 00:00:00 2001 From: Adam Wildavsky Date: Thu, 23 Jul 2026 08:04:25 +0200 Subject: [PATCH 4/8] Write failed build logs to the injected err stream. run_build was hardcoding sys.stderr, which broke the runner constructor contract for redirected or custom error output. Co-authored-by: Cursor --- python/tests/benchmark.py | 2 +- python/tests/benchmark_test.py | 24 ++++++++++++++++++++++++ 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/python/tests/benchmark.py b/python/tests/benchmark.py index e4cc1d5d..aa957331 100755 --- a/python/tests/benchmark.py +++ b/python/tests/benchmark.py @@ -596,7 +596,7 @@ def run_build(self, cmd: Sequence[str], *, cwd: Path | None = None) -> None: check=False, ) if proc.returncode != 0: - sys.stderr.write(self.build_log.read_text(encoding="utf-8", errors="replace")) + self.err.write(self.build_log.read_text(encoding="utf-8", errors="replace")) raise subprocess.CalledProcessError(proc.returncode, cmd) def bazel_dtest(self) -> None: diff --git a/python/tests/benchmark_test.py b/python/tests/benchmark_test.py index 5bdf81b0..da1e0754 100644 --- a/python/tests/benchmark_test.py +++ b/python/tests/benchmark_test.py @@ -136,6 +136,30 @@ def test_cleanup_noop_when_alt_screen_inactive(self) -> None: self.assertEqual(out.getvalue(), "") +class TestRunBuild(unittest.TestCase): + def test_failed_build_output_goes_to_injected_err(self) -> None: + err = io.StringIO() + with tempfile.TemporaryDirectory() as tmp: + runner = benchmark.BenchmarkRunner( + Path(tmp), + benchmark.Config(details=False), + err=err, + ) + + def fake_run(cmd, **kwargs): # type: ignore[no-untyped-def] + log = kwargs["stdout"] + log.write("build blew up\n") + log.flush() + return mock.Mock(returncode=1) + + with mock.patch("subprocess.run", side_effect=fake_run): + with mock.patch("sys.stderr", new_callable=io.StringIO) as fake_stderr: + with self.assertRaises(subprocess.CalledProcessError): + runner.run_build(["false"]) + self.assertIn("build blew up", err.getvalue()) + self.assertEqual(fake_stderr.getvalue(), "") + + class TestRunOrder(unittest.TestCase): def test_default(self) -> None: self.assertEqual(benchmark.run_order(3, reverse=False), [0, 1, 2]) From b68a2ecf8cc4159754e78c12d478ddc5231a2755 Mon Sep 17 00:00:00 2001 From: Adam Wildavsky Date: Thu, 23 Jul 2026 08:24:53 +0200 Subject: [PATCH 5/8] Show NA in summary when a binary lacks avg_user. Dropping the whole solver/file row hid partial results; keep the line and omit ratio/note until both sides are available. Co-authored-by: Cursor --- python/tests/benchmark.py | 24 +++++++++++++++------ python/tests/benchmark_test.py | 39 ++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 7 deletions(-) diff --git a/python/tests/benchmark.py b/python/tests/benchmark.py index aa957331..ea220175 100755 --- a/python/tests/benchmark.py +++ b/python/tests/benchmark.py @@ -231,18 +231,28 @@ def dash() -> None: for solver in SOLVERS: for fname in files: - if not all((solver, fname, b) in counts for b in range(nb)): + avgs: list[float | None] = [] + for b in range(nb): + key = (solver, fname, b) + if key in counts: + avgs.append(sums[key] / counts[key]) + else: + avgs.append(None) + if all(u is None for u in avgs): continue - avgs = [sums[(solver, fname, b)] / counts[(solver, fname, b)] for b in range(nb)] line = f"{solver:<6} {fname:<13}" for u in avgs: - line += f" {u:12.2f}" + if u is None: + line += f" {'NA':>12}" + else: + line += f" {u:12.2f}" if nb == 2: - if avgs[0] > 0: - # If avgs[0] ever becomes zero, we should switch dtest timing from + a0, a1 = avgs[0], avgs[1] + if a0 is not None and a1 is not None and a0 > 0: + # If a0 ever becomes zero, we should switch dtest timing from # milliseconds to microseconds. - r = avgs[1] / avgs[0] - if within_epsilon(avgs[0], avgs[1], epsilon): + r = a1 / a0 + if within_epsilon(a0, a1, epsilon): note = "equal" elif r >= 1: note = f"{Lf(0)} faster" diff --git a/python/tests/benchmark_test.py b/python/tests/benchmark_test.py index da1e0754..331b52fc 100644 --- a/python/tests/benchmark_test.py +++ b/python/tests/benchmark_test.py @@ -325,6 +325,45 @@ def test_zero_baseline_avg_skips_ratio(self) -> None: self.assertNotIn("faster", solve_line) self.assertNotIn("equal", solve_line) + def test_missing_avg_prints_na_and_keeps_row(self) -> None: + rows = [ + benchmark.ResultRow("solve", "list1.txt", 0, 1, 100.0, 1.0, 1.0, 1.0, 1.0), + # Incomplete dtest output: avg_user missing for binary 1 + benchmark.ResultRow("solve", "list1.txt", 1, 1, None, None, None, None, 0.5), + ] + text = benchmark.format_summary( + rows, + labels=["base", "other"], + files=["list1.txt"], + epsilon=0.5, + ) + solve_line = next( + line for line in text.splitlines() if line.startswith("solve ") + ) + self.assertRegex(solve_line, r"\b1\.00\b") + self.assertRegex(solve_line, r"\bNA\b") + self.assertNotRegex(solve_line, r"\d+\.\d+x") + self.assertNotIn("faster", solve_line) + self.assertNotIn("equal", solve_line) + + def test_missing_avg_suppresses_ratio_either_side(self) -> None: + rows = [ + benchmark.ResultRow("solve", "list1.txt", 0, 1, None, None, None, None, 0.1), + benchmark.ResultRow("solve", "list1.txt", 1, 1, 50.0, 1.0, 0.5, 1.0, 0.5), + ] + text = benchmark.format_summary( + rows, + labels=["base", "other"], + files=["list1.txt"], + epsilon=0.5, + ) + solve_line = next( + line for line in text.splitlines() if line.startswith("solve ") + ) + self.assertRegex(solve_line, r"\bNA\b") + self.assertRegex(solve_line, r"\b0\.50\b") + self.assertNotRegex(solve_line, r"\d+\.\d+x") + class TestIsDdsRoot(unittest.TestCase): def test_valid(self) -> None: From 4470ae8f9d397598ffdd1c5952a4a38103894cef Mon Sep 17 00:00:00 2001 From: Adam Wildavsky Date: Thu, 23 Jul 2026 21:25:25 +0100 Subject: [PATCH 6/8] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- python/tests/benchmark.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/python/tests/benchmark.py b/python/tests/benchmark.py index ea220175..f05c95f1 100755 --- a/python/tests/benchmark.py +++ b/python/tests/benchmark.py @@ -632,7 +632,7 @@ def build_branch_binary(self, name: str, dest: Path) -> None: dest.chmod(dest.stat().st_mode | 0o111) def new_tmp_bin(self) -> Path: - fd, name = tempfile.mkstemp(prefix="dds-dtest-bin.") + fd, name = tempfile.mkstemp(prefix="dds-dtest-bin.", suffix=".exe" if os.name == "nt" else "") os.close(fd) path = Path(name) self.tmp_bins.append(path) From 8903b278a07ff2643ddf32b8782c39f32ca1b4d5 Mon Sep 17 00:00:00 2001 From: Adam Wildavsky Date: Thu, 23 Jul 2026 22:30:40 +0200 Subject: [PATCH 7/8] Use dtest.exe path on Windows for Bazel binary lookup. Bazel emits dtest.exe on nt, so the hardcoded dtest path broke the default baseline and checkout-binary overlap check on Windows. Co-authored-by: Cursor --- python/tests/benchmark.py | 10 +++++++++- python/tests/benchmark_test.py | 17 +++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/python/tests/benchmark.py b/python/tests/benchmark.py index f05c95f1..fdd883ef 100755 --- a/python/tests/benchmark.py +++ b/python/tests/benchmark.py @@ -43,11 +43,19 @@ from typing import Mapping, Sequence, TextIO SOLVERS = ("solve", "calc") -DTEST_REL = Path("bazel-bin/library/tests/dtest") ALT_ENTER = "\033[?1049h\033[H\033[2J" ALT_LEAVE = "\033[?1049l" +def dtest_rel(*, os_name: str = os.name) -> Path: + """Relative path to Bazel's dtest binary for the given OS.""" + name = "dtest.exe" if os_name == "nt" else "dtest" + return Path(f"bazel-bin/library/tests/{name}") + + +DTEST_REL = dtest_rel() + + class BenchmarkError(Exception): """User-facing configuration or validation error.""" diff --git a/python/tests/benchmark_test.py b/python/tests/benchmark_test.py index 331b52fc..af65a739 100644 --- a/python/tests/benchmark_test.py +++ b/python/tests/benchmark_test.py @@ -106,6 +106,23 @@ def test_basename(self) -> None: self.assertEqual(benchmark.label_for_path("/tmp/foo/dtest"), "dtest") +class TestDtestRel(unittest.TestCase): + def test_posix_path_omits_exe(self) -> None: + self.assertEqual( + benchmark.dtest_rel(os_name="posix"), + Path("bazel-bin/library/tests/dtest"), + ) + + def test_windows_path_uses_exe(self) -> None: + self.assertEqual( + benchmark.dtest_rel(os_name="nt"), + Path("bazel-bin/library/tests/dtest.exe"), + ) + + def test_module_constant_matches_current_platform(self) -> None: + self.assertEqual(benchmark.DTEST_REL, benchmark.dtest_rel()) + + class TestRunnerCleanup(unittest.TestCase): def test_alt_leave_goes_to_injected_out(self) -> None: out = io.StringIO() From 16f05eb0e72d188d3369e51c0e6f8f4340d673c2 Mon Sep 17 00:00:00 2001 From: Adam Wildavsky Date: Thu, 23 Jul 2026 21:37:50 +0100 Subject: [PATCH 8/8] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- python/tests/benchmark.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/python/tests/benchmark.py b/python/tests/benchmark.py index fdd883ef..8f3d19b8 100755 --- a/python/tests/benchmark.py +++ b/python/tests/benchmark.py @@ -486,10 +486,12 @@ def git_prep_for_branches( Returns (resolved_specs, orig_branch). """ - probe = _git(root, "rev-parse", "--is-inside-work-tree", check=False) + try: + probe = _git(root, "rev-parse", "--is-inside-work-tree", check=False) + except OSError as e: + raise BenchmarkError("--branch requires git to be installed and on PATH") from e if probe.returncode != 0: raise BenchmarkError(f"--branch requires a git work tree at {root}") - sym = _git(root, "symbolic-ref", "--quiet", "--short", "HEAD", check=False) if sym.returncode == 0 and sym.stdout.strip(): orig_branch = sym.stdout.strip()