-
Notifications
You must be signed in to change notification settings - Fork 576
fix(infra): reap orphaned workers and survive partial teardown #1585
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import os | ||
| import signal | ||
| import subprocess | ||
| import sys | ||
| import time | ||
|
|
@@ -256,8 +257,21 @@ def launch_sglang_server(argv): | |
|
|
||
|
|
||
| def main(argv): | ||
| # SIGTERM terminates Python without running finally blocks, orphaning the | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main thread is blocked within the |
||
| # sglang child tree (its multiprocessing scheduler and detokenizer workers) | ||
| # and leaving the node occupied. Convert SIGTERM/SIGINT into SystemExit so | ||
| # the finally-block kill_process_tree below reaps the whole tree. | ||
| def _term_handler(signum, _frame): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| logger.warning(f"sglang launcher received signal {signum}, cleaning up...") | ||
| raise SystemExit(128 + signum) | ||
|
|
||
| signal.signal(signal.SIGTERM, _term_handler) | ||
| signal.signal(signal.SIGINT, _term_handler) | ||
|
|
||
| try: | ||
| launch_sglang_server(argv) | ||
| except SystemExit: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| raise | ||
| except Exception: | ||
| logger.error(traceback.format_exc()) | ||
| sys.exit(1) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -880,24 +880,47 @@ def train( | |
| self._save_perf_tracer(step=global_step) | ||
|
|
||
| def close(self): | ||
| self.saver.finalize() | ||
| if hasattr(self, "_train_rdataset") and self._train_rdataset is not None: | ||
| self._train_rdataset.close() | ||
| if hasattr(self, "_valid_rdataset") and self._valid_rdataset is not None: | ||
| self._valid_rdataset.close() | ||
| if hasattr(self, "data_controller") and self.data_controller is not None: | ||
| self.data_controller.destroy() | ||
| self.stats_logger.close() | ||
| if self.eval_rollout is not None: | ||
| self.eval_rollout.destroy() | ||
| self.rollout.destroy() | ||
| if self.teacher is not None: | ||
| self.teacher.destroy() | ||
| if self.ref is not None: | ||
| self.ref.destroy() | ||
| if self.critic is not None: | ||
| self.critic.destroy() | ||
| self.actor.destroy() | ||
| # Must tolerate a partially-constructed trainer (called from | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no call to self.close() in PPOTrainer.init. The only caller is exit, what is the triggering condition for the issue being fixed here? |
||
| # __init__'s failure path), and one engine's destroy() failure must | ||
| # not keep the remaining workers alive. | ||
| saver = getattr(self, "saver", None) | ||
| if saver is not None: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All component failures—including |
||
| try: | ||
| saver.finalize() | ||
| except Exception: | ||
| logger.warning("saver.finalize() failed during close", exc_info=True) | ||
| for attr in ("_train_rdataset", "_valid_rdataset"): | ||
| rdataset = getattr(self, attr, None) | ||
| if rdataset is not None: | ||
| try: | ||
| rdataset.close() | ||
| except Exception: | ||
| logger.warning(f"{attr}.close() failed during close", exc_info=True) | ||
| data_controller = getattr(self, "data_controller", None) | ||
| if data_controller is not None: | ||
| try: | ||
| data_controller.destroy() | ||
| except Exception: | ||
| logger.warning( | ||
| "data_controller.destroy() failed during close", exc_info=True | ||
| ) | ||
| stats_logger = getattr(self, "stats_logger", None) | ||
| if stats_logger is not None: | ||
| try: | ||
| stats_logger.close() | ||
| except Exception: | ||
| logger.warning( | ||
| "stats_logger.close() failed during close", exc_info=True | ||
| ) | ||
| for attr in ("eval_rollout", "rollout", "teacher", "ref", "critic", "actor"): | ||
| engine = getattr(self, attr, None) | ||
| if engine is not None: | ||
| try: | ||
| engine.destroy() | ||
| except Exception: | ||
| logger.warning( | ||
| f"{attr}.destroy() failed during close", exc_info=True | ||
| ) | ||
| perf_tracer.save(force=True) | ||
|
|
||
| def _config_perf_tracer(self): | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,64 @@ | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| """Tests for worker teardown and long-op robustness.""" | ||
|
|
||
| import os | ||
| import signal | ||
| from types import SimpleNamespace | ||
| from unittest import mock | ||
|
|
||
| import pytest | ||
|
|
||
| from areal.infra.launcher import sglang_server | ||
| from areal.trainer.rl_trainer import PPOTrainer | ||
|
|
||
|
|
||
| class TestSglangLauncherReapsItsChildren: | ||
| def test_sigterm_is_converted_into_systemexit(self): | ||
| with ( | ||
| mock.patch.object(sglang_server, "launch_sglang_server") as launch, | ||
| mock.patch.object(sglang_server, "kill_process_tree") as reap, | ||
| ): | ||
| launch.side_effect = lambda argv: os.kill(os.getpid(), signal.SIGTERM) | ||
|
|
||
| with pytest.raises(SystemExit) as exc: | ||
| sglang_server.main([]) | ||
|
|
||
| assert exc.value.code == 128 + signal.SIGTERM | ||
| reap.assert_called_once() | ||
|
|
||
| def test_unexpected_errors_still_reap_the_tree(self): | ||
| with ( | ||
| mock.patch.object(sglang_server, "launch_sglang_server") as launch, | ||
| mock.patch.object(sglang_server, "kill_process_tree") as reap, | ||
| ): | ||
| launch.side_effect = RuntimeError("boom") | ||
|
|
||
| with pytest.raises(SystemExit): | ||
| sglang_server.main([]) | ||
|
|
||
| reap.assert_called_once() | ||
|
|
||
|
|
||
| class TestTrainerCloseToleratesPartialConstruction: | ||
| def test_close_on_a_bare_trainer_does_not_raise(self): | ||
| trainer = object.__new__(PPOTrainer) | ||
|
|
||
| PPOTrainer.close(trainer) | ||
|
|
||
| def test_one_failing_component_does_not_skip_the_others(self): | ||
| trainer = object.__new__(PPOTrainer) | ||
| closed = [] | ||
| trainer.saver = SimpleNamespace( | ||
| finalize=lambda: (_ for _ in ()).throw(RuntimeError("saver down")) | ||
| ) | ||
| trainer.stats_logger = SimpleNamespace(close=lambda: closed.append("stats")) | ||
|
|
||
| PPOTrainer.close(trainer) | ||
|
|
||
| assert "stats" in closed, ( | ||
| "a failing saver prevented the remaining components from closing" | ||
| ) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| pytest.main([__file__, "-v"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
_call_workersfunction distributes thesaveoperation to all workers usingasyncio.gather, while theengine'ssaveinternally performs an all-gather TP operation—a collective communication. The scheduler's retry loop will resend the same POST onTimeoutError,ClientConnectionError, HTTP 503, and HTTP 500s containing "timeout." Only "connection refused" indicates that the request wasn't delivered; the others don't. TheEngine'ssave/loadlacks request IDs or deduplication mechanisms. A replay causes a worker to enter all-gather twice, while its peer rank enters only once → mismatched collective communication → NCCL deadlock, plus repeated writes to the same checkpoint path.