AnshumanAtrey / AnshumanAtrey/pharmarl
CRITICAL: Cell 16 in train_pharmarl.ipynb is a stub — no loss.backward(), no optim.step(), model weights never update
- Langage dominant
- Python
- Étoiles
- 0
- Forks
- 1
- Métriques de merge des PR
- Aucune PR mergée en 30 j
Description
## The bug
Cell 16 of \`colab/train_pharmarl.ipynb\` is the alleged GRPO training loop. Verified locally just now against current main (commit \`64b9fdc\`):
| Check | Result |
|---|---|
| Has \`loss.backward()\` | ❌ NO |
| Has \`optim.step()\` | ❌ NO |
| Has PPO-clipped advantage | ❌ NO |
| Has KL penalty against reference | ❌ NO |
| Constructs an \`optim = torch.optim.AdamW(...)\` | ✅ YES (but never used for stepping) |
| Computes \`advantages = [(r - mean_r) for r in rewards]\` | ✅ YES (but never used) |
| Has the explicit comment **"This stub logs the group statistics"** | ✅ YES |
The full disclosure is in the cell's own comment:
> \`\`\`
> # NOTE: full GRPO update requires re-running the model in train mode on the action tokens,
> # weighted by advantages, with a KL penalty against the reference. This stub logs the
> # group statistics — replace with TRL GRPOTrainer.train_step once you've adapted the
> # multi-turn rollout into a flat (prompt, completion) batch.
> \`\`\`
## Impact
**If Sahil runs cells 1→27 today, the model at the end is byte-identical to the model at the start.** The "training run" will:
- Print step counts
- Log to W&B (mean_reward, max, min, std per step)
- Look exactly like training is happening
But the model weights never update. The held-out JNK3 delta we'd report would be **sampling noise**, not learned transfer. The whole 20% Reward Improvement criterion of the rubric collapses.
This is what @AnshumanAtrey was already worried about when asking earlier "should we do unsloth + colab or do it locally" — the answer is moot because cell 16 doesn't train regardless.
## The fix
A real GRPO trainer exists locally on @BeingSeight's machine at \`scripts/train_grpo.py\` (commit \`377df79\` per his report) but **is not on origin/main**. He needs to push it. After push:
\`\`\`python
# Replace cell 16 with a !python -m scripts.train_grpo invocation,
# OR delete cell 16 and run the script directly:
!python -m scripts.train_grpo \\
--env-url http://localhost:8000 \\
--model unsloth/Llama-3.2-3B-Instruct \\
--max-seq-len 768 \\
--num-generations 4 \\
--max-steps 200 \\
--sft-warmup-steps 60 \\
--save-every 25 \\
--output-dir ./trained \\
--hf-repo USERNAME/pharmarl-llama-3b-trained \\
--hf-token "\$HF_TOKEN"
\`\`\`
The script reportedly has:
- \`loss.backward()\` at L151 (SFT) + L358 (GRPO)
- \`optim.step()\` at L155 + L363
- PPO-clipped advantage with \`torch.clamp(ratio, 1-clip_eps, 1+clip_eps) * adv\` at L293
- KL penalty against frozen reference (Schulman-K3)
- \`--save-every 25\` checkpoints to HF Hub (resilience against Colab disconnects)
## Why this didn't surface earlier
- The notebook \`smoke_notebook_locally.py\` script tests the **rollout HTTP loop**, not the training math
- The 98 pytest tests cover env, oracles, rubrics, redteam, critic, oversight, action normalization — none of them test that GRPO actually updates weights
- The "smoke run" in the notebook only verifies the LLM can produce parseable JSON, not that gradients flow
## Action items
1. @BeingSeight: \`git push\` the branch with \`scripts/train_grpo.py\` to origin/main. Resolve any merge conflicts with the 4 commits I pushed today (\`a044d71\`, \`8e5c738\`, \`30260ee\`, \`64b9fdc\`).
2. Add a \`tests/test_grpo_actually_trains.py\` that runs ~5 GRPO steps and asserts at least one parameter delta > 1e-6. Pin this so this regression can't happen again.
3. Once the script lands, retire cell 16 from the notebook (or convert it to a single-line shell-out to the script). The notebook stays useful for setup + eval + demo cells, just not for training.
## Status — should we ship the env without a trained model?
If we cannot land \`train_grpo.py\` and run training in time, the submission becomes:
- Env shipped ✅
- Three sub-theme mechanics (Patronus/Halluminate/Fleet AI) shipped ✅
- Inverted scaling baseline finding documented ✅
- **Trained-model results = absent** (Reward Improvement 20%)
That's still a finalist-tier submission, but loses the "trained 1.5B/3B beats untrained 70B" pitch beat. Decision: @AnshumanAtrey + @BeingSeight need to commit to a path now (push real trainer, or pivot the pitch to "env + baselines, training as future work").
Guide de contribution
Aucun guide de contribution indexé pour ce dépôt
Évaluation
Cette issue n'a pas encore été évaluée.