Add portable benchmark run artifacts - #310
Conversation
|
@RitwijParmar is attempting to deploy a commit to the Coval Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughAdds an optional Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
runner/src/coval_bench/runner/artifacts.py (1)
152-156: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEnsure defensive type checking for
transcript.To remain consistent with the
isinstance(transcript, str)check used fortranscript_charsin_result_row, consider handling non-string values safely here to prevent a potentialAttributeErrorfrom.encode()if the database model ever yields unexpected data.🛡️ Proposed fix
def _transcript_sha256(transcript: str | None) -> str | None: - if transcript is None: + if not isinstance(transcript, str): return None return hashlib.sha256(transcript.encode("utf-8")).hexdigest()🤖 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 `@runner/src/coval_bench/runner/artifacts.py` around lines 152 - 156, Update _transcript_sha256 to validate that transcript is a string before calling encode; return None for None or other non-string values, while preserving the existing SHA-256 calculation for valid strings.
🤖 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 `@runner/README.md`:
- Around line 49-52: Update the portable JSONL artifact command in the runner
README to bind-mount the host artifact directory to the container path used by
RUN_ARTIFACT_DIR, ensuring artifacts survive the --rm container lifecycle and
remain accessible on the host.
---
Nitpick comments:
In `@runner/src/coval_bench/runner/artifacts.py`:
- Around line 152-156: Update _transcript_sha256 to validate that transcript is
a string before calling encode; return None for None or other non-string values,
while preserving the existing SHA-256 calculation for valid strings.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dc972584-600a-4d19-824c-233f6a3386bc
📒 Files selected for processing (6)
runner/README.mdrunner/src/coval_bench/config.pyrunner/src/coval_bench/runner/artifacts.pyrunner/src/coval_bench/runner/orchestrator.pyrunner/tests/unit/test_orchestrator.pyrunner/tests/unit/test_run_artifacts.py
| docker compose run --rm runner coval-bench run --smoke --kind tts | ||
|
|
||
| # Optional: write a portable JSONL artifact for the run: | ||
| mkdir -p artifacts |
There was a problem hiding this comment.
Make mount writable first. On Linux,
mkdir -p artifacts normally creates a 0755 directory owned by the host user, while the image runs as UID/GID 65532:65532. The bind mount preserves host ownership, so the runner cannot create the temporary JSONL file. The write then logs run_artifact_write_failed, and the documented command exits without the artifact. The example must prepare a directory writable by the container user or run the container with a compatible UID/GID.
| success_count=success_count, | ||
| fail_count=fail_count, | ||
| ) | ||
| await _write_run_artifact_if_enabled( |
There was a problem hiding this comment.
Avoid partial re-finalization. The run has already been persisted with
final_status before this await. If SIGTERM arrives while asyncio.to_thread is running, CancelledError enters the SIGTERM branch and calls finish_run again with RunStatus.PARTIAL. A fully completed run can therefore be overwritten as partial solely because shutdown began during best-effort artifact output. Cancellation after finalization should not enter the partial-run finalization path.
|
The artifact is useful because a failed voice benchmark usually gets debugged after the run is gone. The next piece I would add is a replay command that takes one JSONL artifact and re-runs only the failed cases with the same provider settings and artifact schema. That would let someone compare a provider failure against a later run without touching the whole dataset. I would keep it opt-in and make the artifact self-describing enough to reject a mismatched runner or dataset. |
This adds an opt-in JSONL artifact for benchmark runs.
The DB stays the source of truth. The artifact is for debugging one run after the fact, especially when a provider fails or a run finishes partial.
It writes:
I kept transcripts out of the artifact. It stores only sha256 plus character count. That gives enough identity for debugging without dumping raw transcript text into local files.
Enable it with
RUN_ARTIFACT_DIR. If unset, behavior is unchanged.Review updates pushed in
d68e385:--rmChecked locally:
uv run pytest tests/unit/test_run_artifacts.py tests/unit/test_orchestrator.pyuv run ruff check src/coval_bench/config.py src/coval_bench/runner/artifacts.py src/coval_bench/runner/orchestrator.py tests/unit/test_run_artifacts.py tests/unit/test_orchestrator.pyuv run ruff format --check src/coval_bench/config.py src/coval_bench/runner/artifacts.py src/coval_bench/runner/orchestrator.py tests/unit/test_run_artifacts.py tests/unit/test_orchestrator.pyuv run mypy src/coval_bench/runner/artifacts.py src/coval_bench/runner/orchestrator.py