Refactor speedrun mode and leaderboard tooling - #328
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1688b4da36
ℹ️ 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".
|
|
||
| def main() -> None: | ||
| parser = argparse.ArgumentParser(description="Append a record to LEADERBOARD.md") | ||
| parser.add_argument("json_path", nargs="?", default="logs/speedrun/speedrun_metrics.json") |
There was a problem hiding this comment.
Align append default JSON path with speedrun output naming
append_leaderboard.py defaults to logs/speedrun/speedrun_metrics.json, but SpeedrunTracker._resolve_metrics_path() now rewrites that default to either {wandb_run_id}.json (wandb enabled) or speedrun_YYYYMMDD_HHMMSS.json (wandb disabled). In the common workflow of running training with --speedrun and then running append with no positional argument, the script will fail with Error: logs/speedrun/speedrun_metrics.json not found because that file is never produced anymore. Please update the default lookup behavior (or output naming) so the no-arg append flow remains functional.
Useful? React with 👍 / 👎.
|
Thanks for the follow-up here. I closed #245 so we can keep the discussion on this PR. This looks like the right direction. One thing I’d like to do for discoverability is keep the main README change separate from this feature PR. Could you add a That should keep this PR focused on the feature/refactor while still making the speedrun workflow easy to find. |
|
Thank you for thinking about discoverability and for suggesting a better place for I added |
|
Thanks! Looks good. I merged #348 to see if we get any traction here :) |
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
6f13f4d to
7d474fc
Compare
Update after rebuilding on current upstream
|
|
Closing until this gets more traction/interest -- i suspect it gets spun out in a separate repo. |
|
I have a few ideas for features that would be useful for an RL speedrun, so I’d like to come back to it when I can. Thanks for all your feedback throughout this PR! |
Supersedes #245.
Hi @natolambert, I've finished refactoring the previous PR into this follow-up. I moved the speedrun-specific logic into a
speedrun.pyhelper module to keep the main loop intrain.pymore focused and easier to follow. This helper module is a new file added in this PR.All changes are below:
Missing metadata
model_nameanddatasetare now included in the speedrun JSON payload throughSpeedrunTracker, and surfaced as leaderboard columns with sorting support.Framing
LEADERBOARD.mdnow opens with an explicit note that the feature is experimental, and its relevant sections were updated to match the refactor and the new leaderboard layout.Code organization
train.pyinto a dedicatedSpeedrunTrackerclass (code/policy_gradients/speedrun.py).tracker.record_step(),tracker.check_goal(), andtracker.write_metrics().speedrun_opts()has been removed fromutils.py.Practical concerns
{wandb_run_id}.jsonwhen a wandb run ID is available, orspeedrun_YYYYMMDD_HHMMSS.jsonotherwise. This prevents overwriting previous results.append_leaderboard.pyhas been deduplicated into shared helpers, reducing the script from 338 to 218 lines.remove_leaderboard.pyhas been updated for the new column layout.Notes
Separately, I also created a Colab notebook tracing the split / buffer / join data flow around
ReplayBufferin the training loop: Colab notebook.I created it as a personal study note to help me work through the main training loop. It is not part of this PR.