Skip to content

fix(batch_runner): write a discard tombstone so resume skips no-reasoning prompts - #93542

Closed
chelsealong wants to merge 1 commit into
NousResearch:mainfrom
chelsealong:fix/batch-runner-discard-resume-93527
Closed

chelsealong wants to merge 1 commit into
NousResearch:mainfrom
chelsealong:fix/batch-runner-discard-resume-93527

Conversation

@chelsealong

Copy link
Copy Markdown

What does this PR do?

Fixes #93527. The "no reasoning in any turn" discard path in
_process_batch_worker() marked the prompt as completed in the
in-memory/checkpoint tracking but never wrote a row to batch_N.jsonl.
--resume's only dataset filter, _scan_completed_prompts_by_content(),
scans exclusively those batch_*.jsonl files for prompt text — so any
discarded prompt whose batch hadn't yet returned from the worker pool
(e.g. the run was interrupted mid-batch, a very normal way for a long
batch job to end) is invisible to it. On --resume it gets reprocessed
at full agent/API cost and discarded again, repeating indefinitely.

The fix: write a lightweight tombstone row for each discarded prompt
({"prompt_index": N, "conversations": ..., "discarded": "no_reasoning"})
into the same batch_N.jsonl file used for real trajectories, so the
existing content-based scan naturally picks it up as already processed
(it only special-cases failed rows for retry; anything else with a
human-turn goes into the completed-prompts set). Two follow-on gaps
called out in the issue are fixed too:

  • The trajectory-combine step that builds trajectories.jsonl now
    explicitly skips these tombstones, so they never leak into the
    training data.
  • statistics.json now reports discarded_no_reasoning (previously
    only printed to stdout, so totals couldn't be reconciled after the
    fact).

Related Issue

Fixes #93527

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • batch_runner.py: _process_batch_worker() writes a discard tombstone
    row (with fsync, matching the durability of the success path) instead
    of writing nothing.
  • batch_runner.py: the batch-file combine step in BatchRunner.run()
    skips rows with a discarded key before they can reach
    trajectories.jsonl.
  • batch_runner.py: discarded_no_reasoning is aggregated across batches
    and included in statistics.json.
  • tests/test_batch_runner_checkpoint.py: updated the existing
    test_discarded_no_reasoning_prompts_are_marked_completed (which
    asserted the old, buggy behavior — no row written) to assert the
    tombstone is written instead, and added
    test_resume_after_all_discarded_batch_reruns_zero_prompts, a direct
    regression test for the issue: it drives _process_batch_worker() to
    discard a prompt, then drives _scan_completed_prompts_by_content() and
    _filter_dataset_by_completed() exactly as run() does, and asserts
    the discarded prompt is (a) visible to the content scan and (b) not
    rescheduled on resume.

How to Test

Verified the new/updated tests fail without the fix and pass with it, by
diffing out the batch_runner.py change and rerunning:

$ git stash push -- batch_runner.py   # (only the fix, tests unstaged)
$ python3 -m pytest tests/test_batch_runner_checkpoint.py -q
...
FAILED tests/test_batch_runner_checkpoint.py::TestBatchWorkerResumeBehavior::test_discarded_no_reasoning_prompts_are_marked_completed
FAILED tests/test_batch_runner_checkpoint.py::TestBatchWorkerResumeBehavior::test_resume_after_all_discarded_batch_reruns_zero_prompts
2 failed, 13 passed in 1.25s

$ git stash pop                       # fix restored
$ python3 -m pytest tests/test_batch_runner_checkpoint.py tests/test_batch_runner_durability.py tests/test_batch_runner_exit_code.py -q
......................                                                   [100%]
22 passed in 6.14s

Also ran, both clean:

$ python3 -m ruff check batch_runner.py tests/test_batch_runner_checkpoint.py
All checks passed!

$ python3 scripts/check-windows-footguns.py batch_runner.py
✓ No Windows footguns found (1 file(s) scanned).

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q (the specific test files above) and all tests pass
  • I've added tests for my changes (required for bug fixes)
  • I've tested on my platform: Linux, Python 3.12

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A, no public API/config surface changed
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
  • I've considered cross-platform impact (Windows, macOS) — verified with scripts/check-windows-footguns.py
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A

AI assistance disclosure

This PR was prepared with AI assistance (Claude), including the root-cause
analysis, the fix, and the tests. All changes were verified locally
(tests, ruff, windows-footguns check) before submission.

…ning prompts

The no-reasoning discard path marked prompts completed in the checkpoint
but never wrote a row to batch_N.jsonl. --resume's content-based scan
only reads those files, so any discarded prompt not yet flushed to the
checkpoint (e.g. the batch was interrupted before it fully returned) is
invisible to it and gets reprocessed at full cost, then discarded again.

Write a tombstone row ({"discarded": "no_reasoning", ...}) for each
discarded prompt so the scan sees it as done. The trajectory combine
step excludes these tombstones from trajectories.jsonl, and
statistics.json now reports discarded_no_reasoning.
@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have labels Aug 24, 2026
teknium1 pushed a commit that referenced this pull request Aug 24, 2026
…ning prompts

The no-reasoning discard branch in _process_batch_worker continued
before writing any JSONL row, so run(resume=True) — which filters
solely via _scan_completed_prompts_by_content over batch_*.jsonl —
never saw discarded prompts and re-ran them at full cost on every
resume. Write a tombstone row on discard, exclude tombstones from the
trajectories.jsonl merge, and report discarded_no_reasoning in
final statistics.

Salvaged from #93542.
Fixes #93527
teknium1 pushed a commit that referenced this pull request Aug 24, 2026
…bstones

Complete the #93527 fix: the tombstone now carries the human prompt
text via _entry_prompt_text (handling flat prompt, ShareGPT, and
chat-style shapes), _scan_completed_prompts_by_content counts
discarded rows as completed instead of only reading ShareGPT
conversations, and the merge step reports excluded tombstones in the
combined-count summary. Adds a dedicated regression suite covering the
tombstone round-trip, the all-discarded-batch resume path, and merge
exclusion.

Salvaged from #93579 (issue reporter's PR), building on #93542.
Fixes #93527
@teknium1

Copy link
Copy Markdown
Collaborator

Landed on main as 316d52f + a26154a with your authorship preserved (cherry-picked by a parallel maintainer session). Closing as landed — the tombstone row + scanner-honor split was the right shape for the resume regression (#93527, prior art #9950/#12997). Thanks!

@teknium1 teknium1 closed this Aug 24, 2026
and7777 pushed a commit to and7777/hermes-agent that referenced this pull request Aug 27, 2026
…ning prompts

The no-reasoning discard branch in _process_batch_worker continued
before writing any JSONL row, so run(resume=True) — which filters
solely via _scan_completed_prompts_by_content over batch_*.jsonl —
never saw discarded prompts and re-ran them at full cost on every
resume. Write a tombstone row on discard, exclude tombstones from the
trajectories.jsonl merge, and report discarded_no_reasoning in
final statistics.

Salvaged from NousResearch#93542.
Fixes NousResearch#93527
and7777 pushed a commit to and7777/hermes-agent that referenced this pull request Aug 27, 2026
…bstones

Complete the NousResearch#93527 fix: the tombstone now carries the human prompt
text via _entry_prompt_text (handling flat prompt, ShareGPT, and
chat-style shapes), _scan_completed_prompts_by_content counts
discarded rows as completed instead of only reading ShareGPT
conversations, and the merge step reports excluded tombstones in the
combined-count summary. Adds a dedicated regression suite covering the
tombstone round-trip, the all-discarded-batch resume path, and merge
exclusion.

Salvaged from NousResearch#93579 (issue reporter's PR), building on NousResearch#93542.
Fixes NousResearch#93527
melon-xf added a commit to melon-xf/hermes-agent that referenced this pull request Sep 3, 2026
…ning prompts

The no-reasoning discard branch in _process_batch_worker continued
before writing any JSONL row, so run(resume=True) — which filters
solely via _scan_completed_prompts_by_content over batch_*.jsonl —
never saw discarded prompts and re-ran them at full cost on every
resume. Write a tombstone row on discard, exclude tombstones from the
trajectories.jsonl merge, and report discarded_no_reasoning in
final statistics.

Salvaged from NousResearch#93542.
Fixes NousResearch#93527
melon-xf added a commit to melon-xf/hermes-agent that referenced this pull request Sep 3, 2026
…bstones

Complete the NousResearch#93527 fix: the tombstone now carries the human prompt
text via _entry_prompt_text (handling flat prompt, ShareGPT, and
chat-style shapes), _scan_completed_prompts_by_content counts
discarded rows as completed instead of only reading ShareGPT
conversations, and the merge step reports excluded tombstones in the
combined-count summary. Adds a dedicated regression suite covering the
tombstone round-trip, the all-discarded-batch resume path, and merge
exclusion.

Salvaged from NousResearch#93579 (issue reporter's PR), building on NousResearch#93542.
Fixes NousResearch#93527
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: batch_runner --resume re-runs every no-reasoning discarded sample; the content-based resume scan never sees them

3 participants