[train] Cleanup docs references to skyrl-train #1236
Conversation
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
![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 provides a comprehensive cleanup of the documentation, removing outdated references to skyrl-train and clarifying backend support for various features. The changes are extensive and largely accurate, updating file paths, commands, and adding helpful callouts. I've identified a couple of minor inconsistencies in the testing instructions that could be improved for better clarity.
| cd skyrl-gym | ||
| uv run --isolated --extra dev pytest tests/ |
There was a problem hiding this comment.
The instruction on line 33 states that "All commands below should be run from the repository root." However, this command block requires changing the directory to skyrl-gym, which could be confusing for users. To maintain consistency with the top-level instruction, consider using the --cwd flag of uv to run the command from the root directory.
# SkyRL-Gym
uv run --cwd skyrl-gym --isolated --extra dev pytest tests/
| cd skyrl-gym | ||
| uv run --isolated --extra dev pytest tests/ |
There was a problem hiding this comment.
Similar to the CPU tests for SkyRL-Gym, this command block requires changing the current directory, which contradicts the instruction on line 33 to run all commands from the repository root. For consistency, you could use the --cwd flag of uv.
uv run --cwd skyrl-gym --isolated --extra dev pytest tests/
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
🐛 1 issue in files not directly in the diff
🐛 Unconditional sleep() call after removing if self.colocate_all guard (skyrl-agent/skyrl_agent/integrations/skyrl_train/trainer.py:342)
The inference_engine_client.sleep() call is now invoked unconditionally, even when the inference engine runs on dedicated (disaggregated) GPUs. Previously, the call was guarded by if self.colocate_all:, ensuring sleep() was only called when training and inference shared the same GPUs.
Root Cause and Impact
In the old code, the sleep call was conditional:
if self.colocate_all:
await self.inference_engine_client.sleep()In the new code at skyrl-agent/skyrl_agent/integrations/skyrl_train/trainer.py:342, the guard is removed:
asyncio.run(self.inference_engine_client.sleep())When colocate_all=false (disaggregated deployment), calling sleep() on the inference engine is incorrect — it will shut down the inference engine's GPU resources even though the engine is running on its own dedicated GPUs and doesn't need to yield memory. This is inconsistent with the other sleep()/wake_up() calls in the same file (e.g., at skyrl-agent/skyrl_agent/integrations/skyrl_train/trainer.py:446-448) which still check if self.colocate_all.
Impact: When running with colocate_all=false, the inference engine will be unnecessarily slept after each generation batch, potentially causing errors or requiring an extra wake-up cycle before the next weight sync, degrading performance or causing failures.
View 12 additional findings in Devin Review.
… from skyrl-train migration (#1246) ## Summary Fix stale test docstrings and remaining docs references left over from the `skyrl-train` → `skyrl/train` migration (follow-up to #1236, #1187). ### Tests (`tests/backends/skyrl_train/`) - **`--extra vllm` → `--extra fsdp`** in 11 test file docstrings — the `vllm` extra was renamed to `fsdp` during the migration - **`tests/gpu/` → `tests/backends/skyrl_train/gpu/`** in 26 test file docstrings — paths were stale after the test directory was relocated - **`inf_engines/` → `inference_engines/`** in 2 test file docstrings — abbreviated directory name was never updated - **`skyrl-train/skyrl_train/` → `skyrl/backends/skyrl_train/`** in 1 test file docstring — old package directory reference ### Docs (`docs/content/docs/`) - **`DictConfig` → `SkyRLTrainConfig`** in entrypoint function signatures (`custom_algorithms.mdx`, `dapo.mdx`, `new_env.mdx`) - **Remove `@hydra.main` boilerplate** — replaced with `SkyRLTrainConfig.from_cli_overrides()` pattern (`new_env.mdx`) - **Remove `+` prefix** from CLI overrides — no longer needed after Hydra removal (`dapo.mdx`, `harbor/index.mdx`) - **`${oc.env:HOME}` → `~`** for default paths — OmegaConf interpolations replaced with tilde (`checkpointing.mdx`) - **Fix stale GitHub URL** pointing to old `skyrl-train/skyrl_train/workers/worker.py` (`overview.mdx`) - **Remove Hydra searchpath syntax** and `+`/`++` config prefixes from Harbor example invocation (`harbor/index.mdx`) ## Test plan - [x] Verified no remaining `--extra vllm` in `tests/backends/skyrl_train/` - [x] Verified no remaining `tests/gpu/` paths in test docstrings - [x] Verified no remaining `DictConfig`, `@hydra.main`, `+generator.`, `+trainer.`, `${oc.env:HOME}`, or `skyrl-train/skyrl_train/` in docs <!-- devin-review-badge-begin --> --- <a href="https://app.devin.ai/review/novasky-ai/skyrl/pull/1246" target="_blank"> <picture> <source media="(prefers-color-scheme: dark)" srcset="https://static.devin.ai/assets/gh-open-in-devin-review-dark.svg?v=1"> <img src="https://static.devin.ai/assets/gh-open-in-devin-review-light.svg?v=1" alt="Open with Devin"> </picture> </a> <!-- devin-review-badge-end --> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 participants
What does this PR do?
Cleans up docs references to the older
skyrl-trainpackageAlso adds callouts to the tutorial, example and other relevant pages to highlight that they are limited to the fsdp and the megatron backend (jax backend has some limitations). I thought this is better wording than just saying that these are for the "skyrl-train" backend because that naming is a bit confusing.