Adds CI tests for template generator#5885
Conversation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Adds CI tests for template generator
Summary
Solid addition of CI test coverage for the template generator workflow. The tests are well-structured, use appropriate pytest fixtures (tmp_path, monkeypatch), and validate the critical contract between the generator and the RL training scripts (registry entry points). The parametrized design gives good coverage of both internal/external paths and single/multi-agent workflows with minimal code duplication.
Findings
1. [Suggestion] Module-level sys.path mutation persists across the entire test session
File: source/isaaclab_rl/test/test_template_generator.py (line 16)
sys.path.insert(0, str(TEMPLATE_TOOL_DIR))This permanently prepends tools/template/ to sys.path for the entire pytest process. If other test files in the same session happen to have modules with overlapping names (e.g., a common module), the wrong one could get imported. Consider scoping this via a conftest.py fixture or using monkeypatch.syspath_prepend in a session-scoped fixture.
2. [Suggestion] Missing negative test for get_algorithms_per_rl_library(single_agent=False, multi_agent=False)
The source code asserts that at least one of the flags must be True:
assert single_agent or multi_agent, "At least one of 'single_agent' or 'multi_agent' must be True"A quick pytest.raises(AssertionError) test would confirm this validation is exercised and protect against accidental removal of the guard.
3. [Suggestion] No test coverage for manager-based + multi-agent workflow combination
The tests cover:
manager-based+single-agent✓direct+single-agent✓direct+multi-agent✓
But manager-based + multi-agent is not tested. If the generator supports this combination (or should reject it), it would be valuable to have a test that documents that behavior.
4. [Nit] _unregister called before _load_registration_module is redundant on first iteration
_unregister(task_id)
_load_registration_module(task_dir, module_name)The first _unregister call is defensive (the ID shouldn't exist yet), which is fine for safety. However, a brief inline comment noting it's a precaution would improve readability for future maintainers.
5. [Note] CI checks are still pending
All substantive CI checks (including isaaclab_rl) are in pending state. Confirm these pass before merging — particularly that the new test file is actually discovered and executed by the isaaclab_rl test runner.
Verdict
Looks good overall. The test coverage meaningfully validates the generator's contract with downstream training scripts. The suggestions above are incremental improvements — none are blockers.
Update (commit 0ff0908): New commits are unrelated to the template generator tests (docs updates for JAX/DGX Spark, contributor addition, demo script fixes). Previous suggestions remain as-is — none were addressed in this push, but none are blockers. No new issues introduced by the incremental changes.
Greptile SummaryThis PR adds a new pytest test file that exercises the
Confidence Score: 4/5Safe to merge; the change adds test coverage only and does not modify any production code path. The new test file is well-structured and covers the most important generator behaviors. The three observations are all about test hygiene: a module-level sys.path mutation that persists across the session, bare Python asserts in a helper function, and a gym-registry entry that can leak if assertions fail mid-loop. None of these cause functional failures in normal single-pass CI runs. source/isaaclab_rl/test/test_template_generator.py — the sys.path mutation and registry cleanup patterns are worth a second look before this test file grows. Important Files Changed
Sequence DiagramsequenceDiagram
participant T as Test
participant G as generator.generate()
participant FS as Filesystem (tmp_path)
participant IL as importlib
participant GR as gym.registry
T->>G: generate(specification)
G->>FS: write __init__.py, agent cfgs, env files
G-->>T: return
T->>GR: _unregister(task_id)
T->>IL: _load_registration_module(task_dir, module_name)
IL->>FS: spec_from_file_location(__init__.py)
IL->>GR: exec_module → gym.register(task_id, ...)
IL->>IL: finally: sys.modules.pop(module_name)
IL-->>T: return
T->>GR: gym.spec(task_id)
GR-->>T: EnvSpec
T->>T: assert entry_point, kwargs keys
T->>GR: _unregister(task_id)
Reviews (1): Last reviewed commit: "Adds CI tests for template generator" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| ROOT_DIR = Path(__file__).resolve().parents[3] | ||
| TEMPLATE_TOOL_DIR = ROOT_DIR / "tools" / "template" | ||
| sys.path.insert(0, str(TEMPLATE_TOOL_DIR)) |
There was a problem hiding this comment.
sys.path mutated at module level
sys.path.insert(0, str(TEMPLATE_TOOL_DIR)) runs once when pytest collects this file and is never reverted, so tools/template stays on sys.path for the entire session. Any other test file collected afterward that happens to import generator or import common would silently resolve to the template tool's modules instead of its own. Moving this side-effect into a session-scoped autouse fixture (or a conftest.py entry) would scope the mutation to just this module's tests.
| def _load_registration_module(task_dir: Path, module_name: str) -> None: | ||
| """Execute a generated task registration module without importing its parent package.""" | ||
| spec = importlib.util.spec_from_file_location( | ||
| module_name, task_dir / "__init__.py", submodule_search_locations=[str(task_dir)] | ||
| ) | ||
| assert spec is not None | ||
| assert spec.loader is not None | ||
|
|
||
| module = importlib.util.module_from_spec(spec) | ||
| sys.modules[module_name] = module | ||
| try: | ||
| spec.loader.exec_module(module) | ||
| finally: | ||
| for name in list(sys.modules): | ||
| if name == module_name or name.startswith(f"{module_name}."): | ||
| sys.modules.pop(name, None) |
There was a problem hiding this comment.
_load_registration_module uses bare assert for preconditions
assert spec is not None and assert spec.loader is not None are standard Python asserts, which can be disabled with python -O and are not rewritten by pytest's assertion introspection. If either condition fails, the error message will be a plain AssertionError with no contextual detail. Raising a descriptive RuntimeError or ValueError here would make test-setup failures easier to diagnose in CI logs.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| for workflow_name in ["manager-based", "direct"]: | ||
| workflow_type = "single-agent" | ||
| task_id = _task_id(project_name, workflow_name, workflow_type, external) | ||
| task_dir = _task_dir(root_dir, project_name, workflow_name, workflow_type, external) | ||
| module_name = f"_template_test_{project_name}_{workflow_name.replace('-', '_')}" | ||
| _unregister(task_id) | ||
| _load_registration_module(task_dir, module_name) | ||
|
|
||
| spec = gym.spec(task_id) | ||
| task_folder = _task_folder(project_name, workflow_type) | ||
| task_class = _task_class(project_name, workflow_type) | ||
| agents_module = f"{module_name}.agents" | ||
|
|
||
| if workflow_name == "direct": | ||
| assert spec.entry_point == f"{module_name}.{task_folder}_env:{task_class}Env" | ||
| else: | ||
| assert spec.entry_point == "isaaclab.envs:ManagerBasedRLEnv" | ||
|
|
||
| assert spec.kwargs["env_cfg_entry_point"] == f"{module_name}.{task_folder}_env_cfg:{task_class}EnvCfg" | ||
| assert spec.kwargs["rl_games_cfg_entry_point"] == f"{agents_module}:rl_games_ppo_cfg.yaml" | ||
| assert spec.kwargs["rsl_rl_cfg_entry_point"] == f"{agents_module}.rsl_rl_ppo_cfg:PPORunnerCfg" | ||
| assert spec.kwargs["skrl_amp_cfg_entry_point"] == f"{agents_module}:skrl_amp_cfg.yaml" | ||
| assert spec.kwargs["skrl_cfg_entry_point"] == f"{agents_module}:skrl_ppo_cfg.yaml" | ||
| assert spec.kwargs["sb3_cfg_entry_point"] == f"{agents_module}:sb3_ppo_cfg.yaml" | ||
| assert "skrl_ppo_cfg_entry_point" not in spec.kwargs | ||
|
|
||
| _unregister(task_id) |
There was a problem hiding this comment.
The loop unregisters task_id at the start of each iteration and re-registers it, but the corresponding cleanup _unregister(task_id) at line 186 is only reached if all assertions in that iteration pass. If, say, the manager-based assertions pass but the direct assertions fail, the direct task ID is left registered for the rest of the session. Because each parametrized invocation produces unique task IDs this doesn't cause test-to-test interference today, but it's fragile. A pytest.fixture with yield + _unregister, or a try/finally block around the assertion block, would guarantee cleanup regardless of failure.
1 participant
Description
Adds CI tests for the template generator workflow
Coverage includes:
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there