[feat] Add Delta Weight Sync Support in SkyRL - #1935
Conversation
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
Signed-off-by: SumanthRH <sumanthrh@anyscale.com>
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
Signed-off-by: SumanthRH <sumanthrh99@gmail.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a new checkpoint-delta weight synchronization strategy ('Delta strategy') for non-colocated training and inference setups, allowing compressed XOR deltas to be published and fetched via shared filesystems or cloud storage (GCS/S3). The review feedback highlights critical bugs in both FSDP and Megatron workers where the persistent inference client is prematurely closed during non-delta syncs. Additionally, the reviewer pointed out several resource leaks (file descriptors and handles) during lock exceptions or mmap failures, high memory overhead when writing local tensors, and robustness issues concerning unhandled memory errors, uncleaned temporary files, and dataclass schema evolution.
| def acquire(self, blocking: bool = True) -> bool: | ||
| self.path.parent.mkdir(parents=True, exist_ok=True) | ||
| fd = os.open(self.path, os.O_CREAT | os.O_RDWR, 0o644) | ||
| flags = fcntl.LOCK_EX | ||
| if not blocking: | ||
| flags |= fcntl.LOCK_NB | ||
| try: | ||
| fcntl.flock(fd, flags) | ||
| except BlockingIOError: | ||
| os.close(fd) | ||
| return False | ||
| self._fd = fd | ||
| return True |
There was a problem hiding this comment.
Resource Leak: File Descriptor Leaked on flock Exception
If fcntl.flock(fd, flags) raises an exception other than BlockingIOError (for example, OSError with ENOLCK or EOPNOTSUPP on NFS filesystems that do not support locking), the file descriptor fd is leaked because it is never closed. Since NFS is explicitly supported as a target filesystem for delta weight sync, this is a highly probable failure mode.
def acquire(self, blocking: bool = True) -> bool:
self.path.parent.mkdir(parents=True, exist_ok=True)
fd = os.open(self.path, os.O_CREAT | os.O_RDWR, 0o644)
flags = fcntl.LOCK_EX
if not blocking:
flags |= fcntl.LOCK_NB
try:
fcntl.flock(fd, flags)
except BlockingIOError:
os.close(fd)
return False
except Exception:
os.close(fd)
raise
self._fd = fd
return True| def _copy_from_uri(uri: str, local_path: Path) -> None: | ||
| local_path.parent.mkdir(parents=True, exist_ok=True) | ||
| tmp_path = local_path.with_name(f".{local_path.name}.{os.getpid()}.tmp") | ||
| if tmp_path.exists(): | ||
| tmp_path.unlink() | ||
| if _is_cloud_uri(uri): | ||
| _run_cloud_cp(uri, str(tmp_path)) | ||
| else: | ||
| shutil.copy2(Path(uri), tmp_path) | ||
| os.replace(tmp_path, local_path) |
There was a problem hiding this comment.
Robustness: Clean Up Temporary Files on Copy Failure
If _run_cloud_cp or shutil.copy2 fails, the temporary file tmp_path is left behind in the checkpoint directory. Wrapping the copy and replace operations in a try...except block ensures that any stale temporary files are cleaned up on failure.
| def _copy_from_uri(uri: str, local_path: Path) -> None: | |
| local_path.parent.mkdir(parents=True, exist_ok=True) | |
| tmp_path = local_path.with_name(f".{local_path.name}.{os.getpid()}.tmp") | |
| if tmp_path.exists(): | |
| tmp_path.unlink() | |
| if _is_cloud_uri(uri): | |
| _run_cloud_cp(uri, str(tmp_path)) | |
| else: | |
| shutil.copy2(Path(uri), tmp_path) | |
| os.replace(tmp_path, local_path) | |
| def _copy_from_uri(uri: str, local_path: Path) -> None: | |
| local_path.parent.mkdir(parents=True, exist_ok=True) | |
| tmp_path = local_path.with_name(f".{local_path.name}.{os.getpid()}.tmp") | |
| if tmp_path.exists(): | |
| tmp_path.unlink() | |
| try: | |
| if _is_cloud_uri(uri): | |
| _run_cloud_cp(uri, str(tmp_path)) | |
| else: | |
| shutil.copy2(Path(uri), tmp_path) | |
| os.replace(tmp_path, local_path) | |
| except Exception: | |
| tmp_path.unlink(missing_ok=True) | |
| raise |
| ie_cfg: "InferenceEngineConfig", inference_world_size: Optional[int] = None | ||
| ie_cfg: "InferenceEngineConfig", | ||
| inference_world_size: Optional[int] = None, | ||
| base_model_path: Optional[str] = None, |
There was a problem hiding this comment.
Interface change: Added base_model_path because this is needed for the delta weight sync strategy
Debatable if we should upgrade this to the full TrainerConfig and pass that - but IMO we should keep the args for our abstractions as minimal as possible so I've just used base_model_path for now.
| with inference actors. | ||
| """ | ||
|
|
||
| handles_prefix_cache_reset: bool = False |
There was a problem hiding this comment.
Interface change: Added a class attribute handles_prefix_cache_reset to indicate whether the sender will handle resetting prefix cache internally.
| # reading partially-updated weights during the NCCL broadcast. | ||
| await self._inference_engine_client.pause_generation() | ||
| try: | ||
| if self.cfg.generator.inference_engine.weight_sync_backend == "delta": |
There was a problem hiding this comment.
Important change: Delta weight sync runs pause_generation internally - so we need to skip it in the controller/ WorkerDispatch method
What does this PR do?
Adds delta weight sync to SkyRL following #1903
Implementation
Test Plan
Tests by component
All CPU tests live in
tests/backends/skyrl_train/weight_sync/unless noted otherwise.DeltaTransferStrategybelow —test_delta_create_init_info_requires_sync_dir(requiredsync_dir) andtest_delta_create_init_info(checkpoint_load_formatvalidation). No direct unit test; see coverage gaps.test_transfer_strategies.py::test_delta_create_init_info: config →DeltaInitInfofield mapping, includingoverride_existing_receiverfor remote engines2.
test_transfer_strategies.py::test_delta_create_init_info_requires_sync_dir: raisesValueErrorwhen the delta sub-config is absent orsync_diris unsettest_delta_sender_seed_sync_skips_chunk_iteration: the first sync treatsbase_model_pathas version 0 and skips chunk extraction/publish entirely2.
test_delta_checkpoint_non_source_rank_drains_without_publishing: non-source ranks drain the chunk stream to drive the extractor's collectives without publishingtest_delta_checkpoint_payload_stores_xor_patch: payload is the XOR patch, andbase ^ patch == updatedbyte-for-byte2.
test_delta_checkpoint_publisher_converts_to_base_checkpoint_dtype: runtime bf16 tensors are normalized back to the base checkpoint's fp323.
test_delta_checkpoint_splits_payload_files_by_size:max_file_size_in_gbstarts a new safetensors file4. test_delta_checkpoint_skips_missing_lm_head_when_checkpoint_ties_embeddings: tied-embedding models skiplm_head.weightinstead of failing5.
test_delta_checkpoint_unchanged_publish_advances_version: an unchanged publish is not a no-op — it advances the version and writes an empty deltatest_delta_checkpoint_publisher_converts_to_base_checkpoint_dtype: assertsdtype,payload_key,checksum_algorithm == "xxh3-128"anduncompressed_num_bytes2.
test_delta_checkpoint_unchanged_publish_advances_version: asserts emptytensors/payload_fileson a no-change versioninference_servers/test_remote_inference_client.py::test_fetch_weights: fan-out of the pre-pause fetch to every servertest_delta_checkpoint_publish_fetch_and_reload_roundtrip: publish → fetch → reload; changed and unchanged tensors both land correctly, state advances to v12.
test_delta_checkpoint_replays_multiple_versions_for_late_join: a receiver joining at v0 replays v1 then v23.
test_local_checkpoint_store_fetch_is_single_writer_with_concurrent_ray_actors: concurrent Ray actors sharing a cache dir serialize on the file lock and download once4.
test_delta_checkpoint_checksum_failure_marks_write_in_progress: a corrupt manifest leaveswrite_in_progress=True, and a subsequent valid delta recovers5.
test_delta_checkpoint_vllm_multi_thread_safetensors_iterator_roundtrip:iter_tensorsround-trip under the multi-thread safetensors loader6.
test_safe_path_name_disambiguates_long_sibling_uris: per-version cache keys stay distinct when thesync_dirURI is long enough to be truncatedtest_delta_checkpoint_gcs_cli_publish_fetch_roundtrip: full publish/fetch round-trip for GCS with mocked transfer2.
test_delta_checkpoint_s3_cli_publish_fetch_roundtrip: similar test for S3Integration Tests
tests/backends/skyrl_train/gpu/gpu_ci/test_delta_weight_sync_e2e.py::test_delta_weight_sync_sparse_update_e2e, parametrized overfsdpandmegatron(the latter behind themegatronmarker).Runs
Qwen/Qwen3-0.6Bnon-colocated (1 trainer GPU, 1 vLLM engine at TP=1), applies a sparse weight perturbation between syncs to simulate a weight update on the trainer side.Other tests
test_prefix_cache_reset.py: Tests who resets the inference engines' prefix cache (PolicyWorker or WeightTransferSender)test_worker_dispatch.py::TestSaveWeights: Tests the new dispatcher branch insave_weights_for_samplerfor delta weight sync.E2E runs
I've tested Delta weight sync with disk, GCS and S3 based weight transfer with a small model (Qwen 1.5B Instruct) on the GSM8K dataset on 4 GPUs.
gsm8k-qwen1p5b-nccl: Baseline, NCCL based weight syncgsm8k-qwen1p5b-delta-disk: Delta based weight sync via shared diskgsm8k-qwen1p5b-delta-gcs: Delta based weight sync via GCSgsm8k-qwen1p5b-delta-s3: Delta based weight sync via S3gsm8k-qwen1p5b-delta-s3-disagg: Delta based weight sync via S3 in a disaggregated setup: trainer and inference nodes are in separate clusters.Performance
Here are some performance numbers for delta weight sync for training
Qwen/Qwen3.5-35B-A3Bon the DAPO recipe with delta weight sync via GCS on 2 8xB200 nodes (1 trainer, 1 inference node in a non-colocated setup) :sync_weights/fetch_weights/update_weightsThere are some known low hanging fruits (ex: fetch and publish are both handled by a single rank right now, weight loading is currently disk -> CPU -> GPU instead of disk -> GPU because of a vllm limitation)