[3/N] Add back skyrl-train and skyrl-tx code#1140
[3/N] Add back skyrl-train and skyrl-tx code#1140erictang000 merged 1 commit intoNovaSky-AI:mainfrom
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request re-introduces a significant amount of code for skyrl-train and skyrl-tx, including extensive documentation, examples, and CI configurations. The overall structure is well-organized. My review focuses on ensuring security best practices and script correctness. I've identified a potential security vulnerability regarding the handling of environment files containing secrets and a bug in one of the example run scripts. Please see the detailed comments for suggestions.
| .pytest_cache/ | ||
| build/ | ||
| dist/ | ||
| *.egg-info/ |
There was a problem hiding this comment.
To prevent accidental leakage of secrets, it's a good practice to ignore environment files that are meant to hold sensitive information. Files like .env.llm_judge and .env.miniswe contain placeholders for secrets or sensitive URLs and should not be tracked by git. Users can then create local copies from example files.
I recommend adding a rule to ignore all .env files, while keeping .env.example files. This can be done by renaming the secret-containing files to have a .example suffix and adding a rule like the following to your .gitignore.
*.egg-info/
# Ignore environment files that may contain secrets
.env*
!*.env.example
| # bash examples/mini_swe_agent/run_mini_swe_30B.sh | ||
|
|
||
| # ensure that all worker nodes can access this data directory | ||
| DATA_DIR="$DATA/data/swe_gym_subset" |
There was a problem hiding this comment.
The environment variable $DATA is used here but it's not defined within the script. This will likely cause the script to fail if $DATA is not set in the user's environment. Other scripts in this PR use $HOME as a default. To make the script more robust and self-contained, you should either define $DATA or use a more standard variable like $HOME.
| DATA_DIR="$DATA/data/swe_gym_subset" | |
| DATA_DIR="$HOME/data/swe_gym_subset" |
1 participant
Add back skyrl-train and skyrl-tx code for now while we ensure the migration to
skyrlis smooth.See #1138, #1139 for context.