Skip to content

Fix Moli benchmark harness and add opt-in layout recovery - #25

Open
lanyue-llk wants to merge 35 commits into
mainfrom
codex/moli-benchmark-harness-fix
Open

lanyue-llk wants to merge 35 commits into
mainfrom
codex/moli-benchmark-harness-fix

Conversation

@lanyue-llk

@lanyue-llk lanyue-llk commented Sep 17, 2026 •

Copy link
Copy Markdown

Summary

Consolidate the Moli benchmark infrastructure and fixed-layout comparison tooling. No Moli source code or binary is modified.

Infrastructure and reproducibility

  • Fix Selenium process tracking and cleanup on macOS, and handle framework download timeouts without unhandled promise rejection.
  • Route Schema.* CDP probes through the page session unless explicitly browser-scoped. Preserve historical measured results while correcting routing notes.
  • Freeze the full 372-task Moli 0.1.1 failure cohort, independent of Chrome outcomes. Bind binary, task, dependency and execution identities for version comparisons.

Fixed layout only

  • Layout is off by default; --moli-layout off and --moli-layout on select one mode for the entire run.
  • Remove --try-layout, failed-case rescheduling, replacement matrices, recovery receipts, retry-only artifact paths, the recovery summarizer and its design documentation.
  • Every case runs its configured repetitions, normally three, whether it passes or fails. Failure never changes layout or adds executions.
  • Fixed-layout summaries read ordinary complete results.jsonl runs. The CLI accepts repeated --source RUN_DIR arguments for disjoint task batches, preserving layout and binary checks.
  • Retained mixed-layout recovery evidence is explicitly rejected by fixed-layout reports; existing measurements are not deleted or rewritten.

Evidence and reports

  • Keep one result per engine/task/attempt and require all three attempts to pass for a successful case.
  • Validate fixed-layout source hashes, task coverage, binary identity, individual attempts and retained error logs.
  • Keep resource populations separate from the 372-case accuracy cohort. Resource comparisons and published measurements are maintained in browser-eval PR #27.
  • benchmarks/moli-0.1.1-failure-task-ids.txt remains the original 372-case input cohort. New-version successes do not prune that historical definition.

Verification

  • Real Moli 1.1.9 release: three cases × three attempts × two fixed layouts = 18 executions. No retry files or additional executions.
Case Fixed off Fixed on
pw_raw_browser_getversion 3/3 3/3
v2_diag_gbcr_fixed 0/3 3/3
v4_cdp_emulation_setemulatedostextscale 0/3 0/3
  • Full Linux harness CI: 631 passed, 10 skipped on commit 1ab9c55.
  • Focused CLI, fixed-layout, summary, cohort and manifest checks pass. Direct summary CLI invocation from another working directory is covered.
  • Removed switch and auto mode are rejected. Fixed off/on remain accepted; legacy mixed-layout evidence is rejected.
  • No benchmark accuracy rerun, no measured output or binaries committed, no Moli changes or merges.

@lanyue-llk
lanyue-llk marked this pull request as ready for review September 17, 2026 15:32
@lanyue-llk lanyue-llk changed the title Fix Moli cohort infrastructure failures and freeze version comparisons Fix Moli benchmark infrastructure and CDP target routing Sep 18, 2026
@lanyue-llk

Copy link
Copy Markdown
Author

Review: no blocking findings in #25

The stated goal matches the changed paths: the macOS Selenium process tracking addresses the platform-specific infrastructure failure; the download progress helper attaches a rejection handler before awaiting a potentially slow click and removes the CDP listener; routing Schema.* to the page session corrects the harness target rather than changing the browser's claimed capability. The cohort tooling freezes the historical selection and checks complete attempts and recorded comparison conditions. Keeping the historical result rows intact while correcting their interpretation is appropriate.

I reviewed the runner, adapter, cohort comparator, task correction, and relevant tests. The consolidated CI check passes. Locally, 26 download/CDP/cohort tests passed on the #27 tree (which contains #25 unchanged), and both PR diffs pass git diff --check. I did not run a complete binary-backed cohort here; the PR's existing macOS and full-cohort evidence supplies that coverage. #27 has a separate actionable automatic-layout classifier finding and should be resolved before the stacked change is merged.

@lanyue-llk lanyue-llk changed the title Fix Moli benchmark infrastructure and CDP target routing Fix Moli benchmark harness and add opt-in layout recovery Sep 18, 2026
Comment thread runner/run.py Outdated
reporter.phase(f"Failed after {reporter.completed_rows}/{reporter.total_rows} result rows")
raise BenchError("parallel run failed for some attempts:\n" + "\n".join(errors[:10]))
reporter.finish()
if "moli" in selected_engines and args.moli_layout == "off" and getattr(args, "try_layout", False):

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

代码写的有点丑啊,一大堆

Comment thread benchmarks/moli-0.1.1-failure-cohort.json
Comment thread test/test_download_completion.py Outdated

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

pr 上的修改文件怎么那么多?确定都需要吗

@lanyue-llk

Copy link
Copy Markdown
Author

Chromium baseline correction

The two Chrome 151 non-passes in four_engine_full_20260812 were benchmark defects rather than Chromium capability failures:

  • pw_raw_schema_getdomains: the harness sent Schema.getDomains to the browser root. The corrected page-session route passes.
  • pw_raw_browser_getbrowsercommandline: Chrome exposes this diagnostic command only when launched with --enable-automation. The task now declares a scoped browser_automation launch profile; the corrected task passes.

Targeted verification used Chrome for Testing 151.0.7922.47 and ran each formerly failing task once. Both passed. The other 1,926 historical Chrome task results were not rerun. This yields a corrected Chromium baseline of 1,928 / 1,928 (100.00%), while the original immutable run remains 1,926 / 1,928 (99.90%) in historical evidence.

Verification:

  • 155 related tests passed.
  • Repository validation passed for all 1,928 task definitions.
  • PR CI harness passed.
  • The official Linux Chrome binary was independently downloaded and matched the historical SHA-256 3b0be9872ea937893cb1e1523fde071d38c1ed4ef866b3f7976240094a868c93. The targeted executions were on macOS arm64 with the same Chrome version; they are not represented as a new Linux full run.

@lanyue-llk lanyue-llk left a comment •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Independent review — 1ab9c552077b2d25d1333583c3c3881e8ddb4997

Reviewed the PR diff and the current fixed --moli-layout off/on execution, attempt summaries, resource collection, and evidence validation. I found two reproducible issues:

[P2] Normalize the cohort run ID before deriving paths and receipts

tools/run_moli_cohort.py:82–83 builds its run directory and conditions receipt from the original argument, although line 69 accepts hyphens and arbitrary lengths. The invoked runner applies compact_run_id() in reserve_run_dir(). Directly exercising that real reservation function gives:

  • release-1_1_9 → release_1_1_9; the wrapper's expected directory does not exist.
  • An 80-character x ID → xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx_d929cdee; the wrapper's expected directory does not exist.

Consequently the benchmark can finish successfully, then the wrapper fails while opening run_manifest.json, with its conditions receipt stored under a different ID. Case folding also changes the receipt's recorded identity even on a case-insensitive filesystem. Normalize once with the runner's canonical function before collision checks, receipt creation, and command construction, or reject noncanonical IDs before any mutation. Add a wrapper-level regression covering a hyphenated ID and a compacted ID.

[P2] Bind each resource measurement to the certified binary

tools/summarize_moli_macos_resources.py:144–145 validates each row's layout but never checks its engine_provenance.binary_sha256 against the manifest/receipt. Using the existing two-task resource fixture, changing one off-run row's binary hash from 999…999 to aaa…aaa, and calling summarize_fixed_pairs() still returns quality.publishable: true and attributes that measurement to 999…999.

A stale or mixed-binary results file is therefore accepted as the certified candidate. Check every row's binary hash against the certified identity and reject missing identities as well. The fixed-layout accuracy summarizer already performs this check. Add negative controls for both mismatched and missing row hashes, alongside the valid cohort case.

Validation performed: 127 passed, 8 skipped across test_moli_fixed_layout.py, test_summarize_moli_fixed_layout.py, test_summarize_moli_macos_resources.py, test_framework_driver.py, and test_resources.py. The two reproductions above were run independently in temporary directories; no repository or measured evidence was changed. No additional actionable finding in the removal of layout retries itself.

Reproduction entry points, from the repository root with its Python dependencies installed:

from pathlib import Path
from tempfile import TemporaryDirectory
from runner.run import reserve_run_dir
with TemporaryDirectory() as tmp:
    print(reserve_run_dir(Path(tmp), "release-1_1_9", "error")[0])
    print((Path(tmp) / "release-1_1_9").exists())  # False

For the second finding, use the _run, _receipt, _protocol, and _write_jsonl fixtures in test/test_summarize_moli_macos_resources.py; create the off/on runs, change off/results.jsonl row 0's engine_provenance.binary_sha256, and call summarize_fixed_pairs(off, on, receipt, protocol, expected_tasks=2, expected_frozen_calls=5). The returned quality.publishable is True despite the mismatch.

@lanyue-llk

Copy link
Copy Markdown
Author

已处理独立 review 的两项问题,修复提交 5c620b5:

  1. run_moli_cohort.py 在任何目录/收据操作前复用底层 compact_run_id()。带连字符、大小写/重复下划线和超长 ID 的命令、结果目录、conditions 收据使用同一个名称,重复执行仍拒绝覆盖。
  2. 资源汇总逐条检查 engine_provenance.binary_sha256 与本次 binary receipt 一致;缺失或其他构建的哈希立即拒绝,不再进入可发布汇总。

新增五个参数化回归。独立审查 agent 已复核修改,相关 23 项测试通过;原“错误 binary 仍 publishable”的复现现在按预期抛出 row binary identity mismatch。未重跑评测或改写历史结果。

Scroll agent-browser and Ferrum targets into view before clicking
* Apply automation profile to Moli and set explicit viewport

* test: cover automation profiles for supported engines

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant