Skip to content

Commit b84a6b9

Browse files
authored
Merge pull request #216 from unilabsim/fix/issue-169-quality-guardrails
fix: tighten quality guardrails for issue 169
2 parents 6f456a6 + b99f79e commit b84a6b9

9 files changed

Lines changed: 195 additions & 14 deletions

File tree

‎.github/ISSUE_TEMPLATE/bug_report.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ body:
3737
label: Reproduction
3838
description: Exact command, config, or sequence that reproduces the issue.
3939
placeholder: |
40-
uv run python scripts/train_offpolicy.py algo=sac task=g1_sac ...
40+
uv run python scripts/train_offpolicy.py algo=sac task=sac/g1_sac/mujoco ...
4141
validations:
4242
required: true
4343

‎.github/workflows/ci.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,4 +96,4 @@ jobs:
9696
- name: Install dependencies
9797
run: uv sync --extra motrix
9898
- name: Test with coverage
99-
run: uv run pytest -m "not slow and not veryslow" --cov=unilab --cov-report markdown-append:$GITHUB_STEP_SUMMARY --cov-fail-under=10
99+
run: uv run pytest -m "not slow and not veryslow" --cov=unilab --cov-report markdown-append:$GITHUB_STEP_SUMMARY --cov-fail-under=25

‎CONTRIBUTING.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,13 +98,15 @@ uv run pytest -m veryslow -v
9898

9999
PRs targeting `main` trigger five jobs automatically: `ruff-lint`, `ruff-format`, `mypy`, `pyright`, and `test`. The workflow also enables manual runs through `workflow_dispatch`, validates docs through the pytest suite on docs changes, and cancels older in-progress runs for the same PR branch.
100100

101+
Coverage policy: the default CI lane enforces a minimum non-slow coverage floor, and that floor should only ratchet upward when the suite meaningfully expands.
102+
101103
| Job | Content | Blocking on failure |
102104
|-----|---------|---------------------|
103105
| `ruff-lint` | `uv sync --only-group dev` + `uv run --no-sync ruff check --output-format=github .` on `ubuntu-slim` | ✅ |
104106
| `ruff-format` | `uv sync --only-group dev` + `uv run --no-sync ruff format --check .` on `ubuntu-slim` | ✅ |
105107
| `mypy` | `uv sync` + `uv run mypy src/unilab` on `macos-26` | ✅ |
106108
| `pyright` | `uv sync` + `uv run pyright` on `macos-26` | ✅ |
107-
| `test` | `uv sync --extra motrix` + `uv run pytest -m "not slow and not veryslow" --cov=unilab --cov-report markdown-append:$GITHUB_STEP_SUMMARY --cov-fail-under=10` on `ubuntu-slim` with Python 3.11 | ✅ |
109+
| `test` | `uv sync --extra motrix` + `uv run pytest -m "not slow and not veryslow" --cov=unilab --cov-report markdown-append:$GITHUB_STEP_SUMMARY --cov-fail-under=25` on `ubuntu-slim` with Python 3.11 | ✅ |
108110

109111
Collaboration-metadata-only changes such as `LICENSE`, issue templates, `CODEOWNERS`, and `.github/pull_request_template.md` still skip CI. Docs changes do trigger CI and are validated by `tests/scripts/test_check_docs.py`.
110112

‎docs/zh_CN/CONTRIBUTING.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,13 +98,15 @@ uv run pytest -m veryslow -v
9898

9999
指向 `main` 的 PR 会自动触发五个 job: `ruff-lint`、`ruff-format`、`mypy`、`pyright` 和 `test`。workflow 也支持通过 `workflow_dispatch` 手动触发;文档改动会通过 pytest 套件参与校验,并且会自动取消同一 PR 分支上较早的进行中运行。
100100

101+
覆盖率策略: 默认 CI lane 会对非 slow 测试施加最低覆盖率门槛,这个门槛只应随着测试护栏增强而逐步上调,不应回退。
102+
101103
| Job | 内容 | 失败是否阻断 |
102104
|-----|------|--------------|
103105
| `ruff-lint` | 在 `ubuntu-slim` 上执行 `uv sync --only-group dev` + `uv run --no-sync ruff check --output-format=github .` | ✅ |
104106
| `ruff-format` | 在 `ubuntu-slim` 上执行 `uv sync --only-group dev` + `uv run --no-sync ruff format --check .` | ✅ |
105107
| `mypy` | 在 `macos-26` 上执行 `uv sync` + `uv run mypy src/unilab` | ✅ |
106108
| `pyright` | 在 `macos-26` 上执行 `uv sync` + `uv run pyright` | ✅ |
107-
| `test` | 在 `ubuntu-slim` 上以 Python 3.11 执行 `uv sync --extra motrix` + `uv run pytest -m "not slow and not veryslow" --cov=unilab --cov-report markdown-append:$GITHUB_STEP_SUMMARY --cov-fail-under=10` | ✅ |
109+
| `test` | 在 `ubuntu-slim` 上以 Python 3.11 执行 `uv sync --extra motrix` + `uv run pytest -m "not slow and not veryslow" --cov=unilab --cov-report markdown-append:$GITHUB_STEP_SUMMARY --cov-fail-under=25` | ✅ |
108110

109111
只有协作元信息改动,例如 `LICENSE`、issue templates、`CODEOWNERS` 和 `.github/pull_request_template.md`,才会跳过 CI。文档改动会触发 CI,并由 `tests/scripts/test_check_docs.py` 校验。
110112

‎scripts/play_interactive.py‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,11 @@
2828
ROOT_DIR = Path(__file__).parent.parent
2929
sys.path.append(str(ROOT_DIR))
3030

31-
from unilab.training import ensure_registries, resolve_task_checkpoint_path
31+
from unilab.training import (
32+
ensure_registries,
33+
get_entrypoint_log_root,
34+
resolve_task_checkpoint_path,
35+
)
3236

3337
ensure_registries()
3438

@@ -397,10 +401,9 @@ def play_interactive(args):
397401
flat_obs_dim = int(sum(env.obs_groups_spec.values()))
398402

399403
policy_obs_mode = args.policy_obs_mode
404+
algo_log_name = getattr(args, "algo_log_name", "rsl_rl_ppo")
400405
ckpt = None
401406
if args.action_mode == "policy":
402-
# Get algo_log_name from config if available, otherwise use default
403-
algo_log_name = getattr(args, "algo_log_name", "rsl_rl_ppo")
404407
ckpt = resolve_checkpoint(
405408
args.task, args.load_run, getattr(args, "checkpoint", None), algo_log_name
406409
)
@@ -436,7 +439,11 @@ def play_interactive(args):
436439
if ckpt is None:
437440
print("[play_interactive] WARNING: no checkpoint found — falling back to zero actions.")
438441
else:
439-
log_dir = str(ROOT_DIR / "logs" / "rsl_rl_train" / args.task / "play_temp")
442+
log_dir = str(
443+
get_entrypoint_log_root(ROOT_DIR, algo_log_name=algo_log_name)
444+
/ args.task
445+
/ "play_temp"
446+
)
440447
runner = OnPolicyRunner(wrapped_env, train_cfg, log_dir=log_dir, device=device)
441448
runner.load(
442449
ckpt,

‎tests/ipc/test_async_runner.py‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
from __future__ import annotations
44

55
import multiprocessing as mp
6+
import signal
67
import time
78
from typing import Any
89
from unittest.mock import MagicMock
@@ -155,7 +156,12 @@ def _worker_wait_for_stop(stop_event) -> None:
155156

156157

157158
def test_close_joins_running_collector():
158-
"""close() must signal the stop event and join the collector process."""
159+
"""close() must signal the stop event and reap the collector process.
160+
161+
Under heavy CI load a spawned process may still be importing the test module
162+
when close() hits its timeout path, so SIGTERM is also an acceptable outcome
163+
as long as the collector does not leak.
164+
"""
159165
r = _make_runner()
160166
r._collector_process = _SPAWN_CTX.Process(
161167
target=_worker_wait_for_stop,
@@ -167,8 +173,9 @@ def test_close_joins_running_collector():
167173

168174
r.close()
169175

176+
assert r._stop_event.is_set()
170177
assert not r._collector_process.is_alive()
171-
assert r._collector_process.exitcode == 0
178+
assert r._collector_process.exitcode in (0, -signal.SIGTERM)
172179

173180

174181
# ---------------------------------------------------------------------------

‎tests/scripts/doc_checks.py‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -42,13 +42,15 @@
4242
"*.md",
4343
"docs/**/*.md",
4444
"src/**/*.md",
45+
".github/ISSUE_TEMPLATE/**/*.yml",
46+
".github/ISSUE_TEMPLATE/**/*.yaml",
4547
]
4648

4749
SKIP_PATTERNS = [
48-
r"\.git",
49-
r"\.venv",
50-
r"__pycache__",
51-
r"\.pytest_cache",
50+
r"(^|/)\.git(/|$)",
51+
r"(^|/)\.venv(/|$)",
52+
r"(^|/)__pycache__(/|$)",
53+
r"(^|/)\.pytest_cache(/|$)",
5254
]
5355

5456

@@ -67,6 +69,12 @@ def find_docs(root: Path) -> list[Path]:
6769
for path in root.glob(pattern):
6870
if path.is_file() and not should_skip(path):
6971
docs.append(path)
72+
issue_template_dir = root / ".github" / "ISSUE_TEMPLATE"
73+
if issue_template_dir.exists():
74+
for pattern in ("*.yml", "*.yaml"):
75+
for path in issue_template_dir.rglob(pattern):
76+
if path.is_file() and not should_skip(path):
77+
docs.append(path)
7078
return sorted(set(docs))
7179

7280

‎tests/scripts/test_check_docs.py‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,3 +49,32 @@ def test_check_script_references_ignores_tests_paths():
4949
errors = doc_checks.check_script_references(content, doc_path, root)
5050

5151
assert errors == []
52+
53+
54+
def test_collect_doc_errors_scans_issue_templates_for_hydra_semantics(tmp_path):
55+
issue_template = tmp_path / ".github" / "ISSUE_TEMPLATE" / "bug_report.yml"
56+
issue_template.parent.mkdir(parents=True)
57+
issue_template.write_text(
58+
"placeholder: |\n uv run python scripts/train_offpolicy.py algo=sac task=g1_sac ...\n",
59+
encoding="utf-8",
60+
)
61+
script_path = tmp_path / "scripts" / "train_offpolicy.py"
62+
script_path.parent.mkdir(parents=True)
63+
script_path.write_text("", encoding="utf-8")
64+
65+
errors = doc_checks.collect_doc_errors(tmp_path)
66+
67+
assert any("task=g1_sac" in error for error in errors)
68+
69+
70+
def test_collect_doc_errors_scans_issue_templates_for_script_paths(tmp_path):
71+
issue_template = tmp_path / ".github" / "ISSUE_TEMPLATE" / "bug_report.yml"
72+
issue_template.parent.mkdir(parents=True)
73+
issue_template.write_text(
74+
"placeholder: |\n uv run python scripts/missing_entrypoint.py task=go1_joystick/mujoco\n",
75+
encoding="utf-8",
76+
)
77+
78+
errors = doc_checks.collect_doc_errors(tmp_path)
79+
80+
assert any("Script not found: scripts/missing_entrypoint.py" in error for error in errors)

‎tests/scripts/test_train_scripts.py‎

Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
from typing import Any, cast
1515
from unittest.mock import MagicMock
1616

17+
import numpy as np
1718
import pytest
1819
from hydra import compose, initialize_config_dir
1920
from hydra.core.global_hydra import GlobalHydra
@@ -1051,6 +1052,131 @@ def test_play_resolve_checkpoint_uses_algo_log_name(tmp_path):
10511052
mod.ROOT_DIR = original_root
10521053

10531054

1055+
def test_play_interactive_runner_log_dir_uses_algo_log_name(monkeypatch: pytest.MonkeyPatch):
1056+
import types
1057+
1058+
mod = _play_interactive()
1059+
captured: dict[str, object] = {}
1060+
1061+
class FakeWrapper:
1062+
def __init__(self, env, device, policy_obs_mode):
1063+
self.env = env
1064+
captured["policy_obs_mode"] = policy_obs_mode
1065+
1066+
def reset(self):
1067+
return None, {}
1068+
1069+
class FakeRunner:
1070+
def __init__(self, wrapped_env, train_cfg, log_dir, device):
1071+
del wrapped_env, train_cfg, device
1072+
captured["log_dir"] = log_dir
1073+
1074+
def load(self, ckpt, load_cfg):
1075+
captured["ckpt"] = ckpt
1076+
captured["load_cfg"] = load_cfg
1077+
1078+
def get_inference_policy(self, device):
1079+
del device
1080+
return object()
1081+
1082+
class FakeViewer:
1083+
def __enter__(self):
1084+
return self
1085+
1086+
def __exit__(self, exc_type, exc, tb):
1087+
return False
1088+
1089+
def is_running(self):
1090+
return False
1091+
1092+
def sync(self):
1093+
pass
1094+
1095+
user_scn = type("Scene", (), {"ngeom": 0})()
1096+
1097+
fake_env = types.SimpleNamespace(
1098+
obs_groups_spec={"obs": 5},
1099+
action_space=types.SimpleNamespace(shape=(3,), low=np.full((3,), -1.0), high=np.ones((3,))),
1100+
cfg=types.SimpleNamespace(ctrl_dt=0.02),
1101+
_backend=types.SimpleNamespace(model=object()),
1102+
)
1103+
1104+
monkeypatch.setattr(mod.registry, "make", lambda *args, **kwargs: fake_env)
1105+
monkeypatch.setattr(mod, "resolve_checkpoint", lambda *args, **kwargs: "/tmp/model_10.pt")
1106+
monkeypatch.setattr(
1107+
mod,
1108+
"get_entrypoint_log_root",
1109+
lambda root_dir, *, algo_log_name, log_root=None: Path("/tmp") / algo_log_name,
1110+
)
1111+
monkeypatch.setattr(mod, "RslRlVecEnvWrapper", FakeWrapper)
1112+
monkeypatch.setattr(mod, "OnPolicyRunner", FakeRunner)
1113+
monkeypatch.setattr(mod, "PPOConfig", lambda: types.SimpleNamespace(to_dict=lambda: {}))
1114+
monkeypatch.setattr(mod, "is_rsl_rl_v4", lambda: False)
1115+
monkeypatch.setattr(mod, "convert_config_v3_to_v4", lambda cfg: cfg)
1116+
monkeypatch.setattr(mod.mujoco, "MjData", lambda model: object())
1117+
monkeypatch.setattr(mod.mujoco, "mj_setState", lambda *args, **kwargs: None)
1118+
monkeypatch.setattr(mod.mujoco, "mj_forward", lambda *args, **kwargs: None)
1119+
monkeypatch.setattr(mod.mujoco, "mjtState", types.SimpleNamespace(mjSTATE_FULLPHYSICS=0))
1120+
monkeypatch.setattr(mod.mujoco.viewer, "launch_passive", lambda *args, **kwargs: FakeViewer())
1121+
1122+
args = types.SimpleNamespace(
1123+
task="MyTask",
1124+
load_run="-1",
1125+
checkpoint=None,
1126+
action_mode="policy",
1127+
policy_obs_mode="flat",
1128+
algo_log_name="custom_ppo",
1129+
show_target_bodies=False,
1130+
show_reward_debug=False,
1131+
target_body_names="",
1132+
target_max_bodies=0,
1133+
target_marker_radius=0.02,
1134+
target_axis_length=0.08,
1135+
target_marker_alpha=0.75,
1136+
target_show_axes=False,
1137+
reward_debug_show_velocity=False,
1138+
reward_debug_lin_vel_scale=0.08,
1139+
reward_debug_ang_vel_scale=0.05,
1140+
reward_debug_show_connectors=False,
1141+
reward_debug_show_global_anchor=False,
1142+
)
1143+
1144+
mod.play_interactive(args)
1145+
1146+
assert captured["ckpt"] == "/tmp/model_10.pt"
1147+
assert captured["log_dir"] == "/tmp/custom_ppo/MyTask/play_temp"
1148+
1149+
1150+
def test_play_interactive_import_does_not_swallow_registry_bootstrap_errors(
1151+
monkeypatch: pytest.MonkeyPatch,
1152+
):
1153+
import types
1154+
1155+
play_interactive_path = _SCRIPTS_DIR / "play_interactive.py"
1156+
training_mod = cast(Any, types.ModuleType("unilab.training"))
1157+
1158+
def _fail_bootstrap() -> None:
1159+
raise RuntimeError("bootstrap failed")
1160+
1161+
training_mod.ensure_registries = _fail_bootstrap
1162+
training_mod.get_entrypoint_log_root = lambda *args, **kwargs: Path("/tmp")
1163+
training_mod.resolve_task_checkpoint_path = lambda *args, **kwargs: (None, None)
1164+
monkeypatch.setitem(sys.modules, "unilab.training", training_mod)
1165+
1166+
mujoco_mod = cast(Any, types.ModuleType("mujoco"))
1167+
mujoco_mod.viewer = cast(Any, types.ModuleType("mujoco.viewer"))
1168+
monkeypatch.setitem(sys.modules, "mujoco", mujoco_mod)
1169+
monkeypatch.setitem(sys.modules, "mujoco.viewer", mujoco_mod.viewer)
1170+
1171+
spec = importlib.util.spec_from_file_location(
1172+
"play_interactive_test_module", play_interactive_path
1173+
)
1174+
mod = importlib.util.module_from_spec(spec) # type: ignore[arg-type]
1175+
1176+
with pytest.raises(RuntimeError, match="bootstrap failed"):
1177+
spec.loader.exec_module(mod) # type: ignore[union-attr]
1178+
1179+
10541180
def test_gen_grasp_import_does_not_swallow_registry_bootstrap_errors(
10551181
monkeypatch: pytest.MonkeyPatch,
10561182
):

0 commit comments

Comments
 (0)