Truer comparisons and new --wasm_branch flag for benchmark.py

[tameware]

No description provided.

[Copilot]

Pull request overview

This PR updates the Python benchmark harness for DDS’ dtest to (1) support benchmarking the WASM build via Node and (2) improve cross-binary comparisons by computing TOTAL as a deal-weighted user-time average rather than summing wall-clock time.

Changes:

• Add --wasm_branch / --wasm-branch to build //wasm:dtest_wasm from a git ref, run it under Node, and include it in mixed native/WASM comparisons.
• Adjust summary “TOTAL” computation to use sum(user_ms) / sum(deals) (deal-weighted) for more meaningful aggregate comparisons across hand files.
• Expand unit tests to cover WASM timing parsing (sys_ms = n/a), new CLI flag parsing, git ref handling, and Node invocation behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
python/tests/benchmark.py	Adds WASM branch support, Node execution path, and updates summary aggregation logic.
python/tests/benchmark_test.py	Adds/updates tests for WASM parsing, new flags, git handling, and WASM runner behavior.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

python/tests/benchmark.py:270

• format_summary() no longer uses ResultRow.wall_s (TOTAL was switched from wall totals to per-deal user-time averages), but run_dtest() still measures wall time and ResultRow still stores wall_s. This looks like dead data now and increases complexity (including in tests that have to pass a placeholder value). Consider removing wall_s from ResultRow and dropping the wall-time measurement/return value, or reintroducing a wall-time summary column if it’s still needed.

    # Per-solver totals for overall avg user ms = sum(user_ms) / sum(deals).
    total_user: dict[str, list[float]] = {s: [0.0] * nb for s in SOLVERS}
    total_deals: dict[str, list[int]] = {s: [0] * nb for s in SOLVERS}
    user_seen: dict[str, list[bool]] = {s: [False] * nb for s in SOLVERS}

python/tests/benchmark.py:214

• dtest_timing_usable() currently returns true whenever user_ms is present, but run_dtest() uses it to decide whether to warn about incomplete timing output. As written, output that has User time (ms) but lacks enough information to compute avg_user (ms/deal) will not warn and will later render as NA in the per-file summary. Consider requiring avg_user as well (sys time can still be optional for wasm).

This issue also appears on line 267 of the same file.

def dtest_timing_usable(parsed: DtestTiming) -> bool:
    """True when output has the user timing the summary needs.

    Sys time may be n/a on platforms without a process CPU clock (e.g. wasm32).
    """
    return parsed.user_ms is not None

[tameware]

@copilot please re-review after 4af1481: removed dead wall_s; dtest_timing_usable now requires avg_user as well as user_ms.

[tameware]

Requesting Copilot re-review of the latest changes.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

python/tests/benchmark.py:916

• When running a .js (wasm) binary, run_dtest() sets cwd=binary.parent, but -f currently passes hands as-is. If HANDS_DIR is a relative path (allowed via env), that relative hands path will be resolved against the wasm temp dir / bazel-bin/wasm rather than the repo root, causing dtest_wasm to fail to find the hand file. Passing an absolute/normalized hand-file path avoids this discrepancy between native and wasm runs.

    def dtest_command(self, binary: Path, solver: str, hands: Path) -> list[str]:
        args = ["-f", str(hands), "-s", solver, *self.cfg.dtest_extra]
        if binary.suffix == ".js":
            return ["node", str(binary), *args]
        return [str(binary), *args]

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

python/tests/benchmark.py:283

• The TOTAL row weights overall avg user ms by deals_from_hand_file(row.file), which assumes the number of hands processed always matches the listN.txt filename. But parse_dtest_output() already parses "Number of hands" and can derive avg_user from that; if a hand file’s contents don’t match its name (or dtest runs fewer hands than the filename implies), the TOTAL averages become incorrect. Consider carrying the parsed hand count through DtestTiming/ResultRow (or otherwise using the dtest-reported count) and using that for weighting.

        deals = deals_from_hand_file(row.file)
        if (
            row.user_ms is not None
            and deals is not None
            and deals > 0

python/tests/benchmark.py:917

• When benchmarking a wasm dtest passed via --binary as a relative .js path (e.g. bazel-bin/wasm/dtest.js), run_dtest() sets cwd=binary.parent but still passes the full relative script path to node. That makes Node resolve the script relative to cwd (effectively cwd/bazel-bin/wasm/dtest.js), which will fail to find the file. Using an absolute script path avoids this and still keeps cwd pointing at the .wasm sibling.

        # Absolute -f so wasm runs (cwd = js parent) still find HANDS_DIR paths.
        args = ["-f", str(hands.resolve()), "-s", solver, *self.cfg.dtest_extra]
        if binary.suffix == ".js":
            return ["node", str(binary), *args]
        return [str(binary), *args]

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

python/tests/benchmark.py:299

• TOTAL aggregation currently includes rows as long as user_ms is present (even when avg_user is missing). This can produce a numeric TOTAL cell for a binary where the per-file row prints NA (avg_user=None) and where run_dtest already warned that timing output is incomplete. To keep the summary consistent and avoid totals being computed from partial timing output, only include rows that have usable user timing (at least avg_user) when accumulating total_user/total_deals.

        if (
            row.user_ms is not None
            and deals is not None
            and deals > 0
            and row.solver in total_user
        ):
            total_user[row.solver][row.bin_idx] += row.user_ms
            total_deals[row.solver][row.bin_idx] += deals
            user_seen[row.solver][row.bin_idx] = True

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

python/tests/benchmark.py:290

• In TOTAL aggregation, a parsed row.hands value of 0 is treated as “missing” and replaced with the deal count inferred from the filename (e.g. list1.txt => 1). If dtest legitimately reports 0 hands processed (e.g. empty/invalid hand file), this will incorrectly include that row in totals and can render a misleading 0.00 ms/deal instead of excluding the row (or showing NA).

        deals = (
            row.hands
            if row.hands is not None and row.hands > 0
            else deals_from_hand_file(row.file)
        )

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.