fix: stabilize FASB fine-tuning protocol - #8
Conversation
|
@codex review this PR. Focus on whether FASB stabilization avoids timeout collapse scientifically, uses dev checkpoint selection instead of final-heldout tuning, and preserves fair Axis 1 comparison. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: edf969a2e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mode: fasb_ppo | ||
| metadrive: {env_class: metadrive.envs.MetaDriveEnv, config: {start_seed: 2000, num_scenarios: 500, traffic_density: 0.1, random_traffic: true, use_render: false, horizon: 500, log_level: 50, crash_vehicle_done: false, crash_object_done: false, out_of_road_done: false}} | ||
| vec_env: {type: dummy, n_envs: 1, start_method: forkserver} | ||
| algorithm: {backend: sb3, name: PPO, policy: MlpPolicy, checkpoint_path: runs/research_v1/base_pretrain_s42/checkpoints/final.zip, params: {learning_rate: 0.00003, n_steps: 128, batch_size: 64, n_epochs: 10, gamma: 0.99, gae_lambda: 0.95, clip_range: 0.2, ent_coef: 0.0, vf_coef: 0.5, max_grad_norm: 0.5, verbose: 1, device: cpu, policy_kwargs: {net_arch: [256, 256]}}} |
There was a problem hiding this comment.
This config changes FASB’s optimizer regime (learning_rate: 0.00003) while the Axis 1 naive and fixed-budget baselines still use the original linear:3.0e-4 schedule, so the reported timeout-collapse recovery is no longer attributable to FASB stabilization alone. Because this PR’s stated goal is a fair Axis 1 method comparison, per-method optimizer retuning introduces a direct confound unless the same dev tuning/checkpoint-selection policy is applied to all compared methods.
Useful? React with 👍 / 👎.
|
@codex review this PR again. I addressed the optimizer-confound by rerunning Axis 1 baselines under the same stable learning rate and dev checkpoint-selection protocol as stable FASB. Please check whether the comparison is now scientifically fair and whether Axis 2–5 can proceed from the calibrated default. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3048bc2bbd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if selected is None: | ||
| print("No checkpoint passed hard reject screen") | ||
| else: | ||
| print(f"Selected checkpoint: {selected['checkpoint']}") | ||
| return 0 |
There was a problem hiding this comment.
When every candidate is hard-rejected, the script only prints a message and still returns success, leaving selected_dev_best.zip absent. The documented next step evaluates that path, and Evaluator._load_policy() falls back to RandomPolicy if the checkpoint file is missing, so an entire method can be scored as random behavior without the pipeline failing. This silently corrupts the fairness comparison in exactly the low-performing cases the hard-reject screen is meant to catch.
Useful? React with 👍 / 👎.
|
@codex review this PR again. I addressed the remaining P1 by making checkpoint selection exit nonzero when all candidates are hard-rejected, and by making explicit missing checkpoint evaluation raise instead of falling back to RandomPolicy. Tests now cover both cases. Please confirm whether there are no remaining issues. |
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review this PR again. I aligned Axis 2–5 final configs/docs with the stable FASB protocol and moved/marked old unstable configs as screening/historical. Please check that teammates cannot accidentally run the old failed FASB defaults for final axes. |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review this PR again. CI failed because a lightweight test imported stable_baselines3 directly; I replaced it with a sys.modules fake so lightweight CI remains dependency-light while still testing checkpoint-load param forwarding. Please check for remaining blockers. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89fd225be7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with pytest.raises(FileNotFoundError): | ||
| Evaluator(cfg, tmp_path)._load_policy(tmp_path / "missing.zip", env) |
There was a problem hiding this comment.
This new lightweight test still depends on stable_baselines3: calling Evaluator._load_policy(tmp_path / "missing.zip", env) imports SB3 before checking whether the checkpoint exists, so environments without SB3 raise ModuleNotFoundError instead of the expected FileNotFoundError. That means dependency-light CI can still fail even though other tests were switched to sys.modules fakes. Please stub SB3 in this test (as done in test_checkpoint_finetune_honors_configured_algorithm_params) or avoid this code path in the lightweight suite.
Useful? React with 👍 / 👎.
|
@codex review this PR again. I addressed the lightweight-CI blocker by checking explicit missing eval checkpoints before importing stable_baselines3, so the missing-checkpoint unit test no longer needs SB3 installed. Please confirm there are no remaining blockers. |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Fixes the Axis 1 stabilization protocol after review found an optimizer confound: stable FASB used
learning_rate=0.00003, while the old naive/fixed baselines usedlinear:3.0e-4.This PR now keeps FASB-PPO as SB3 PPO + failure-aware sampler + adaptive safety penalty, and makes the fair Axis 1 comparison use the same stable optimizer and dev checkpoint-selection policy for all fine-tuned methods.
Codex P1 fairness issue addressed
The previous PR version compared stable FASB against old naive/fixed runs with a different optimizer regime. Optimizer is not the research variable, so that comparison was confounded.
Added fair stable-protocol baseline configs:
configs/research_v1/axis1_naive_stable_final.yamlconfigs/research_v1/axis1_fixed_budget_stable_final.yamlconfigs/research_v1/axis1_fasb_stable_final.yamlAll stable-protocol fine-tuned methods use:
runs/research_v1/base_pretrain_s42/checkpoints/final.zip300000start_seed=2000,num_scenarios=500horizon=500,traffic_density=0.1vec_env.type=dummy,vec_env.n_envs=1algorithm.params.device=cpualgorithm.params.learning_rate=0.00003start_seed=4500,num_scenarios=100,eval.n_episodes=100start_seed=5000,num_scenarios=200,eval.n_episodes=100Stable FASB config remains:
sampler.failure_ratio=0.05safety_budget.d_min=0.10safety_budget.d_max=0.30safety_budget.timeout_budget=0.30penalty_scheduler.lambda_min=0.0penalty_scheduler.lambda_max=0.25Stable-protocol dev selection
Stable FASB was not reselected from final heldout. The final heldout was not used for candidate selection.
Final heldout results
Stable FASB still beats naive stable under fair settings on success, timeout, route completion, episode cost, and safety-efficiency. It does not dominate every raw safety metric: collision is higher than naive stable, while offroad is lower.
Historical diagnostics remain visible in the local Axis 1 report:
Other source fixes in this PR
MixedFailureSamplerfilters canonical failure-buffer records to the active trainer seed range and falls back to random sampling when no eligible failure exists.CostFunctionWrapper.reset()calls plugin reset hooks when present.EventDrivingCostwas added as an optional event-style cost plugin and tested, but it is not the selected stable default.scripts/select_best_checkpoint.pyevaluates saved checkpoints on dev seeds and copies the selected checkpoint toselected_dev_best.zip.Validation
python -m compileall fasb tests scripts examples -qpython -m pytest tests/test_component_validation.py tests/test_failure_buffer.py tests/test_metrics.py tests/test_training_stability_patch.py tests/test_plugin_loading.py tests/test_run_dir.py tests/test_research_plugins.py tests/test_aggregate_results.py tests/test_base_checkpoint_quality.py tests/test_failure_buffer_quality.py tests/test_research_v1_ready.py tests/test_research_v1_artifact_scripts.py tests/test_select_best_checkpoint.py -qpython scripts/validate_components.py --config configs/research_v1/axis1_fasb_stable_final.yamlpython scripts/validate_components.py --config configs/research_v1/axis1_naive_stable_final.yamlpython scripts/validate_components.py --config configs/research_v1/axis1_fixed_budget_stable_final.yamlAxis 2-5 status
Approved to proceed from the calibrated stable FASB default if review agrees the optimizer-confound is resolved. Axis 2/3 should vary sampler and budget/penalty around this default while preserving the shared stable optimizer/checkpoint-selection protocol for compared methods.