Skip to content

Commit cd63131

Browse files
dyurk-lilaclaude
andcommitted
feat(profiler): fall back to local save_path for cloud ckpt_path; reject empty activities
Two robustness fixes from PR review: - Profiler.__init__ detects a cloud save_path (s3://, gs://, gcs:// via the existing is_cloud_path helper) and falls back to ./profiler_traces with a warning. save_path commonly defaults to {ckpt_path}/profiler_traces, and ckpt_path can be a cloud URI -- which torch.profiler can't write to, so the trace would otherwise be silently lost. - TorchProfilerConfig.validate rejects an empty `activities` list. It passed the membership check vacuously, but torch.profiler.profile(activities=[]) records nothing -- now it fails fast at startup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 0c637c3 commit cd63131

4 files changed

Lines changed: 30 additions & 0 deletions

File tree

skyrl/backends/skyrl_train/utils/profiler.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
import torch.distributed
55
from loguru import logger
66

7+
from skyrl.backends.skyrl_train.utils.io.io import is_cloud_path
8+
79
# Map config activity strings to torch ProfilerActivity members.
810
_ACTIVITY_MAP = {
911
"cpu": torch.profiler.ProfilerActivity.CPU,
@@ -61,6 +63,16 @@ def __init__(self, config, default_save_path: str = None):
6163
return
6264
self.config = config
6365
self.save_path = config.save_path or default_save_path or "./profiler_traces"
66+
# torch.profiler writes traces with the local filesystem only. ``save_path``
67+
# commonly defaults to ``{ckpt_path}/profiler_traces``, and ckpt_path can be a
68+
# cloud URI (s3://, gs://) -- which torch can't write to, so the trace would be
69+
# silently lost. Fall back to a local dir so profiling still produces output.
70+
if is_cloud_path(self.save_path):
71+
logger.warning(
72+
f"[Profiler] cloud save_path {self.save_path!r} is not writable by torch.profiler; "
73+
f"falling back to local './profiler_traces'."
74+
)
75+
self.save_path = "./profiler_traces"
6476
self.ranks = list(config.ranks)
6577
self.export_type = getattr(config, "export_type", "chrome_trace")
6678
self.rank = torch.distributed.get_rank() if torch.distributed.is_initialized() else 0

skyrl/train/config/config.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,10 @@ def validate(self) -> None:
216216
return
217217
if not self.ranks:
218218
raise ValueError("`torch_profiler_config.ranks` must be non-empty when profiling is enabled.")
219+
# An empty `activities` passes the membership check below vacuously, but
220+
# `torch.profiler.profile(activities=[])` records nothing -- fail fast instead.
221+
if not self.activities:
222+
raise ValueError("`torch_profiler_config.activities` must be non-empty when profiling is enabled.")
219223
bad_activities = [a for a in self.activities if a.lower() not in TORCH_PROFILER_ACTIVITIES]
220224
if bad_activities:
221225
raise ValueError(

tests/backends/skyrl_train/utils/test_profiler.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,14 @@ def test_default_save_path_used_when_none(tmp_path):
108108
assert prof.save_path == default
109109

110110

111+
def test_cloud_save_path_falls_back_to_local(tmp_path):
112+
# ckpt_path (and thus the derived default save_path) can be a cloud URI, which
113+
# torch.profiler can't write to. The wrapper must fall back to a local dir so
114+
# the trace isn't silently lost.
115+
prof = Profiler(_ProfCfg(save_path="s3://my-bucket/run/profiler_traces"), default_save_path=str(tmp_path))
116+
assert prof.save_path == "./profiler_traces"
117+
118+
111119
def test_kernel_summary_none_when_disabled(tmp_path):
112120
prof = Profiler(_ProfCfg(enable=False), default_save_path=str(tmp_path))
113121
assert prof.get_kernel_summary() is None

tests/train/test_config.py

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -287,6 +287,12 @@ def test_unknown_activity_rejected(self):
287287
with pytest.raises(ValueError, match=r"activities"):
288288
self._cfg(activities=["cpu", "gpu"]).validate()
289289

290+
def test_empty_activities_rejected(self):
291+
# An empty list profiles nothing; the membership check would pass it
292+
# vacuously, so it needs its own guard.
293+
with pytest.raises(ValueError, match=r"activities.*non-empty"):
294+
self._cfg(activities=[]).validate()
295+
290296
def test_activities_case_insensitive(self):
291297
self._cfg(activities=["CPU", "CUDA"]).validate() # must not raise
292298

0 commit comments

Comments
 (0)