Ashwinvk/leapp observation term inputs

[ashwinvkNV]

Description

Adds LEAPP observation-term input boundaries so selected policy observations can be exported as runtime inputs using observation:{group}:{term} metadata. This supports workflows like gear assembly where deployment provides final shaft pose observations directly, instead of exposing simulator-only bookkeeping.

Also updates LEAPP deployment env support, marks gear shaft pose observations, adds docs, and adds focused unit tests.

Fixes # N/A

Type of change

• New feature (non-breaking change which adds functionality)
• Documentation update

Screenshots

N/A

Checklist

• I have read and understood the contribution guidelines
• I have run the pre-commit checks with ./isaaclab.sh --format
• I have made corresponding changes to the documentation
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• I have updated the changelog and corresponding version, if required
• I have added my name to CONTRIBUTORS.md or my name already exists there

[greptile-apps]

Greptile Summary

This PR introduces leapp_observation_input, a new decorator that marks observation terms as explicit LEAPP input boundaries (observation:{group}:{term}), and wires up the corresponding export and deployment infrastructure so selected policy observations can be provided directly by a deployment runtime instead of being traced through simulator state.

• New leapp_observation_input decorator & resolver (leapp_semantics.py, utils.py, __init__.pyi): clean, consistent with existing leapp_tensor_semantics patterns; gear assembly observation classes are annotated as a concrete example.
• Export patcher (export_annotator.py): wraps marked terms so the computed+post-processed value is registered as the LEAPP input tensor, but the post-processing closure captures term_cfg by reference and the same object is immediately mutated to None — so modifiers, clip, and scale are silently dropped from the exported graph.
• Deployment env (leapp_deployment_env.py): conditionally builds an ObservationManager when the LEAPP YAML declares observation:* inputs and computes terms on each step; logic is sound but lacks a length guard on the connection-string split.
• RSL-RL export helpers (rsl_rl/export.py): parameter rename from policy to policy_nn is incomplete — one call site on line 227 still uses the old name, causing NameError for recurrent policies with an uninitialized hidden state.

Confidence Score: 3/5

Two concrete defects need to be fixed before merging: one causes a NameError crash in the RSL-RL recurrent-state export path, and one silently omits observation post-processing from the exported LEAPP graph.

The leapp_observation_input decorator, deployment-env integration, and documentation are well-structured. However, the parameter rename in rsl_rl/export.py left a stale policy reference on line 227 that will raise NameError whenever ensure_actor_hidden_state_initialized is called on a recurrent policy whose state is not yet set — the common first-call case. Separately, _wrap_observation_input captures term_cfg by reference, and the caller immediately nulls term_cfg.modifiers/clip/scale on the same object, so post-processing is never applied to observation-input terms during export, contradicting the documented behaviour. Both issues affect the primary new code paths.

scripts/reinforcement_learning/leapp/rsl_rl/export.py (stale policy reference in ensure_actor_hidden_state_initialized) and source/isaaclab/isaaclab/utils/leapp/export_annotator.py (term_cfg mutation after closure creation in _patch_observation_manager) require attention before merging.

Important Files Changed

Filename	Overview
scripts/reinforcement_learning/leapp/rsl_rl/export.py	Renames helper function parameters from policy to policy_nn for clarity, but line 227 (memory = get_actor_memory_module(policy)) was not updated, leaving a stale reference that causes NameError for any recurrent policy with an uninitialized hidden state.
source/isaaclab/isaaclab/utils/leapp/export_annotator.py	Adds _wrap_observation_input and _apply_observation_post_processing to support observation-term LEAPP input boundaries; however, post-processing (modifiers, clip, scale) is silently dropped because term_cfg fields are nulled out on the shared object after the closure is created.
source/isaaclab/isaaclab/envs/leapp_deployment_env.py	Adds ObservationInputSpec and ObservationManager support to the deployment env; _compute_observation_input correctly applies post-processing at deployment time. Minor: no length guard on observation: connection strings and direct use of private _group_obs_term_names/_group_obs_term_cfgs attributes.
source/isaaclab/isaaclab/utils/leapp/leapp_semantics.py	Adds leapp_observation_input decorator and resolve_leapp_observation_input_semantics helper; implementation is clean and correctly handles both function-based and class-based observation terms.
source/isaaclab/isaaclab/utils/leapp/utils.py	Adds build_observation_connection following the same pattern as the existing build_command_connection; straightforward and correct.
source/isaaclab/test/utils/test_leapp_observation_input.py	New unit tests cover decorator attachment and element-name resolution correctly; the wrapper test does not reproduce the production-time mutation of term_cfg that causes post-processing to be skipped, so the P1 export bug goes undetected.
source/isaaclab_rl/test/export/test_rsl_rl_export_recurrent_state.py	New regression test for RSL-RL 5.x recurrent state helpers; the test exercises the path where actor_state starts as None, which is exactly the path that hits the stale policy NameError in ensure_actor_hidden_state_initialized.
source/isaaclab_tasks/isaaclab_tasks/contrib/deploy/mdp/observations.py	Annotates gear_shaft_pos_w and gear_shaft_quat_w with leapp_observation_input; element name choices (XYZ_ELEMENT_NAMES, QUAT_XYZW_ELEMENT_NAMES) look correct for the respective position/orientation semantics.
source/isaaclab/isaaclab/utils/leapp/init.pyi	Stub updated to export build_observation_connection, leapp_observation_input, and resolve_leapp_observation_input_semantics; consistent with the implementation changes.
docs/source/policy_deployment/05_leapp/exporting_policies_with_leapp.rst	Adds documentation for observation-term input boundaries with correct usage examples; also removes Windows-specific tab-set and updates hyperlink style from __ to _.

Sequence Diagram

sequenceDiagram
    participant OA as ExportPatcher
    participant OM as ObservationManager
    participant WF as wrapped()
    participant AN as annotate

    Note over OA: Export time (_patch_observation_manager)
    OA->>OA: resolve_leapp_observation_input_semantics(term_cfg.func)
    OA->>OA: _wrap_observation_input(original_func, real_env, term_cfg)
    Note right of OA: term_cfg captured by reference in wrapped()
    OA->>OA: "term_cfg.modifiers = None"
    OA->>OA: "term_cfg.clip = None"
    OA->>OA: "term_cfg.scale = None"

    Note over OM,AN: Validation pass (obs_manager.compute called)
    OM->>WF: "term_cfg.func(proxy_env, **params)"
    WF->>WF: "original_func(real_env, **kwargs)"
    WF->>WF: _apply_observation_post_processing(result, term_cfg)
    Note right of WF: term_cfg.modifiers/clip/scale are None - no-op
    WF->>AN: annotate.input_tensors(task_name, sem)
    AN-->>OM: annotated tensor

    Note over OA: Deployment time (_compute_observation_input)
    OA->>OM: observation_manager._group_obs_term_cfgs[group][idx]
    OA->>OA: "term_cfg.func(self, **params)"
    OA->>OA: apply modifiers / clip / scale
    OA-->>OA: return obs tensor

Loading

Comments Outside Diff (1)

1. scripts/reinforcement_learning/leapp/rsl_rl/export.py, line 227 (link)

   Stale policy reference causes NameError

   The function parameter was renamed from policy to policy_nn, but this line (outside the diff hunks) was not updated. For any recurrent policy whose hidden state is None at initialization time, the function reaches line 227 and raises NameError: name 'policy' is not defined, because policy no longer exists in any enclosing scope. The unit test in test_rsl_rl_export_recurrent_state.py exercises exactly this path (hidden_state starts as None) and should reproduce the failure.

_{Reviews (1): Last reviewed commit: "Add LEAPP observation input export docs ..." | Re-trigger Greptile}

[greptile-apps]

Post-processing silently dropped during export

_wrap_observation_input receives term_cfg by reference and the wrapped closure captures it. Immediately after the call, lines 308–310 null out term_cfg.modifiers, term_cfg.clip, and term_cfg.scale on the same object. When wrapped later calls _apply_observation_post_processing(result, term_cfg), all three fields are already None, so modifiers, clipping, and scaling are never applied. This contradicts the documented guarantee that "deterministic post-processing such as modifiers, clipping, and scaling" is included in the LEAPP input boundary. The unit test in test_leapp_observation_input_wrapper_annotates_processed_term does not catch this because it calls _wrap_observation_input directly without the post-mutation performed here.

[greptile-apps]

IndexError on malformed observation: connection strings

parts[1] and parts[2] are accessed without a length guard. A YAML connection value of "observation:policy" (only two colon-separated segments) would raise IndexError at this line. The state: and write: branches above share the same pattern, but adding an explicit len(parts) != 3 check here with a descriptive ValueError would make misconfigured YAMLs much easier to diagnose.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot — PR #6081

Summary: This PR adds LEAPP observation-term input boundaries, allowing selected policy observations to be exported as runtime inputs using observation:{group}:{term} metadata. The design is clean and well-integrated with the existing LEAPP framework.

CI Status: pre-commit and changelog fragments checks are failing. These should be addressed before merge.

---

Findings

🟡 Warning — No guard for malformed observation: connection strings (leapp_deployment_env.py)

The parsing group_name, term_name = parts[1], parts[2] will raise an unguarded IndexError if the YAML contains a malformed connection like "observation:foo" (missing term component). Consider adding a parts-length check with a descriptive error message.

🟡 Warning — _compute_observation_input accesses private _group_obs_term_names/_group_obs_term_cfgs (leapp_deployment_env.py)

The deployment env reaches into observation_manager._group_obs_term_names and ._group_obs_term_cfgs (private attributes). If the ObservationManager internal layout changes, this will break silently. The validation uses the public .active_terms API but the actual computation bypasses it. Consider whether a targeted public method on ObservationManager (e.g. compute_term(group, name)) would be more maintainable.

🔵 Suggestion — _compute_observation_input duplicates post-processing logic (leapp_deployment_env.py)

The modifier/clip/scale post-processing in _compute_observation_input is identical to ExportPatcher._apply_observation_post_processing. Consider extracting a shared utility to avoid divergence if post-processing logic evolves (e.g., new transform types are added).

🔵 Suggestion — Document that leapp_observation_input disables noise at export (export_annotator.py)

When observation_input_semantics is detected, term_cfg.modifiers, term_cfg.clip, and term_cfg.scale are set to None on the term config—but the wrapper applies them internally before registering the LEAPP input. This is correct but subtle. A brief inline comment noting that post-processing moves inside the wrapper (rather than being dropped) would help future readers.

🔵 Suggestion — Test coverage for LeappDeploymentEnv observation input path

The unit tests thoroughly cover the export/annotation side (test_leapp_observation_input.py), but there's no test for _compute_observation_input in LeappDeploymentEnv. A test verifying that the deployment env correctly computes observation inputs from a mock YAML with observation: connections would catch regressions in the deployment path.

---

Verdict: Well-designed feature that fills a real gap for deployment workflows. The architecture cleanly separates export-time annotation from deployment-time computation. Main concerns are the fragile private-attribute access in the deployment env and the lack of input validation on malformed connection strings. Overall quality is high—good docs, good tests on the export side, and proper error messages for misconfiguration.

[isaaclab-review-bot]

🟡 Warning: If a LEAPP YAML contains a malformed connection like "observation:foo" (missing the term component), parts[2] raises an unguarded IndexError. Consider adding validation:

if len(parts) < 3:
    raise ValueError(
        f"Malformed observation connection '{connection}' for input '{key}': "
        "expected format 'observation:<group>:<term>'."
    )

[isaaclab-review-bot]

🟡 Warning: This accesses private attributes _group_obs_term_names and _group_obs_term_cfgs. The validation above uses the public .active_terms API, but the actual computation bypasses it. If ObservationManager internals change, this will break without a clear error. Consider whether a targeted public method (e.g., compute_term(group, name, noise=False)) would be more maintainable long-term.