feat: copy scripts to output dir#46
Open
Neonkraft wants to merge 5 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to make TRL containerized runs more reproducible by “freezing” the code into the run output directory and executing from that frozen copy, while also normalizing output paths so a frozen config can re-derive the same run directory from any working directory.
Changes:
- Resolve
paths.output_base/paths.debug_baseto absolute paths during run directory setup and persist them back onto the config. - Add guardrail output indicating when a run directory already contains frozen
src/post_trainingand/orscripts. - Update the TRL container SLURM template to copy
src/post_trainingandscriptsintorun_dirand run withPYTHONPATH/cwdpointing atrun_dir.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/post_training/utils/paths.py |
Resolves output/debug base paths to absolute and persists them into the config before computing run_dir. |
src/post_training/utils/guardrails.py |
Adds a warning row when frozen source directories already exist under the run directory (TRL container path). |
src/post_training/slurm/job_trl_container.sh.jinja |
Adds a runtime copy step to “freeze” code into run_dir, and switches execution to use run_dir for PYTHONPATH and working directory. |
scripts/submit.py |
Ensures the frozen config path passed into SLURM generation/submission is an absolute resolved path. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+75
to
+84
| if [ -d "{{ run_dir }}/src/post_training" ] || [ -d "{{ run_dir }}/scripts" ]; then | ||
| echo "WARNING: {{ run_dir }}/src/post_training or {{ run_dir }}/scripts already exists." | ||
| echo "WARNING: Skipping copy — keeping the source frozen at original submission time." | ||
| echo "WARNING: Delete these directories manually if you intentionally want to refresh the frozen source." | ||
| else | ||
| echo "Copying src/post_training and scripts to run directory..." | ||
| mkdir -p "{{ run_dir }}/src" | ||
| cp -r "{{ repo_dir }}/src/post_training" "{{ run_dir }}/src/" | ||
| cp -r "{{ repo_dir }}/scripts" "{{ run_dir }}/scripts" | ||
| fi |
Comment on lines
+74
to
+83
| # ── Freeze source code for this run ─────────────────────────────────────── | ||
| if [ -d "{{ run_dir }}/src/post_training" ] || [ -d "{{ run_dir }}/scripts" ]; then | ||
| echo "WARNING: {{ run_dir }}/src/post_training or {{ run_dir }}/scripts already exists." | ||
| echo "WARNING: Skipping copy — keeping the source frozen at original submission time." | ||
| echo "WARNING: Delete these directories manually if you intentionally want to refresh the frozen source." | ||
| else | ||
| echo "Copying src/post_training and scripts to run directory..." | ||
| mkdir -p "{{ run_dir }}/src" | ||
| cp -r "{{ repo_dir }}/src/post_training" "{{ run_dir }}/src/" | ||
| cp -r "{{ repo_dir }}/scripts" "{{ run_dir }}/scripts" |
Comment on lines
+255
to
+266
| if config.backend == "trl" and config.container and config.container.image: | ||
| frozen_src_exists = (run_dir / "src" / "post_training").exists() | ||
| frozen_scripts_exists = (run_dir / "scripts").exists() | ||
| if frozen_src_exists or frozen_scripts_exists: | ||
| _row( | ||
| "Frozen source", | ||
| _yellow( | ||
| "*** src/post_training and/or scripts/ already exist in the run " | ||
| "directory — they will NOT be replaced ***" | ||
| ), | ||
| warn=True, | ||
| ) |
Comment on lines
+74
to
+84
| # ── Freeze source code for this run ─────────────────────────────────────── | ||
| if [ -d "{{ run_dir }}/src/post_training" ] || [ -d "{{ run_dir }}/scripts" ]; then | ||
| echo "WARNING: {{ run_dir }}/src/post_training or {{ run_dir }}/scripts already exists." | ||
| echo "WARNING: Skipping copy — keeping the source frozen at original submission time." | ||
| echo "WARNING: Delete these directories manually if you intentionally want to refresh the frozen source." | ||
| else | ||
| echo "Copying src/post_training and scripts to run directory..." | ||
| mkdir -p "{{ run_dir }}/src" | ||
| cp -r "{{ repo_dir }}/src/post_training" "{{ run_dir }}/src/" | ||
| cp -r "{{ repo_dir }}/scripts" "{{ run_dir }}/scripts" | ||
| fi |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The PR copies the
scriptsandsrc/post_trainingto the output directory and consumes those during the run. This has two benefits:Guardrails have also been added to warn the user if the run directory already contains
scriptsandsrc/post_training. In that case, the newer code is not copied over — the existing copies are left in place. This is based on the assumption that a given run should continue using the same codebase it started with.Type of change
Validation
Validated on LUMI with a short run with a checkpointing frequency of 1.