[CI] Fix resource leak when setUpClass fails (#21338)
This commit is contained in:
@@ -9,10 +9,11 @@ description: Guide for writing SGLang CI/UT tests. Covers CustomTestCase, CI reg
|
||||
|
||||
## Core Rules
|
||||
|
||||
1. **Always use `CustomTestCase`** — never raw `unittest.TestCase`
|
||||
2. **Place tests in `test/registered/<category>/`** — except JIT kernel tests and benchmarks, which live in `python/sglang/jit_kernel/tests/` and `python/sglang/jit_kernel/benchmark/`
|
||||
3. **Reuse server fixtures** — inherit from `DefaultServerBase` or write `setUpClass`/`tearDownClass` with `popen_launch_server`
|
||||
4. **Prefer mock over real server** — when testing logic that doesn't need a server / engine launch (middleware, request routing, config validation, argument parsing), use `unittest.mock.patch` / `MagicMock` and place tests in `test/registered/unit/`. Only launch a real server when the test genuinely needs inference results or server lifecycle behavior.
|
||||
1. **Always use `CustomTestCase`** — never raw `unittest.TestCase`. It ensures `tearDownClass` runs even when `setUpClass` fails, preventing resource leaks in CI.
|
||||
2. **`tearDownClass` must be defensive** — use `hasattr`/null checks before accessing resources (e.g. `cls.process`) that `setUpClass` may not have finished allocating.
|
||||
3. **Place tests in `test/registered/<category>/`** — except JIT kernel tests and benchmarks, which live in `python/sglang/jit_kernel/tests/` and `python/sglang/jit_kernel/benchmark/`
|
||||
4. **Reuse server fixtures** — inherit from `DefaultServerBase` or write `setUpClass`/`tearDownClass` with `popen_launch_server`
|
||||
5. **Prefer mock over real server** — when testing logic that doesn't need a server / engine launch (middleware, request routing, config validation, argument parsing), use `unittest.mock.patch` / `MagicMock` and place tests in `test/registered/unit/`. Only launch a real server when the test genuinely needs inference results or server lifecycle behavior.
|
||||
|
||||
JIT kernel exception:
|
||||
- If the task is adding or updating code under `python/sglang/jit_kernel/`, prefer the `add-jit-kernel` skill first.
|
||||
@@ -135,7 +136,8 @@ class TestMyFeature(CustomTestCase):
|
||||
|
||||
@classmethod
|
||||
def tearDownClass(cls):
|
||||
kill_process_tree(cls.process.pid)
|
||||
if hasattr(cls, "process") and cls.process:
|
||||
kill_process_tree(cls.process.pid)
|
||||
|
||||
def test_basic_functionality(self):
|
||||
response = requests.post(
|
||||
@@ -183,7 +185,8 @@ class TestMyFeaturePerf(CustomTestCase):
|
||||
|
||||
@classmethod
|
||||
def tearDownClass(cls):
|
||||
kill_process_tree(cls.process.pid)
|
||||
if hasattr(cls, "process") and cls.process:
|
||||
kill_process_tree(cls.process.pid)
|
||||
|
||||
def test_latency(self):
|
||||
start = time.perf_counter()
|
||||
@@ -311,5 +314,6 @@ Before submitting a test:
|
||||
- [ ] Backend-independent tests: `register_cuda_ci` only + smallest model
|
||||
- [ ] Logic that doesn't need a server / engine launch → unit test in `registered/unit/` (see Unit Tests section)
|
||||
- [ ] `setUpClass` launches server, `tearDownClass` kills it (if server-based)
|
||||
- [ ] `tearDownClass` is defensive — uses `hasattr`/null checks before accessing resources that may not have been allocated
|
||||
- [ ] Has `if __name__ == "__main__": unittest.main()`
|
||||
- [ ] `est_time` is reasonable (measure locally)
|
||||
|
||||
@@ -2083,6 +2083,33 @@ def _distributed_worker(rank, world_size, backend, port, func, result_queue, kwa
|
||||
|
||||
class CustomTestCase(unittest.TestCase):
|
||||
|
||||
def __init_subclass__(cls, **kwargs):
|
||||
super().__init_subclass__(**kwargs)
|
||||
|
||||
# Wrap the effective setUpClass so that tearDownClass is called
|
||||
# even when setUpClass fails. Python's unittest skips tearDownClass
|
||||
# if setUpClass raises, which can leak resources (ports, processes).
|
||||
setup = cls.setUpClass
|
||||
if getattr(setup, "_safe_setup_wrapped", False):
|
||||
return
|
||||
|
||||
def safe_setUpClass(klass, _orig=setup):
|
||||
try:
|
||||
_orig.__func__(klass)
|
||||
except Exception:
|
||||
# Best-effort cleanup; suppress teardown errors so the
|
||||
# original setUpClass exception propagates clearly.
|
||||
try:
|
||||
klass.tearDownClass()
|
||||
except Exception:
|
||||
pass
|
||||
raise
|
||||
|
||||
# Set sentinel on the raw function so that bound method attribute
|
||||
# lookup (which delegates to __func__) can detect it in subclasses.
|
||||
safe_setUpClass._safe_setup_wrapped = True
|
||||
cls.setUpClass = classmethod(safe_setUpClass)
|
||||
|
||||
def _callTestMethod(self, method):
|
||||
max_retry = envs.SGLANG_TEST_MAX_RETRY.get()
|
||||
if max_retry is None:
|
||||
|
||||
@@ -269,6 +269,7 @@ A scheduled job summarizes test coverage across all backends; [here is an exampl
|
||||
|
||||
## Tips for Writing Elegant Test Cases
|
||||
- Learn from existing examples in [test/registered](https://github.com/sgl-project/sglang/tree/main/test/registered).
|
||||
- **Always use `CustomTestCase`** instead of raw `unittest.TestCase`, and make `tearDownClass` defensive (`hasattr` checks).
|
||||
- Reduce the test time by using smaller models and reusing the server for multiple test cases. Launching a server takes a lot of time, so please reuse a single server for many tests instead of launching many servers.
|
||||
- Use as few GPUs as possible. Use 1-GPU runners whenever possible. Do not run long tests with 8-gpu runners.
|
||||
- If the test cases take too long, consider adding them to nightly tests instead of per-commit tests.
|
||||
|
||||
@@ -63,11 +63,13 @@ class HiCacheStorageBaseMixin:
|
||||
@classmethod
|
||||
def tearDownClass(cls):
|
||||
"""Clean up test environment"""
|
||||
kill_process_tree(cls.process.pid)
|
||||
if hasattr(cls, "process") and cls.process:
|
||||
kill_process_tree(cls.process.pid)
|
||||
|
||||
import shutil
|
||||
|
||||
shutil.rmtree(cls.temp_dir, ignore_errors=True)
|
||||
if hasattr(cls, "temp_dir"):
|
||||
shutil.rmtree(cls.temp_dir, ignore_errors=True)
|
||||
|
||||
@classmethod
|
||||
def _get_model_name(cls):
|
||||
|
||||
Reference in New Issue
Block a user