added failure if users try to determine export method for direct rl e… - #7081
added failure if users try to determine export method for direct rl e…#7081frlai wants to merge 2 commits into
Conversation
…nvironments with export_with
There was a problem hiding this comment.
Isaac Lab Review Bot
The direct-environment validation is implemented across all LEAPP exporters while preserving the manager-based default. Before merge, correct SB3’s misleading direct-environment error and either remove or document the newly accepted pt2 export method.
- Design and architecture: The backend-specific validation is consistent, but SB3 is architecturally different because it does not support direct-environment export at all. Its existing unsupported-environment path should remain authoritative rather than suggesting a direct-export configuration.
- API: Changing the parser default to
Noneis compensated by an explicitonnx-dynamofallback for manager-based environments. However, addingpt2expands the public CLI choice set without corresponding help or changelog coverage and is outside the stated validation change. - Implementation: The rl_games, rsl_rl, and skrl paths now reject an explicitly supplied export method for direct environments as intended. In SB3, the new check masks the existing
NotImplementedErrorand gives unusable advice because that backend cannot export direct environments.
Minor fixes needed. Posted 2 actionable findings inline.
Automated review; human maintainers own approval decisions.
| try: | ||
| env = gym.make(args_cli.task, cfg=env_cfg, render_mode=None) | ||
| if not isinstance(env.unwrapped, ManagerBasedRLEnv): | ||
| if args_cli.export_method is not None: |
There was a problem hiding this comment.
🟡 Warning · Implementation — SB3 direct-env error masks unsupported-environment message
SB3 LEAPP export does not support direct environments at all (the following NotImplementedError). Checking --export_method first replaces that accurate message with advice to "set export_with directly in the annotate.output_tensors() call", which cannot be followed here and implies direct export works for SB3. Raise the NotImplementedError first, or drop this extra check in the SB3 backend.
| choices=["onnx-dynamo", "onnx-torchscript", "jit-script", "jit-trace"], | ||
| help="Method to export the policy", | ||
| default=None, | ||
| choices=["onnx-dynamo", "onnx-torchscript", "jit-script", "jit-trace", "pt2"], |
There was a problem hiding this comment.
🔵 Suggestion · Api — Unrelated pt2 choice added without documentation
Adding "pt2" widens the accepted values of the public --export_method flag and is forwarded verbatim to patch_env_for_export, but it is unrelated to this change's stated purpose and appears in neither the help text nor the changelog fragment. Either drop it from this change or document it in the help string and changelog alongside the supporting backend support.
Greptile SummaryThe PR makes LEAPP export-method selection explicit while preserving
Confidence Score: 5/5The PR appears safe to merge, with no concrete blocking or independently actionable non-blocking issue established in the changed paths. The four exporters consistently distinguish omitted export-method selection from explicit input, retain the prior manager-based default, and reject unsupported direct-environment input without disrupting the existing direct export path. Important Files Changed
Reviews (1): Last reviewed commit: "added failure if users try to determine ..." | Re-trigger Greptile |
Description
export_method is a argument for the export script that allows users to select the format (onnx, pt, etc.). This argument is only valid for managed environments. direct rl environments would ignore this silently. This new pr makes it fail loudly if someone supplies the export_method in cli while specifying a direct deployment environment.
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there