[2/N] Add back skyrl code to top level and delete skyrl-train and skyrl-tx to get git history#1139
Conversation
![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 is a large refactoring to merge skyrl-train and skyrl-tx into a single top-level skyrl package. Most of the changes involve moving files and updating import paths, which seem to be handled correctly. I've found a few minor issues related to duplicate entries in the .gitignore file and a potential typo in the Ray version in CI configuration files. Overall, the refactoring appears well-executed.
| image_uri: novaskyai/skyrl-train-ray-2.51.1-py3.12-cu12.8 | ||
| cloud: sky-anyscale-aws-us-east-1 | ||
| ray_version: "2.51.1" |
There was a problem hiding this comment.
The Ray version 2.51.1 in both image_uri and ray_version appears to be a typo and is likely an invalid version string, which could cause the CI job to fail. Please verify and correct the version number. For example, it might be intended to be 2.5.1.
| image_uri: novaskyai/skyrl-train-ray-2.51.1-py3.12-cu12.8-megatron | ||
| cloud: sky-anyscale-aws-us-east-1 | ||
| ray_version: "2.51.1" |
There was a problem hiding this comment.
The Ray version 2.51.1 in both image_uri and ray_version appears to be a typo and is likely an invalid version string, which could cause the CI job to fail. Please verify and correct the version number. For example, it might be intended to be 2.5.1.
| # Byte-compiled / optimized / DLL files | ||
| __pycache__/ | ||
| *.py[cod] | ||
| *$py.class | ||
|
|
||
| # C extensions | ||
| *.so | ||
|
|
||
| # Distribution / packaging | ||
| .Python | ||
| build/ | ||
| develop-eggs/ | ||
| dist/ | ||
| downloads/ | ||
| eggs/ | ||
| .eggs/ | ||
| lib/ | ||
| lib64/ | ||
| parts/ | ||
| sdist/ | ||
| var/ | ||
| wheels/ | ||
| pip-wheel-metadata/ | ||
| share/python-wheels/ | ||
| *.egg-info/ | ||
| .installed.cfg | ||
| *.egg | ||
| MANIFEST | ||
| uv.lock | ||
|
|
||
| # PyInstaller | ||
| *.manifest | ||
| *.spec | ||
|
|
||
| # Installer logs | ||
| pip-log.txt | ||
| pip-delete-this-directory.txt | ||
|
|
||
| # Unit test / coverage reports | ||
| htmlcov/ | ||
| .tox/ | ||
| .nox/ | ||
| .coverage | ||
| .coverage.* | ||
| .cache | ||
| nosetests.xml | ||
| coverage.xml | ||
| *.cover | ||
| *.py,cover | ||
| .hypothesis/ | ||
| .pytest_cache/ | ||
|
|
||
| # Jupyter Notebook | ||
| .ipynb_checkpoints | ||
|
|
||
| # Environments | ||
| .env | ||
| .venv | ||
| env/ | ||
| venv/ | ||
| ENV/ | ||
| env.bak/ | ||
| venv.bak/ | ||
|
|
||
| # MkDocs build output | ||
| site/ | ||
|
|
||
| # IDEs and editors | ||
| .idea/ | ||
| .vscode/ | ||
|
|
||
| # OS generated files | ||
| .DS_Store | ||
| Thumbs.db | ||
|
|
||
| # Hydra outputs | ||
| outputs/ | ||
|
|
||
| # Local artifacts | ||
| tinker.db | ||
| uv.lock | ||
|
|
||
| # Alembic - don't track pycache | ||
| tx/tinker/alembic/__pycache__/ | ||
|
|
||
| # SQLite databases (tracked in git by default, but ignore if created locally) | ||
| *.db | ||
| *.db-journal | ||
| *.db-wal | ||
| *.db-shm |
There was a problem hiding this comment.
There are a few duplicate entries in the new additions to the .gitignore file. Consolidating them would make the file cleaner and easier to maintain.
uv.lockis duplicated on lines 79 and 131 (already present on line 49).*.dbis duplicated on line 137 (already present on line 46).
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
1 participant
See #1138 for the removal of
skyrlAnd see #1140, #1141 for follow ups.