feat(harness): add portable runfile sessions and replayable Slack reports#2290
Conversation
Signed-off-by: jioffe502 <jioffe@nvidia.com>
Signed-off-by: jioffe502 <jioffe@nvidia.com>
Signed-off-by: jioffe502 <jioffe@nvidia.com>
Signed-off-by: jioffe502 <jioffe@nvidia.com>
Signed-off-by: jioffe502 <jioffe@nvidia.com>
jioffe502
changed the title
Greptile SummaryThis PR adds a portable, artifact-first benchmark workflow to the harness:
|
| Filename | Overview |
|---|---|
| nemo_retriever/src/nemo_retriever/harness/runsets.py | New run_runfiles function adds portable multi-run sessions with correct preflight-all-then-execute ordering; session failure propagation and first-nonzero exit code selection are minor design concerns. |
| nemo_retriever/src/nemo_retriever/graph/retriever.py | Adds execute_in_place path for default graph to preserve embedded delegate state across BEIR queries; removes the per-call clone (resolve_for_local_execution) which was the previous concurrency safety mechanism. |
| nemo_retriever/src/nemo_retriever/graph/pipeline_graph.py | Cleanly extracts execute_in_place from execute, making the clone-on-execute vs. in-place paths explicit; docstring accurately describes the lazy-delegate retention trade-off. |
| nemo_retriever/src/nemo_retriever/harness/execution.py | Well-structured split between preflight_benchmark and run_prepared_benchmark; exception chain for artifact write failures is correct; double write of resolved_benchmark.json is documented. |
| nemo_retriever/src/nemo_retriever/harness/slack.py | Significantly refactored: renames NightlyRunReport/NightlySessionReport to HarnessRunReport/HarnessSessionReport, adds load_replay_report and post_slack_payload, webhook credentials are kept in the environment and redacted from errors. |
| nemo_retriever/src/nemo_retriever/harness/artifact_writer.py | Stale-artifact cleanup on ArtifactWriter.init, traceback capture in capture_output_to_log, and expanded redact for key=value override strings are all correct; results_path is now stored as a relative path in status.json. |
| nemo_retriever/src/nemo_retriever/harness/json_io.py | Atomic write-via-temp-file pattern is correct; tmp file cleanup on failure and HarnessRunError wrapping are both handled properly. |
| nemo_retriever/src/nemo_retriever/harness/dataset_paths.py | New file for machine-local dataset path overrides; validates schema version, unknown keys, and resolves relative paths correctly against the config file's directory. |
| nemo_retriever/src/nemo_retriever/harness/cli.py | New run-files and post-slack commands are properly wired; post-slack catches all exceptions at the CLI boundary consistent with the no-bare-except rule. |
| nemo_retriever/src/nemo_retriever/operators/vdb.py | Embedding error propagation improvement: extracts and appends error field from metadata dict to the vector extraction exception, making failures actionable. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant CLI as harness/cli.py
participant RS as runsets.py
participant EX as execution.py
participant AW as ArtifactWriter
participant SL as slack.py
User->>CLI: "retriever harness run-files *.json --dataset-paths map.yaml"
CLI->>RS: run_runfiles(runfiles, dataset_paths_file)
RS->>RS: load_dataset_paths(dataset_paths_file)
loop preflight all runs
RS->>EX: preflight_benchmark(benchmark, mode, overrides)
EX-->>RS: PreparedBenchmark
end
RS->>RS: _run_session(runs, session_dir)
RS->>AW: write expanded_runs.json
loop execute runs sequentially
RS->>EX: run_prepared_benchmark(prepared)
EX->>AW: ArtifactWriter.__init__ clears stale artifacts
EX->>AW: write status.json, runfile.json, resolved_benchmark.json
EX->>EX: ingest + BEIR query
EX->>AW: write results.json (atomic)
EX-->>RS: RunOutcome
end
RS->>AW: write session_summary.json
RS-->>CLI: RunOutcome(session_dir)
User->>CLI: retriever harness post-slack session_dir --preview
CLI->>SL: load_replay_report([session_dir])
SL->>SL: load_session_report(session_summary.json)
SL-->>CLI: HarnessSessionReport
CLI->>SL: build_slack_payload(report, slack_config)
SL-->>CLI: payload JSON blocks
CLI-->>User: echo payload no webhook call
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant CLI as harness/cli.py
participant RS as runsets.py
participant EX as execution.py
participant AW as ArtifactWriter
participant SL as slack.py
User->>CLI: "retriever harness run-files *.json --dataset-paths map.yaml"
CLI->>RS: run_runfiles(runfiles, dataset_paths_file)
RS->>RS: load_dataset_paths(dataset_paths_file)
loop preflight all runs
RS->>EX: preflight_benchmark(benchmark, mode, overrides)
EX-->>RS: PreparedBenchmark
end
RS->>RS: _run_session(runs, session_dir)
RS->>AW: write expanded_runs.json
loop execute runs sequentially
RS->>EX: run_prepared_benchmark(prepared)
EX->>AW: ArtifactWriter.__init__ clears stale artifacts
EX->>AW: write status.json, runfile.json, resolved_benchmark.json
EX->>EX: ingest + BEIR query
EX->>AW: write results.json (atomic)
EX-->>RS: RunOutcome
end
RS->>AW: write session_summary.json
RS-->>CLI: RunOutcome(session_dir)
User->>CLI: retriever harness post-slack session_dir --preview
CLI->>SL: load_replay_report([session_dir])
SL->>SL: load_session_report(session_summary.json)
SL-->>CLI: HarnessSessionReport
CLI->>SL: build_slack_payload(report, slack_config)
SL-->>CLI: payload JSON blocks
CLI-->>User: echo payload no webhook call
Reviews (9): Last reviewed commit: "Merge branch 'main' into codex/retriever..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| ) | ||
|
|
||
| session_dir.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
P2: Reusing a session directory leaves the previous terminal summary visible during the new run.
After preflight succeeds, this code writes the new expanded_runs.json but does not remove an existing session_summary.json. Pollers or post-slack can therefore read the previous session success while the replacement session is still running; if execution later aborts, that stale summary remains indefinitely. This conflicts with session_summary.json being the authoritative terminal result.
Please remove or invalidate the old summary after all-session preflight succeeds and before starting child runs. A regression test should seed an old summary, start a rerun, and verify that it is not visible until the new terminal summary is written.
There was a problem hiding this comment.
Fixed in 15150d19. After all session preflight succeeds, reused session directories now remove any stale session_summary.json before child runs start; added regression coverage that seeds an old summary and asserts it is not visible during the replacement run.
| ) | ||
| return RunOutcome(exit_code=exc.exit_code, artifact_dir=writer.artifact_dir, results=result) | ||
| except Exception as exc: | ||
| append_text(writer.path("run.log"), f"\n## {traceback.format_exc()}") |
There was a problem hiding this comment.
P2: Artifact-write failures do not produce the documented exit code 30.
Exceptions while writing artifacts enter this generic internal-error path and are classified as exit code 70; if writing the failure result also fails, they escape entirely. As a result, the documented EXIT_ARTIFACT_WRITE_FAILURE = 30 does not appear reachable through this execution path.
Please either classify recoverable artifact-write failures as exit code 30 or remove/update the unsupported contract.
There was a problem hiding this comment.
Fixed in 15150d19. Artifact-write failures during terminal result writing now return EXIT_ARTIFACT_WRITE_FAILURE (30), including the case where writing the failure result itself also fails. Added direct run_benchmark() regression coverage.
| run_results: list[dict[str, Any]] = [] | ||
| exit_code = EXIT_SUCCESS | ||
| for expanded, request in zip(expanded_runs, requests, strict=True): | ||
| outcome = run_benchmark( |
There was a problem hiding this comment.
P1: A raised child-run failure aborts the entire session.
run_benchmark() repeats preflight_benchmark() before its exception handler, so an input disappearing between the session preflight and execution raises here instead of returning a failed RunOutcome. The exception skips all remaining runs and prevents session_summary.json from being written, contradicting the advertised continue-remaining-runs and terminal-artifact behavior. The same problem affects run_runset(), and other setup failures before the run_benchmark() try block can follow this path.
Please either reuse the session preflight result or catch and convert per-run exceptions at the session-loop boundary. Add coverage where the initial preflights succeed but a later execution-time preflight raises, asserting that subsequent runs execute and the session summary is written.
There was a problem hiding this comment.
Fixed in 15150d19 by catching per-child execution exceptions at the session-loop boundary and converting them into failed child outcomes, so remaining runs continue and session_summary.json is still written. Added coverage for both run-files and runset.
| dataset_name = get_benchmark(request.benchmark).dataset | ||
| dataset_paths = local_dataset_paths.get(dataset_name) | ||
| dataset_overrides = dataset_paths.overrides() if dataset_paths is not None else () | ||
| effective_mode = mode or request.mode or "local" | ||
| effective_overrides = (*request.overrides, *dataset_overrides, *overrides) | ||
| effective_requirements = (*request.requirements, *requirements) | ||
| preflight_benchmark( | ||
| request.benchmark, | ||
| mode=effective_mode, | ||
| overrides=effective_overrides, | ||
| requirements=effective_requirements, | ||
| dry_run=dry_run, | ||
| ) |
There was a problem hiding this comment.
KeyError escapes for unregistered benchmarks
get_benchmark(request.benchmark) is called at line 290 to fetch the dataset name before preflight_benchmark runs at line 296. If request.benchmark is not in the registry, get_benchmark raises a raw KeyError, which propagates through run_runfiles and past the except HarnessRunError handler in run_files_command, producing an unhandled Python traceback instead of a clean user-facing error. The companion _runset_or_error helper (line 65) shows the intended pattern: catch KeyError and wrap it in HarnessRunError with a did-you-mean suggestion. The same guard should be applied here.
Prompt To Fix With AI
This is a comment left during a code review.
Path: nemo_retriever/src/nemo_retriever/harness/runsets.py
Line: 290-302
Comment:
**Bare `KeyError` escapes for unregistered benchmarks**
`get_benchmark(request.benchmark)` is called at line 290 to fetch the dataset name before `preflight_benchmark` runs at line 296. If `request.benchmark` is not in the registry, `get_benchmark` raises a raw `KeyError`, which propagates through `run_runfiles` and past the `except HarnessRunError` handler in `run_files_command`, producing an unhandled Python traceback instead of a clean user-facing error. The companion `_runset_or_error` helper (line 65) shows the intended pattern: catch `KeyError` and wrap it in `HarnessRunError` with a `did-you-mean` suggestion. The same guard should be applied here.
How can I resolve this? If you propose a fix, please make it concise.Signed-off-by: jioffe502 <jioffe@nvidia.com>
Signed-off-by: jioffe502 <jioffe@nvidia.com>
2 participants
TL;DR
Add a portable, artifact-first workflow for running one or more checked-in Retriever benchmarks and optionally reporting the completed artifacts to Slack.
Users and agents can copy the dataset path-map example, point it at datasets available on their machine, run one runfile or the four-dataset library suite, inspect a compact terminal summary, and preview or post the same artifacts afterward. Harness execution never implicitly contacts Slack. This PR does not add recurring scheduling or machine-owned nightly infrastructure.
What changed
Portable benchmark sessions
retriever harness run-filesfor one or more JSON/YAML runfiles.--dataset-pathsmap after runfile defaults and before CLI overrides.expanded_runs.json,session_summary.json, and one child artifact directory per run.Smaller artifact contract
status.jsonwhile a run is active.results.jsonwhen one run is terminal.session_summary.jsonwhen a multi-run session is terminal.summary_metrics.jsonfor new runs while retaining read compatibility indiffand Slack replay.run.log.Post-hoc Slack reporting
retriever harness post-slackfor a completed session or one or more run artifacts.--preview, which renders the exact payload without readingSLACK_WEBHOOK_URLor making a network request.--artifact-pathsis explicitly requested.Repeated-query reliability
User-facing boundaries
retriever ingestandretriever queryfor direct product workflows.retriever harness run-filesfor portable benchmark/evaluation sessions.retriever harness post-slackonly as optional artifact post-processing.--dry-runas configuration/planning preflight, not runtime evidence; use a real small benchmark when model startup or execution behavior changes.Suggested review order
harness/cli.py,harness/dataset_paths.py,harness/runsets.py, and the checked-in runfiles.harness/artifact_writer.py,harness/execution.py,harness/environment.py, andharness/diff.py.harness/slack.pyandtest_harness_slack.py.graph/pipeline_graph.py,graph/retriever.py,operators/vdb.py, and their focused tests.harness/README.md,EXPECTED_RESULTS.md, andHANDOFF.md.The commits already follow that dependency/order boundary; no history rewrite is needed.
Validation
177 passed: artifact/session/Slack contract, runfile sessions, graph execution, and repeated-query caching.128 passed: agentic, query workflow, root query CLI, comparator, and service-query regressions.data/multimodal_test.pdfwrote status/result/planning artifacts, created no LanceDB, run log, or query output, and produced a valid Slack preview with no webhook configured.SLACK_WEBHOOK_URLunset.summary_metrics.jsonartifacts.success=true,all_passed=true, and exit code0:Known follow-up
Two oversized BO767 chunks exceeded the embedder context limit during the full run. The session and query gates passed; input truncation/splitting is separate follow-up work.
Checklist