Restore RL entrypoint backend state#6631
Conversation
Scope Torch and SKRL global backend settings to reusable in-process training and playback calls. Preserve attributes that were initially absent and add failure-path regressions for RSL-RL and SKRL.
| def main(): | ||
| """Play with skrl agent.""" | ||
| """Play with SKRL while restoring the caller's global settings.""" | ||
| with preserve_attribute(skrl.config.jax, "backend"): | ||
| _main() |
There was a problem hiding this comment.
Unconditional JAX preservation inconsistent with
train_skrl
main() always enters preserve_attribute(skrl.config.jax, "backend") regardless of args_cli.ml_framework, whereas train_skrl.run() only enters the context when ml_framework.startswith("jax"). For torch-framework play calls the attribute is never mutated inside _main(), so this save/restore cycle is a no-op — harmless, but the inconsistency can mislead readers into thinking the JAX attribute is always mutated. Since args_cli is accessible at module scope, the guard could mirror the training side: if args_cli.ml_framework.startswith("jax"): ....
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!
| def test_skrl_play_main_restores_jax_backend(monkeypatch) -> None: | ||
| """Direct SKRL play calls remove the JAX backend setting they created.""" | ||
| monkeypatch.setattr(sys, "argv", ["play_skrl.py"]) | ||
| namespace = runpy.run_module("isaaclab_rl.entrypoints.backends.play_skrl", run_name="test_play_skrl") | ||
| skrl = namespace["skrl"] | ||
| monkeypatch.delattr(skrl.config.jax, "backend", raising=False) | ||
|
|
||
| def fail_after_mutation() -> None: | ||
| skrl.config.jax.backend = "mutated" | ||
| raise RuntimeError("failed") | ||
|
|
||
| namespace["main"].__globals__["_main"] = fail_after_mutation | ||
| with pytest.raises(RuntimeError, match="failed"): | ||
| namespace["main"]() | ||
|
|
There was a problem hiding this comment.
Module-level side effects executed by
runpy.run_module
runpy.run_module("isaaclab_rl.entrypoints.backends.play_skrl", ...) re-runs the script's entire top-level body, including the skrl version check at lines 96–101 of play_skrl.py that calls exit() on failure. If skrl is not installed at the required version in the test environment the test bails out with a SystemExit before any assertion is reached, producing a misleading failure. The other SKRL test (test_skrl_training_restores_jax_backend) avoids this by patching _parse_args and _run on the already-imported module rather than re-executing it. A similar patch-based approach — importing play_skrl once and patching its _main attribute — would remove this dependency.
| def test_rejected_rsl_training_preserves_torch_backend_state(monkeypatch) -> None: | ||
| """A rejected in-process RSL-RL request does not mutate its caller.""" | ||
| import torch | ||
|
|
||
| caller_state = (False, False, True, True) | ||
| settings = ( | ||
| (torch.backends.cuda.matmul, "allow_tf32"), | ||
| (torch.backends.cudnn, "allow_tf32"), | ||
| (torch.backends.cudnn, "deterministic"), | ||
| (torch.backends.cudnn, "benchmark"), | ||
| ) | ||
| for (target, name), value in zip(settings, caller_state): | ||
| monkeypatch.setattr(target, name, value) | ||
|
|
||
| with pytest.raises(SystemExit): | ||
| dispatch._run_backend("isaaclab_rl.entrypoints.backends.train_rsl_rl", ["--help"], run_as_script=False) | ||
|
|
||
| assert _torch_backend_state() == caller_state |
There was a problem hiding this comment.
Test exercises the pre-mutation exit path only
["--help"] causes argparse to raise SystemExit inside _parse_args, which is called before scoped_torch_backend_flags is entered. The torch flags are therefore never written, so the assertion trivially passes without exercising the context-manager restore path. A complement that actually enters the context — e.g. by patching _run to raise immediately after the flags have been set — would give stronger confidence that the finally branch in preserve_attribute fires correctly for a mid-training failure. (The unit-level test_scoped_backend_state_restores_values_after_exception covers this for the primitives, but not for train_rsl_rl.run() end-to-end.)
| @@ -0,0 +1,46 @@ | |||
| # Copyright (c) 2022-2026, The Isaac Lab Project Developers (https://github.com/isaac-sim/IsaacLab/blob/main/CONTRIBUTORS.md). | |||
There was a problem hiding this comment.
This type of file is not good practice. Let's just use common.py
| cuda_matmul = torch_module.backends.cuda.matmul | ||
| cudnn = torch_module.backends.cudnn | ||
| settings = ( | ||
| (cuda_matmul, "allow_tf32", True), |
There was a problem hiding this comment.
Why not just allow all these to be args, then this function can set them, and the function can be used by rsl_rl training.
That way no hardcoded values?
Move shared state guards into the common entrypoint utilities and make RSL-RL's Torch backend policy explicit at the call site. Scope SKRL playback preservation to JAX calls and cover exceptional RSL-RL cleanup.
Description
The reusable reinforcement learning entrypoints introduced in #6553 restore
sys.argvafter in-process calls, but backend-specific global settings can still leak into the caller.This PR scopes those mutations to each invocation:
skrl.config.jax.backend, including deleting it when the attribute did not exist before the call.main(), covering both unified dispatch and direct imported calls.Relationship
Type of change
Validation
./isaaclab.sh -p -m pytest source/isaaclab_rl/test/test_entrypoints.py -q(11 passed)./isaaclab.sh -p tools/changelog/cli.py check develop./isaaclab.sh -fThe full
source/isaaclab_rl/testdirectory was also attempted locally but did not complete or emit results within the five-minute command timeout.Checklist
CONTRIBUTORS.md