Skip to content

test: add 7 tests for PromptBakingTrainer.training_step - #26

Merged
marksverdhei merged 5 commits into
marksverdhei:mainfrom
hai-pilgrim:test/training-step
Mar 29, 2026
Merged

marksverdhei merged 5 commits into
marksverdhei:mainfrom
hai-pilgrim:test/training-step

Conversation

@hai-pilgrim

Copy link
Copy Markdown

Summary

Adds test coverage for PromptBakingTrainer.training_step, the method that generates trajectories on-the-fly when no precomputed responses are in the batch.

Tests added (tests/test_training_step.py)

  • test_training_step_with_existing_responses_delegates_to_super — when inputs["responses"] is already populated, confirms super().training_step() is called without generating new trajectories
  • test_training_step_generates_trajectories_when_no_responses — verifies _generate_trajectory is called exactly num_trajectories times per prompt
  • test_training_step_populates_inputs_with_generated_responses — checks that generated responses are written into inputs["responses"] before calling super()
  • test_training_step_filters_blank_trajectories — blank/whitespace-only responses are excluded from the batch
  • test_training_step_returns_zero_when_all_trajectories_blank — all-blank responses → returns a zero-loss tensor with requires_grad=True, super() not called
  • test_training_step_warns_when_many_trajectories — a WARNING is logged when len(user_messages) * num_trajectories > 64
  • test_training_step_no_warn_below_threshold — no warning emitted when at or below the 64-trajectory threshold

Test plan

All 7 tests pass on CPU with tiny GPT-2 + LoRA (same fixture as existing test_trainer.py).

🤖 Generated with Claude Code

@marksverdhei

Copy link
Copy Markdown
Owner

Review

Good test coverage for training_step branching logic (delegation, trajectory generation, blank filtering, warning thresholds).

Conflict with #17: This PR mocks _generate_trajectory called in a sequential loop, but #17 replaces that loop with _generate_trajectories_batched. Once #17 merges, these tests will break because training_step will no longer call _generate_trajectory directly.

Please update the tests to mock _generate_trajectories_batched instead, returning [(msg, response)] tuples. The test logic (blank filtering, zero-loss, warning threshold) is still valuable — it just needs to target the new API.

Will merge once updated for the batched generation interface.

marksverdhei and others added 2 commits March 29, 2026 17:48
Covers:
- delegates to super when responses already present
- calls _generate_trajectory num_trajectories times
- populates inputs['responses'] before delegating
- filters blank/whitespace-only trajectories
- returns zero-loss tensor when all trajectories blank
- warns when trajectory count exceeds threshold (64)
- no warning when count is at or below threshold

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR marksverdhei#17 replaces the _generate_trajectory loop in training_step with a
single _generate_trajectories_batched call returning (msg, response)
tuples. Update all test mocks accordingly so tests remain valid after
marksverdhei#17 merges.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@marksverdhei marksverdhei left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The updated mocks targeting _generate_trajectories_batched look correct. Clean test structure covering delegation, blank filtering, zero-loss path, and warning thresholds.

However, this PR depends on #17 (which introduces _generate_trajectories_batched). #17 currently has merge conflicts with main after #20 was merged and needs a rebase first. Once #17 lands, this is ready to merge. Holding approval until then.

@marksverdhei marksverdhei left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Reviewed. Good decision reverting to mock _generate_trajectory (current main API) instead of coupling to #17's unreleased batched method. Tests cover the key training_step branches: super delegation with existing responses, trajectory generation, blank filtering, zero-loss on all-blank, and warning thresholds. LGTM.

@marksverdhei
marksverdhei merged commit 8da2ae5 into marksverdhei:main Mar 29, 2026
2 checks passed
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.

2 participants