Skip to content

Reject canonical holdout pools in PPO training #17

Description

@NeoLorenzo

Audit Priority: P1
Audit Type: FIX

Problem / Opportunity

The Phase 1 PPO trainer can currently train on maps assigned to canonical non-training splits if POLYVISION_LEVEL_POOL_GLOB is pointed at them. TribesGymWrapper._resolve_level_pool() accepts any matching glob, and the trainer's preflight validation fingerprints the resolved pool but does not verify that its maps belong to the authoritative train assignment.

Repository documentation explicitly says PPO training must never use validation, test, or human_benchmark maps. That rule is therefore procedural rather than enforced by the gradient-training path.

Why This Matters

Accidental gradient updates on a canonical validation, test, or human-benchmark map would contaminate the corresponding evidence pool. The run could otherwise proceed normally, so the mistake may only be discovered after substantial compute or after results have already been interpreted. Content-level isolation is especially important because copied or renamed CSVs should not become safe training inputs merely because their path changed.

This matters now: the project is relying on strict split semantics for current human-relative evidence and is separately planning a genuinely pristine final-evidence benchmark in #2.

Evidence

  • docs/training.md states that training must never use validation, test, or human_benchmark; those pool overrides are for evaluation, not PPO gradient runs.
  • docs/reproducibility.md freezes the authoritative 5,000/250/250/17 train/validation/test/human-benchmark split and states that training maps cannot later become honestly held out.
  • pol_env/Tribes/py/register_env.py::_resolve_level_pool() resolves POLYVISION_LEVEL_POOL_GLOB generically and returns any matched files; it does not inspect split_manifest.json assignments.
  • py_rl/cleanrl/cleanrl/ppo.py uses that wrapper for both strict preflight validation and the asynchronous training environments. Its validator computes a pool identity for cache/provenance purposes, but there is no train-only membership check before optimization begins.
  • tools/phase1_eval_core.py::load_verified_pool() already enforces the inverse boundary for evaluation by rejecting training-map identity leakage into evaluation pools, showing that content identity is available and scientifically meaningful.
  • Open issue Establish a genuinely pristine Phase 1 test benchmark #2 concerns creating and protecting a future pristine final-evidence benchmark; it does not currently enforce train-only membership for the PPO gradient path across the existing canonical splits.

Proposed Outcome

Make the maintained PPO training entry point fail closed before training starts if any resolved map is assigned to a canonical non-training split. Validate by authoritative content identity/manifest assignment rather than filename or directory name so copied or renamed holdout maps are still rejected.

Keep the scope focused on protecting canonical split assignments. Do not silently redefine whether genuinely external/non-manifest map pools are permitted for controlled experiments.

Definition of Done

  • Before PPO rollout/optimization begins, the trainer validates the resolved canonical map pool against the authoritative split manifest.
  • Any map assigned to validation, test, human_benchmark, or another canonical non-training/final-evidence split causes a clear fail-closed error.
  • Holdout membership is detected by authoritative map/content identity rather than path alone, so moving or renaming a canonical holdout CSV does not bypass the guard.
  • The standard 5,000-map Phase 1 training pool continues to pass unchanged.
  • The failure message identifies enough offending split/map information to diagnose the configuration without exposing or consuming protected benchmark results.
  • Regression tests cover the normal train pool contract, at least one canonical holdout assignment, and a renamed/copied holdout fixture or equivalent content-identity case.
  • The maintained cheap CI gate exercises the train/holdout isolation check without running PPO training or consuming benchmark outcomes.

Constraints / Non-Goals

This issue does not create the future pristine benchmark proposed in #2, change existing split assignments, prevent evaluation from reading evaluation pools, or require banning all external/non-manifest maps. It only makes the repository's existing train-vs-holdout rule enforceable at the PPO training boundary.

Validation

Run a cheap trainer/preflight fixture against a valid train-only manifest subset and confirm it proceeds past the pool guard. Then point the same path at a canonical holdout identity, including under a different filename/path, and confirm the trainer stops before any gradient-training rollout begins.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions