Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 28 additions & 1 deletion skyrl/train/utils/utils.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import faulthandler
import functools
import ipaddress
import logging
import math
import os
import socket
import sys
import threading
import time
from copy import deepcopy
from datetime import datetime
Expand All @@ -29,6 +31,23 @@
from skyrl.train.config.config import SkyRLTrainConfig


def _start_ray_init_watchdog(timeout_s: float) -> threading.Event:
"""Exit the driver if Ray initialization stops making progress."""
completed = threading.Event()
if timeout_s <= 0:
return completed
Comment on lines +37 to +38

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If timeout_s is NaN (e.g., if parsed from a malformed float representation), the condition timeout_s <= 0 will evaluate to False. Subsequently, calling completed.wait(timeout_s) with NaN will raise a ValueError in Python's threading library, crashing the watchdog thread.

Add a check for math.isnan(timeout_s) to safely handle this edge case.

Suggested change
if timeout_s <= 0:
return completed
if timeout_s <= 0 or math.isnan(timeout_s):
return completed


def watchdog() -> None:
if completed.wait(timeout_s):
return
logger.error(f"ray.init() did not complete within {timeout_s:g}s; terminating the driver")
faulthandler.dump_traceback(file=sys.stderr, all_threads=True)
os._exit(1)
Comment on lines +43 to +45

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If sys.stderr is redirected or replaced with a custom stream that does not support a file descriptor (e.g., in certain application servers, container environments, or testing frameworks), faulthandler.dump_traceback will raise a RuntimeError or AttributeError.

If an exception is raised here, the watchdog thread will crash before executing os._exit(1), leaving the stalled process hanging indefinitely.

Wrap the traceback dump in a try...except block to ensure that the driver is guaranteed to terminate even if the traceback dump fails.

Suggested change
logger.error(f"ray.init() did not complete within {timeout_s:g}s; terminating the driver")
faulthandler.dump_traceback(file=sys.stderr, all_threads=True)
os._exit(1)
logger.error(f"ray.init() did not complete within {timeout_s:g}s; terminating the driver")
try:
faulthandler.dump_traceback(file=sys.stderr, all_threads=True)
except Exception as e:
logger.error(f"Failed to dump traceback: {e}")
os._exit(1)


threading.Thread(target=watchdog, name="skyrl-ray-init-watchdog", daemon=True).start()
return completed


class Timer:
def __init__(self, message, update_dict=None):
self.message = message
Expand Down Expand Up @@ -918,7 +937,15 @@ def initialize_ray(cfg: SkyRLTrainConfig):

# log_to_driver=True allows training progress from skyrl_entrypoint to reach stdout.
# Infrastructure logs (vLLM, workers) are redirected to log file via os.dup2 in their init.
ray.init(runtime_env={"env_vars": env_vars}, log_to_driver=True)
ray_init_timeout_s = float(os.environ.get("SKYRL_RAY_INIT_TIMEOUT_IN_S", "0"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Parsing the environment variable SKYRL_RAY_INIT_TIMEOUT_IN_S directly with float() without handling potential parsing errors can cause the driver to crash with a ValueError if the variable is set to an empty string or an invalid non-numeric value (which is common in templated deployments).

Safely parse the environment variable with a try...except ValueError block and default to 0.0 (disabled) with a warning log.

Suggested change
ray_init_timeout_s = float(os.environ.get("SKYRL_RAY_INIT_TIMEOUT_IN_S", "0"))
ray_init_timeout_env = os.environ.get("SKYRL_RAY_INIT_TIMEOUT_IN_S", "0")
try:
ray_init_timeout_s = float(ray_init_timeout_env) if ray_init_timeout_env.strip() else 0.0
except ValueError:
logger.warning(
f"Invalid SKYRL_RAY_INIT_TIMEOUT_IN_S value: {ray_init_timeout_env!r}. "
"Disabling the watchdog (timeout=0)."
)
ray_init_timeout_s = 0.0

logger.info(f"Starting ray.init() (timeout={ray_init_timeout_s:g}s; 0 disables the watchdog)")
started_at = time.monotonic()
ray_init_completed = _start_ray_init_watchdog(ray_init_timeout_s)
try:
ray.init(runtime_env={"env_vars": env_vars}, log_to_driver=True)
finally:
ray_init_completed.set()
logger.info(f"ray.init() completed in {time.monotonic() - started_at:.1f}s")

if not verbose_logging:
logger.info(f"Infrastructure logs will be written to: {log_file}")
Expand Down
29 changes: 29 additions & 0 deletions tests/train/utils/test_initialize_ray.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
import threading

from skyrl.train.utils import utils


def test_ray_init_watchdog_exits_after_timeout(monkeypatch):
exited = threading.Event()
exit_codes = []

def fake_exit(code):
exit_codes.append(code)
exited.set()

monkeypatch.setattr(utils.faulthandler, "dump_traceback", lambda **kwargs: None)
monkeypatch.setattr(utils.os, "_exit", fake_exit)

completed = utils._start_ray_init_watchdog(0.01)

assert exited.wait(timeout=1)
assert exit_codes == [1]
completed.set()


def test_ray_init_watchdog_can_be_disabled(monkeypatch):
monkeypatch.setattr(utils.os, "_exit", lambda code: (_ for _ in ()).throw(AssertionError(code)))

completed = utils._start_ray_init_watchdog(0)

assert not completed.is_set()
Loading