hardcoded /tmp paths prevent concurrent eval runs plus various m3 issues#121
hardcoded /tmp paths prevent concurrent eval runs plus various m3 issues#121haroldship wants to merge 9 commits into
Conversation
Console log, FINAL SUMMARY hand-off file, and outer registry log all lived at fixed /tmp paths, so two concurrent m3 runs on one host interleaved and clobbered each other's artifacts (and bundles staged the mixed content). eval.sh now resolves M3_RUN_TMP_DIR (pre-set env wins, else a private mktemp dir per run) and keeps console/summary/registry logs inside it; eval_m3.py reads M3_SUMMARY_FILE from the environment. compare.sh exports one M3_RUN_TMP_DIR for all its sequential eval.sh runs and stages logs from there, preserving the truncate-per-run semantics. run_groundedness_ab.sh moves its arm marker into the same dir; run_with_container.sh gains an M3_REGISTRY_LOG override. create_eval_bundle.py checks the run-scoped dir first and keeps the legacy fixed paths as fallback for logs left by older runs. Other benchmarks share the same pattern (see #115) and are out of scope here.
--m3-data accepts a directory of input/output files, but the bundle task-copy/hash logic assumed a single file and crashed with IsADirectoryError via shutil.copy2/open(). Add shared helpers that copytree directories and hash their contents recursively.
run_config_mode's except (KeyboardInterrupt, asyncio.CancelledError) branch has fired unattended across several bundles (2026-06-24, 2026-07-04, 2026-07-05, 2026-07-09), always seconds after a call_model log line, with no human present. That branch alone can't tell a real SIGINT apart from a bare CancelledError raised inside the process. Add a SIGINT observer that timestamps real signal delivery without changing Ctrl-C behavior (#91, #92), and a periodic stall watchdog that dumps every live asyncio task's stack so a silent hang (no timeout, no exception) leaves a trace of where execution is blocked.
… bundle evaluate_single_task wrote _vakra/prediction/<domain>.json (and, in GT mode, _vakra/groundtruth/) to the shared benchmarks/m3/results/ directory regardless of experiment. Two concurrent eval.sh runs for different experiments hitting the same domain name could clobber each other's prediction file there, and old runs' files were never cleared. Use bundle_dir/results when a workspace bundle exists (same fallback pattern _finalize_and_save_results already uses), so each named experiment gets its own isolated, self-cleaning directory tree and the files land inside the bundle automatically. Legacy (un-experimented) runs keep the old shared path.
evaluate_single_task re-derived `domains` from m3_data_loader.available_domains(task_id) and unconditionally overwrote the caller's value with the full domain list for the task. That discarded the single-domain narrowing rewrite_config_with_loader_ domains + expand_registry_config already apply upstream specifically so the sequential per-service loop can start one registry per domain. The first call (e.g. domain "address") would then try to walk every domain for the task using only the registry actually started for the first one, so every domain after it had zero tools registered. Observed on cap3-guard-test: address completed fine, then airline failed with "Application 'airline' not found in registry" / "Loaded 0 tools for 'airline'".
|
Warning Review limit reached
Next review available in: 5 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughBundle assembly now supports directory task sources with directory-aware hashes. M3 scripts use run-scoped artifact paths, evaluation outputs are bundle-scoped, and interrupt/stall diagnostics plus regression tests were added. ChangesDirectory task sources
M3 execution isolation and diagnostics
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ures (#122) * fix(deps): bump pillow and json-repair to clear CI security-scan failures CI's pip-audit step was failing (blocking PR #120 and #121) because two dependencies had known security bugs with fixes already published. - pillow 12.2.0 -> 12.3.0, json-repair 0.59.10 -> 0.61.4 (both just needed a lockfile bump, no code changes required) - setuptools also has a known bug (fixed in 83.0.0), but torch pins it to "<82" so we can't move it ourselves; added it to the security recipe's ignore list with a comment, matching the existing docling/torch entries * fix(ci): ignore setuptools CVE in the CI workflow's pip-audit too CodeRabbit caught that the CI workflow runs its own copy of the pip-audit command instead of `just security`, so the setuptools ignore added in the justfile wasn't actually reaching CI — the Security check was still red on this PR. Mirrored the same --ignore-vuln flag and explanation into ci.yml.
haroldship
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
scripts/create_eval_bundle.py (1)
67-76: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDeduplicate log paths to prevent passing duplicate logs to the bundle.
If both run-scoped logs and legacy
/tmplogs exist, this loop will append all of them tologs. This results in duplicate log file basenames being passed to the bundle creation process (e.g., twom3_registry.logfiles from different directories).Consider stopping at the first found instance of each log type so that stale
/tmplogs are ignored when the run-scoped logs are present.♻️ Proposed refactor
- fallbacks: list[Path] = [] run_tmp_dir = os.getenv("M3_RUN_TMP_DIR") - if run_tmp_dir: - fallbacks.append(Path(run_tmp_dir) / "m3_registry.log") - fallbacks.append(Path(run_tmp_dir) / "m3_console.log") - fallbacks.append(Path("/tmp/m3_registry.log")) # noqa: S108 # nosec B108 - fallbacks.append(Path("/tmp/m3_console.log")) # noqa: S108 # nosec B108 - for fallback in fallbacks: - if fallback.is_file() and fallback.stat().st_size > 0: - logs.append(fallback) + for log_name in ("m3_registry.log", "m3_console.log"): + candidates = [] + if run_tmp_dir: + candidates.append(Path(run_tmp_dir) / log_name) + candidates.append(Path("/tmp") / log_name) # noqa: S108 # nosec B108 + + for candidate in candidates: + if candidate.is_file() and candidate.stat().st_size > 0: + logs.append(candidate) + break🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/create_eval_bundle.py` around lines 67 - 76, Update the fallback log collection loop in create_eval_bundle so it selects at most one existing path for each log type, preferring the run-scoped M3_RUN_TMP_DIR files over legacy /tmp files. Stop searching for a log type once its first preferred instance is found, ensuring logs contains no duplicate m3_registry.log or m3_console.log entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/helpers/bundle.py`:
- Around line 115-141: Update _task_source_hash to hash directory files
incrementally in chunks instead of calling p.read_bytes(), reusing the
chunked-reading approach already established by _file_sha256 while preserving
the relative-path ordering and hash output format.
In `@benchmarks/m3/eval.sh`:
- Around line 215-219: The cleanup and bundle argument construction in
create_bundle must tolerate execution before REGISTRY_LOG and CONSOLE_LOG are
initialized. At benchmarks/m3/eval.sh lines 215-219, collect only existing
registry and non-empty console logs before appending --log-file entries; at
lines 297-301, apply the same guarded collection and add --log-files only when
logs exist. Also guard the CONSOLE_LOG reference in the line 250 echo with a
default such as “<not set>”.
In `@benchmarks/m3/tests/test_interrupt_diagnostics.py`:
- Around line 42-57: Update
test_sigint_observer_timestamps_signal_and_preserves_default_behavior to
temporarily install signal.default_int_handler for SIGINT before calling
eval_m3._install_sigint_observer(), then restore the prior handler in cleanup.
Keep the existing KeyboardInterrupt and observer-warning assertions unchanged.
---
Nitpick comments:
In `@scripts/create_eval_bundle.py`:
- Around line 67-76: Update the fallback log collection loop in
create_eval_bundle so it selects at most one existing path for each log type,
preferring the run-scoped M3_RUN_TMP_DIR files over legacy /tmp files. Stop
searching for a log type once its first preferred instance is found, ensuring
logs contains no duplicate m3_registry.log or m3_console.log entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8bd6e4aa-d0f4-4039-84a3-2a2d508164e0
📒 Files selected for processing (11)
benchmarks/helpers/bundle.pybenchmarks/helpers/tests/test_bundle_workspace.pybenchmarks/m3/compare.shbenchmarks/m3/eval.shbenchmarks/m3/eval_m3.pybenchmarks/m3/run_groundedness_ab.shbenchmarks/m3/run_with_container.shbenchmarks/m3/tests/test_interrupt_diagnostics.pybenchmarks/m3/tests/test_sequential_domain_scoping.pybenchmarks/m3/tests/test_vakra_output_scoping.pyscripts/create_eval_bundle.py
- guard unbound CONSOLE_LOG/REGISTRY_LOG in eval.sh's cleanup-trap path - read directory task-source files in chunks in _task_source_hash
Fixes Applied SuccessfullyFixed 2 file(s) based on 2 CodeRabbit feedback item(s). (A third item, on Files modified:
Commit: The latest autofix changes are on the |
_install_sigint_observer currently calls signal.default_int_handler directly, so this isn't strictly needed today, but guards against a future change to delegate to whatever handler was previously installed.
Follow-up: also applied issue #3 defensivelyOn reflection, applied the third CodeRabbit finding too ( File modified: Commit: All 3 review threads on this PR are now addressed. |
1 participant
Summary
If you run two M3 evals at the same time on one machine — or run
compare.sh, which runs several one after another — they were all writing their logs and hand-off files to the same fixed spots in/tmp. Two runs writing to the same file at the same time step on each other: logs get mixed together, and a run's results bundle can end up staged with the wrong content.While tracking that down, a handful of other bugs turned up along the way. Here's everything in this PR, in plain terms:
e0993c2) — console logs, theFINAL SUMMARYhand-off file, and the registry log all lived at fixed/tmppaths shared by every run. Fix: each run now gets its own private folder (M3_RUN_TMP_DIR— a pre-set env var wins, otherwise a freshmktempdir is created), andeval.sh,compare.sh,run_groundedness_ab.sh, andrun_with_container.shall write there instead of the shared path. The bundle-builder (create_eval_bundle.py) checks the new per-run folder first and only falls back to the old shared path for logs left by older runs.7186ba7) —--m3-datacan point at a folder instead of a single file, but the code that copies and hashes task data assumed a single file and crashed withIsADirectoryError. Fixed with shared helpers that handle folders too.f66de56, diagnostics only, no behavior change) — when a run died with no one watching, the code couldn't tell a real Ctrl-C apart from an internal cancellation, and gave no clue where it got stuck. Added a way to timestamp real signals and periodically dump what every background task is doing, so a silent hang leaves a trail.35b50ec) — prediction/ground-truth files were written to one shared results folder regardless of which experiment was running, and old runs' files were never cleaned up. Now each named experiment gets its own isolated, self-cleaning folder; old (un-named) runs keep the previous shared path.fe3545a) — a spot in the code was recomputing the full domain list for a task and overwriting the narrowed-down list you actually meant to run. So the first domain (e.g. "address") would complete fine, and every domain after it would fail with "Application 'airline' not found in registry" — because only the first domain's registry was ever started. Observed oncap3-guard-test.Fixes #115.
Tests
uv run --no-sync pytest benchmarks/m3/tests/ benchmarks/helpers/tests/ -m sanity: 107 passed, 2 skipped (Vakra vendor not installed, pre-existing and unrelated), 0 failures.Test plan for reviewers
uv run --no-sync pytest benchmarks/m3/tests/ benchmarks/helpers/tests/ -m sanity./benchmarks/m3/eval.sh --m3-datainvocations on one host and confirm no artifact clobberingSummary by CodeRabbit
New Features
Bug Fixes