Add EnvGraphSpec to ArenaEnv method

[xyao-nv]

Summary

Convert EnvGraph to ArenaEnv

Detailed description

• Adds ArenaEnvGraphSpec.to_arena_env() so YAML graph specs can be converted into IsaacLabArenaEnvironment.
• Resolves task classes at runtime and converts multi-task specs into a sequential composite task using all task entries.
• Includes nodes, state, task validation in to_yaml().

[isaaclab-review-bot]

Code Review

Thanks for adding the to_arena_env() method and the comprehensive conversion utilities! This is a substantial addition that bridges the gap between declarative graph specs and runtime environment instances. Below are my observations:

✅ Strengths

1. Clean API Design: The to_arena_env() method on ArenaEnvGraphSpec provides a natural, discoverable entry point. The TYPE_CHECKING guard for the import keeps the module dependency graph clean.

2. Extensible Task Resolution: The task factories mechanism allows custom task types while still supporting class name and module discovery fallbacks. The resolution order (import path → class name → module stem) is intuitive.

3. Well-Structured Utilities: The conversion logic is broken into focused helper functions (_select_state_spec, _instantiate_node_assets, _apply_state_spatial_constraints, etc.), making the code readable and testable.

4. Good Test Coverage: The new test case exercises sequential task composition and validates the full conversion pipeline through a realistic YAML fixture.

---

🔍 Suggestions & Questions

1. Consider documenting the task_args aliasing behavior
The _align_task_kwargs function does clever fuzzy matching of YAML keys to constructor parameters (lines 357-390). While useful for YAML ergonomics, this implicit behavior could surprise users when debugging. Consider either:

• Adding a note in docstrings about which aliases are supported, or
• Logging/warning when a fuzzy match is used

2. _discover_task_classes does package-wide scanning
If the direct module candidates fail, the fallback scans all modules under isaaclab_arena.tasks. This could become slow as the task library grows. Is caching viable here, or is this path rarely hit in practice?

3. Potential edge case in _match_task_arg_to_parameter
The substring matching (normalized_key in _normalize_identifier(name)) could yield unexpected results for short keys. For example, a task arg "x" would match any parameter containing the letter "x". The narrowing to required params helps, but it might be worth documenting or tightening this.

4. The IN spatial constraint raises NotImplementedError
Noticed that ArenaEnvGraphSpatialConstraintType.IN is defined but raises at conversion time. The TODO comment references May 21—is there a tracking issue for this, or should one be created?

5. Minor import ordering
In arena_env_graph_spec.py, the diff shows yaml import moved below from collections.abc which deviates from the typical stdlib-first ordering in this codebase. Not blocking, just a style nit.

---

🧪 Testing

The new test test_arena_env_graph_conversion_builds_sequential_pick_and_place_task is well-structured. One suggestion: consider adding a negative test for the IN constraint NotImplementedError to lock in the expected behavior until solver support lands.

---

Overall, this is a solid PR that adds valuable functionality. The suggestions above are minor refinements. Happy to discuss any of these points!

---

Update (53a0a44)

Thanks for the follow-up commit! This update addresses several important areas:

✅ What's Improved

1. API Simplification: Removed the task_factories parameter from to_arena_env() and build_arena_env_from_graph_spec(). This simplifies the public API—users now rely on task class discovery rather than custom factories.

2. Validation Before Instantiation: Added comprehensive pre-conversion validation via _validate_graph_spec_for_conversion():

   • _validate_spatial_constraint_shapes() ensures proper child node arity for each constraint type
   • _validate_task_arg_node_references() catches dangling node references in task args early
   • _validate_initial_state_supported_for_conversion() prevents unsupported IN constraints from reaching the runtime

3. Expanded Test Coverage: Three new validation tests added:

   • test_arena_env_graph_conversion_validates_references_before_instantiation
   • test_arena_env_graph_conversion_validates_relationship_shape_before_instantiation
   • test_arena_env_graph_conversion_validates_task_arg_node_references_before_instantiation

4. Code Consolidation:

   • Unified _normalize_task_class_name and _normalize_task_module_name into single _normalize_task_name()
   • Cleaned up the test data by consolidating two_pick_and_place_maple_table_env_graph.yaml into the existing test fixture
   • Tightened error messages for better debugging

5. TYPE_CHECKING Guards: Expanded use of TYPE_CHECKING imports in the conversion utils to keep the module lean at runtime.

🔍 Minor Observations

• The validation now catches my earlier suggestion #4 (the IN constraint issue) with a clear assertion before hitting the runtime NotImplementedError.
• Good use of early-exit validation pattern—fail fast with descriptive errors rather than cryptic runtime failures during asset instantiation.

🧪 Test Fixture Updates

The pick_and_place_maple_table_env_graph.yaml now includes the second task and third state spec inline, which makes the sequential task test self-contained. The removed two_pick_and_place_maple_table_env_graph.yaml was redundant.

---

No blocking concerns. This is a nice cleanup that improves both usability and debuggability. 👍

---

Update (f28226a): Reviewed 4 new commits (03410c8 → f28226a).

Changes Since Last Review

These commits are a significant cleanup and simplification pass:

1. API Simplification: Removed state_spec_id parameter from to_arena_env() — now always uses the first state spec. This is cleaner but removes flexibility; the TODO notes a future aggregation approach.

2. Refactored is_unary() in Relations: Moved is_unary() from RelationBase to individual subclasses (UnaryRelation, IsAnchor, RandomAroundSolution, RotateAroundSolution). ⚠️ Note: If external code subclasses RelationBase directly without implementing is_unary(), it will now fail with AttributeError. Since these relations are likely internal, this is probably fine, but worth a note in release/migration docs if public.

3. Removed Helper Indirection: _select_state_spec(), _select_embodiment(), and _relation_params_for_constraint() are gone — logic is inlined or moved upstream. The upstream contract now expects constraint.params to arrive in constructor-ready form (no more Side enum conversion or position_xyz unpacking).

4. Hoisted Imports: Several imports moved from inside functions to module-level. Trades laziness for clarity; should be fine given these modules are already in use when conversion runs.

5. Extensive Docstring Improvements: arena_env_graph_task_conversion_utils.py and graph_spec_utils.py now have detailed docstrings with examples. Nice for discoverability.

6. Test Adjustment: Removed test_arena_env_graph_conversion_validates_selected_initial_state_before_instantiation since the state_spec_id param no longer exists.

Assessment

No new bugs spotted. The simplification is reasonable — removing state_spec_id reduces API surface while the TODO signals the intended future direction. The is_unary() change is the most notable from a compatibility standpoint but appears safe for internal usage.

✅ New commits look good.

---

Update (4d10198): Reviewed incremental changes.

Changes

• Inlined helper functions: Removed _child_asset(), _add_relation(), _set_initial_pose() — logic now directly calls asset methods. Cleaner code, relies on upstream validation to catch issues early.
• Single-pass OBJECT_REFERENCE instantiation: Consolidated into main loop with documented upstream contract (nodes must be ordered parent-first). Avoids second iteration.
• Module-level imports: Hoisted isaaclab_arena.tasks, SequentialTaskBase, TaskBase imports.
• Minor: Removed unused ArenaEnvGraphSpatialConstraintSpec import and redundant tuple() wrapping.

Assessment

Continuation of the cleanup theme. No new issues introduced. ✅

---

Update (b0ae704): Reviewed incremental changes.

Changes

This commit is a major API simplification that removes the fuzzy/loose matching behavior:

1. Removed fuzzy task_args matching — _align_task_kwargs() and related helpers are gone. Task arg keys must now exactly match constructor parameter names.

2. Removed task type loose resolution — _normalize_task_name(), _task_module_candidates(), and module-stem matching removed. Task type: in YAML must now be the exact class name (e.g., PickAndPlaceTask) or a qualified import path.

3. Removed auto-injection of instance_name — upstream must now emit instance_name explicitly in node.params when needed.

4. Removed assert_task_arg_node_references_exist() validation — node reference validation now happens at runtime rather than spec construction.

5. Test fixtures updated to use exact names (pick_up_object instead of object, PickAndPlaceTask instead of pick_and_place).

Assessment

This directly addresses my earlier suggestions (#1-#3 from the original review) by removing the complexity entirely rather than documenting it. The strict upstream contract approach is cleaner and more predictable.

⚠️ Breaking change for existing YAML files that relied on loose matching — they'll need to use exact class/parameter names.

No new issues. ✅

---

Update (ced545a): Reviewed incremental changes.

Changes

This commit is a refactoring to break circular imports by extracting types into a dedicated module:

1. New arena_env_graph_types.py module: Contains all the pure-data schema types (enums and dataclasses) that were previously in arena_env_graph_spec.py:

   • Enums: ArenaEnvGraphNodeType, ArenaEnvGraphSpatialConstraintType, ArenaEnvGraphTaskConstraintType
   • Dataclasses: ArenaEnvGraphNodeSpec, ArenaEnvGraphObjectReferenceNodeSpec, ArenaEnvGraphSpatialConstraintSpec, ArenaEnvGraphTaskConstraintSpec, ArenaEnvGraphStateSpec, ArenaEnvGraphTaskSpec

2. arena_env_graph_spec.py slimmed down: Now imports/re-exports types via __all__ for backward compatibility. The build_arena_env_from_graph_spec import is hoisted to module level (removing the lazy import in to_arena_env()).

3. Import path updates: arena_env_graph_conversion_utils.py, arena_env_graph_task_conversion_utils.py, and graph_spec_utils.py now import types from the new arena_env_graph_types module.

4. Test fixture adjustments: Node indices updated in validation tests due to node reordering (background now at index 1, object_reference at index 2). Task type updated to exact name PickAndPlaceTask.

Assessment

Clean separation of concerns:

• Types module: Pure data (no behavior, no dependencies on conversion logic)
• Spec module: Parsing, validation, and conversion entry point

This breaks the potential circular import chain (conversion_utils → spec → conversion_utils) while maintaining backward compatibility via re-exports. No new issues. ✅

[qianl-nv]

Thanks @xyao-nv for the draft. Overall looks ok, but isaaclab_arena/environments/arena_env_graph_conversion_utils.py is a long file and could potentially be clean up to make it simpler. Especially in terms of spec validation, many checks should happen at the spec construction step, not at the spec->arena env conversion step.

[qianl-nv]

should we build a map from ArenaEnvGraphSpatialConstraintType to the actual relation class (in the relation.py files), and for the relation classes to have a static method in_unary(). would be easier to keep track then keeping a map here.
this might also solve the not_supported types in _validate_initial_state_supported_for_conversion cleaner.

[xyao-nv]

agree, updated

[xyao-nv]

Changes made:

• Add is_unary() to Relation and RelationBase
• Add a spatial constraints lookup table to auto populate Relation class objects
• Move some validation related steps in graph_spec_utils.py

[xyao-nv]

self review: maybe it will change based on what subtask tracking brings

[xyao-nv]

self review: think about how to enforce those contract in upstream

[xyao-nv]

self review: I wonder if it shall be enforced to only use bare class name, as import path may be changed

[xyao-nv]

note: purely mv stuffs around, so this file only contains data, no orchestration/method.

[xyao-nv]

note: there are too many detailed fun calls, so I avoid spamming the class method.

[alexmillane]

A couple of nits, but my main comment is that we should add registries for tasks and constraints to avoid some stuff in here that makes the graph decoding not extensible out-of-source.

[alexmillane]

Shall we add a final else?

Perhaps right now the left over nodes are not handled? We could add a warning for now?

[alexmillane]

nit: suggestion to use if..else, rather than if...continue.

For some reason claude likes to do this, but I (perhaps personally) feel that if...else is a far more normal construction.

[alexmillane]

Suggestion to check this (probably in a function).

[alexmillane]

nit: move to a function so this function (assets_to_scene_and_embodiment)

[alexmillane]

Can we Any -> Asset?

[alexmillane]

prefer if...else over if...continue.

[alexmillane]

Is there a reason to not make this a module level constant? I believe that would be more normal

SPATIAL_CONSTRAINT_RELATION_CLASSES={
    ArenaEnvGraphSpatialConstraintType.IS_ANCHOR: IsAnchor,
    ...
}

[alexmillane]

Elsewhere in arena we deal with the "string to object" problem via a registry.

We have a few registries in registries.py.

If you compare your code for decoding the scene to the code here for decoding the tasks, there's a big difference in complexity. Furthermore, the code here is not extendable by a user who wants to add tasks from outside the source-tree, because we only search for tasks effectively in a single folder.

I suggest we move to a registry for tasks. Effectively this just associates a string name to each task, and gives us a nice interface for looking them up.

What do you think?

[alexmillane]

At some point we'll have to clean this up and add a relation for this 😅

Let's do that later.

[alexmillane]

One issue here is that it requires a user to maintain the relationship between constraints in the relations folder and this static struct here.

That's not ideal, although not a huge problem for us. The issue is that if someone wants to extend arena with new relations. They can do that on the relations side, but they can't do it here, because this mapping is hard coded.

I think the correction solution here is to have a constraint registry. Such a registry provides a mapping between a string name, and a constrain object. Such a registry is extensible by the user. It also eliminates the need to maintain this mapping between a name and an object - constraint declare their names.