[docs] Add slime quickstart guide - #221
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new guide for LLM post-training using the SLIME framework on Kinetic, covering environment setup, Docker image building, and job submission. The review feedback suggests improving the efficiency of checkpoint uploads by using parallel transfers and ensuring script robustness in non-interactive environments by using full command names instead of shell aliases.
| def _upload_directory_to_gcs(local_dir: str, gcs_dir: str) -> None: | ||
| from google.cloud import storage | ||
|
|
||
| if not gcs_dir.startswith("gs://"): | ||
| raise ValueError(f"Expected a gs:// output path, got {gcs_dir!r}") | ||
|
|
||
| bucket_name, _, prefix = gcs_dir[5:].partition("/") | ||
| client = storage.Client() | ||
| bucket = client.bucket(bucket_name) | ||
| local_root = Path(local_dir) | ||
|
|
||
| for path in local_root.rglob("*"): | ||
| if path.is_file(): | ||
| rel = path.relative_to(local_root).as_posix() | ||
| blob = bucket.blob(f"{prefix.rstrip('/')}/{rel}") | ||
| blob.upload_from_filename(str(path)) |
There was a problem hiding this comment.
The current implementation of _upload_directory_to_gcs is inefficient for LLM checkpoints as it uploads files sequentially. Additionally, it has a bug where it creates blobs with a leading slash if the GCS path is a root bucket (e.g., gs://my-bucket), and it relies on global imports which can be brittle during remote serialization.
I recommend using google.cloud.storage.transfer_manager for parallel uploads and making the function self-contained with its imports.
def _upload_directory_to_gcs(local_dir: str, gcs_dir: str) -> None:
from pathlib import Path
from google.cloud import storage
from google.cloud.storage import transfer_manager
if not gcs_dir.startswith("gs://"):
raise ValueError(f"Expected a gs:// output path, got {gcs_dir!r}")
bucket_name, _, prefix = gcs_dir[5:].partition("/")
prefix = prefix.strip("/")
client = storage.Client()
bucket = client.bucket(bucket_name)
local_root = Path(local_dir)
# Collect all file paths relative to the local root
files = [
str(p.relative_to(local_root))
for p in local_root.rglob("*") if p.is_file()
]
transfer_manager.upload_many_from_filenames(
bucket,
files,
source_directory=local_dir,
blob_name_prefix=f"{prefix}/" if prefix else "",
worker_type=transfer_manager.THREAD,
)There was a problem hiding this comment.
You should use the data API for this if possible:
https://kinetic.readthedocs.io/en/latest/guides/data.html
| hf download zai-org/GLM-Z1-9B-0414 \ | ||
| --local-dir /root/GLM-Z1-9B-0414 | ||
| hf download --repo-type dataset zhuzilin/dapo-math-17k \ | ||
| --local-dir /root/dapo-math-17k | ||
| hf download --repo-type dataset zhuzilin/aime-2024 \ | ||
| --local-dir /root/aime-2024 |
There was a problem hiding this comment.
The command hf is typically an alias for huggingface-cli. In a non-interactive shell environment like a Kinetic pod, this alias may not be available. It is safer to use the full command name huggingface-cli to ensure the script runs correctly.
| hf download zai-org/GLM-Z1-9B-0414 \ | |
| --local-dir /root/GLM-Z1-9B-0414 | |
| hf download --repo-type dataset zhuzilin/dapo-math-17k \ | |
| --local-dir /root/dapo-math-17k | |
| hf download --repo-type dataset zhuzilin/aime-2024 \ | |
| --local-dir /root/aime-2024 | |
| huggingface-cli download zai-org/GLM-Z1-9B-0414 \ | |
| --local-dir /root/GLM-Z1-9B-0414 | |
| huggingface-cli download --repo-type dataset zhuzilin/dapo-math-17k \ | |
| --local-dir /root/dapo-math-17k | |
| huggingface-cli download --repo-type dataset zhuzilin/aime-2024 \ | |
| --local-dir /root/aime-2024 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #221 +/- ##
=======================================
Coverage ? 90.52%
=======================================
Files ? 67
Lines ? 10153
Branches ? 0
=======================================
Hits ? 9191
Misses ? 962
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jeffcarp
left a comment
There was a problem hiding this comment.
Thanks so much! Left some comments
| guides/distributed_training | ||
| advanced/batched_jobs | ||
| guides/llm_finetuning | ||
| guides/llm_post_training |
There was a problem hiding this comment.
The doc structure was recombobulated in #210 - can you move this to docs/examples?
|
Done! @jeffcarp |
| def _upload_directory_to_gcs(local_dir: str, gcs_dir: str) -> None: | ||
| from pathlib import Path | ||
| from google.cloud import storage | ||
| from google.cloud.storage import transfer_manager | ||
|
|
||
| if not gcs_dir.startswith("gs://"): | ||
| raise ValueError(f"Expected a gs:// output path, got {gcs_dir!r}") | ||
|
|
||
| bucket_name, _, prefix = gcs_dir[5:].partition("/") | ||
| prefix = prefix.strip("/") | ||
|
|
||
| client = storage.Client() | ||
| bucket = client.bucket(bucket_name) | ||
| local_root = Path(local_dir) | ||
|
|
||
| # Collect all file paths relative to the local root | ||
| files = [ | ||
| str(p.relative_to(local_root)) | ||
| for p in local_root.rglob("*") if p.is_file() | ||
| ] | ||
|
|
||
| transfer_manager.upload_many_from_filenames( | ||
| bucket, | ||
| files, | ||
| source_directory=local_dir, | ||
| blob_name_prefix=f"{prefix}/" if prefix else "", | ||
| worker_type=transfer_manager.THREAD, | ||
| ) |
There was a problem hiding this comment.
Can we use the Data() API here? https://kinetic.readthedocs.io/en/latest/guides/data.html
There was a problem hiding this comment.
Let's remove this function in that case and use the Data API instead.
There was a problem hiding this comment.
Please follow the repository conventions when writing example guides. The script should be present as a python file under the examples directory at the root of the repo and that file can directly be referenced here in the documentation using the following directive:
```{literalinclude} ../../examples/llm_post_training.py
:language: python```
… helper function and used Data(...)
|
@jeffcarp @JyotinderSingh the changes are done, please review |
|
@jeffcarp the renaming is done. |
|
@jeffcarp @JyotinderSingh any changes required? |
The SLIME quickstart guide has been added. It has been ensured that steps work for kinetic.