Add order-does-not-matter composite tasks#677
Conversation
…d/order_does_not_matter_composite_task_prototype
…d/order_does_not_matter_composite_task_prototype
…d/order_does_not_matter_composite_task_prototype
…d/order_does_not_matter_composite_task_prototype
…d/order_does_not_matter_composite_task_prototype
…d/order_does_not_matter_composite_task_prototype
…d/composite_task_v03
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot — PR #677
Title: Draft: Add order-does-not-matter composite tasks
Author: @peterd-NV
Files Changed: 5 (composite_task_base.py, sequential_task_base.py, tests)
Summary
This PR introduces a CompositeTaskBase class that generalizes composite task handling where subtask completion order does not matter. The existing SequentialTaskBase is refactored to inherit from this new base class, preserving its sequential ordering semantics through method overrides.
📦 Latest Update (b0e2bb0)
Changes since last review (5ceddde → b0e2bb0):
-
Skippable None state option for
desired_subtask_success_state:- Type annotation changed from
list[bool] | Nonetolist[bool | None] | None Noneentries in the list are now treated as "don't care" wildcards- Added assertion to validate entries are
True,False, orNone - Updated success logic: positions with
Noneare skipped when checking the desired pattern - Applied consistently to both
CompositeTaskBaseandSequentialTaskBase
- Type annotation changed from
-
Bug fix: Changed
if desired_subtask_success_state:toif desired_subtask_success_state is not None:- This correctly handles empty lists and prevents falsy list edge cases
-
New tests for the None wildcard feature:
test_composite_desired_subtask_success_state_with_nonein composite task teststest_sequential_desired_subtask_success_state_with_nonein sequential task tests- Tests verify don't-care semantics, pattern matching, and edge cases (all-None)
-
Merge from main + lint fixes
📋 Previous Review Summary (5ceddde)
The previous commits introduced:
CompositeTaskBaseclass with order-independent subtask completionSequentialTaskBaserefactored to inherit from composite base- Comprehensive tests for both task types
- Type annotation fixes (
configclass→Anyreturn types) - Environment randomization config updates
✅ Assessment
The new changes are clean and well-implemented:
- Feature value: The
Nonewildcard indesired_subtask_success_stateenables flexible task definitions where only some subtask final states matter — useful for tasks with optional steps - Correctness: The truthiness fix (
is not None) is important for proper handling of empty desired states - Test coverage: New tests adequately cover the wildcard semantics for both composite and sequential tasks
- Consistency: Changes applied uniformly across both task base classes
LGTM 👍
peterd-NV
changed the title
peterd-NV
changed the title
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot — PR #677
Title: Draft: Add order-does-not-matter composite tasks
Author: @peterd-NV
Files Changed: 5 (composite_task_base.py, sequential_task_base.py, tests)
Latest Commit: 9db9e8a
Summary
This PR introduces a CompositeTaskBase class that generalizes composite task handling where subtask completion order does not matter. The existing SequentialTaskBase is refactored to inherit from this new base class, preserving its sequential ordering semantics through method overrides.
🆕 Latest Changes (cleanup commit)
Since the previous review:
- Cleanup commit (9db9e8a): Code formatting and organization improvements
The core changes from previous commits remain:
- Skippable
Nonestate option fordesired_subtask_success_state- entries can now beTrue,False, orNone(wildcard/"don't care") - Bug fix:
if desired_subtask_success_state:→if desired_subtask_success_state is not None:for correct handling of empty/falsy lists - Comprehensive tests for wildcard semantics in both composite and sequential task classes
Architecture Highlights
CompositeTaskBase (new)
- Order-independent subtask completion tracking via
_subtask_success_state - Success requires all subtasks to have succeeded at some point AND match
desired_subtask_success_state(if provided) Noneentries in desired state are wildcards — skipped during pattern matching- Shared utility methods:
add_suffix_configclass_transform,remove_configclass_transform - Metrics, events, and termination configs automatically combined from subtasks
SequentialTaskBase (refactored)
- Now inherits from
CompositeTaskBase - Overrides
composite_task_success_functo enforce sequential ordering via_current_subtask_idx - Overrides
reset_subtask_success_stateto reset the index in addition to success state - Maintains backward compatibility with existing sequential task semantics
Test Coverage
test_composite_task_base.py (new)
- Config transformation utilities (
add_suffix,remove) Nonewildcard semantics for desired subtask success state
test_composite_open_door.py (new)
- Order-independence verification (forward and reverse completion order)
- Multi-environment correctness
- Reset/termination behavior
test_sequential_task_base.py (updated)
- Sequential ordering enforcement (subtask N+1 blocked until N succeeds)
- Success state latching behavior
desired_subtask_success_statepattern matching includingNonewildcards- Reset clearing both state and index
✅ Assessment
Strengths:
- Clean refactoring that extracts common composite task logic
Nonewildcard feature enables flexible task definitions with optional steps- Good separation of concerns between base and sequential classes
- Comprehensive test coverage for both new and existing functionality
Minor observations (non-blocking):
- The
compute_metric_from_recordingmethod accessesrecorded_metric_data[0].shape[1]— this assumes all episodes have consistent shape, which should be fine but could add a defensive check - Consider adding docstrings to the new test classes (
_MockSuccessFunc,_MockSubtask,_MockEnv) for clarity
Overall this is a well-structured addition that properly generalizes the task composition system.
LGTM 👍
Note: This is a draft PR — when ready for final review, please mark it ready for review.
📝 Update (fa29499)
Commit: fa29499a — "fix stale comment"
Changes since 9db9e8a:
-
Performance optimization in
composite_task_base.py:- Loop restructured to evaluate subtask success functions once per subtask, then distribute results across envs
- Previously: nested loop called success func for each (env, subtask) pair — O(envs × subtasks) function calls
- Now: outer loop over subtasks, call success func once (returns results for all envs), inner loop distributes — O(subtasks) function calls
- This is a meaningful performance improvement for multi-env training
-
Comment cleanup in
composite_task_base.py:- Fixed stale docstring: "sequential task" → "composite task" in
get_metrics()
- Fixed stale docstring: "sequential task" → "composite task" in
-
Parallel optimization in
sequential_task_base.py:- Similar batching optimization applied to the sequential variant
- Added smart subtask filtering: only evaluates success functions for subtasks that are currently active across any env (
sorted(set(env._current_subtask_idx))) - Falls back to full evaluation when
desired_subtask_success_staterequires checking all subtasks
Assessment: Good cleanup commit. The loop restructuring is a sensible optimization that reduces redundant subtask function evaluations from O(N×M) to O(N) where N=subtasks, M=envs. The stale comment fix addresses the terminology inconsistency.
Still LGTM 👍
peterd-NV
requested review from
alexmillane,
cvolkcvolk,
qianl-nv,
viiik-inside,
xyao-nv and
zhx06
as code owners
Greptile SummaryThis PR introduces
Confidence Score: 5/5Safe to merge; the core latch and state-machine logic is correct and well-covered by new tests. The refactoring is structurally sound: static-method override dispatch works correctly, the sequential state machine is preserved, and the composite latch is implemented cleanly. The two observations are an edge-case semantic trap with all-None desired state and a pre-existing performance note about evaluating unreachable subtasks, neither of which causes incorrect task completion in normal usage. composite_task_base.py — the all-None desired_subtask_success_state edge case is worth a follow-up guard or documentation note. Important Files Changed
Class Diagram%%{init: {'theme': 'neutral'}}%%
classDiagram
class TaskBase {
+get_scene_cfg()
+get_events_cfg()
+get_termination_cfg()
+get_metrics()
+get_mimic_env_cfg()
}
class CompositeTaskBase {
+subtasks: list[TaskBase]
+desired_subtask_success_state: list[bool|None]|None
+composite_task_success_func(env, subtasks, desired)$
+reset_subtask_success_state(env, env_ids, subtasks)$
+get_scene_cfg()
+get_events_cfg()
+get_termination_cfg()
+get_metrics()
+get_mimic_env_cfg()
}
class SequentialTaskBase {
+composite_task_success_func(env, subtasks, desired)$
+reset_subtask_success_state(env, env_ids, subtasks)$
}
TaskBase <|-- CompositeTaskBase
CompositeTaskBase <|-- SequentialTaskBase
Reviews (3): Last reviewed commit: "Merge branch 'main' of github.com:isaac-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def get_metrics(self) -> list[MetricBase]: | ||
| "Get metrics for the sequential task." |
There was a problem hiding this comment.
CompositeTaskBase, not the sequential task class.
| def get_metrics(self) -> list[MetricBase]: | |
| "Get metrics for the sequential task." | |
| def get_metrics(self) -> list[MetricBase]: | |
| "Get metrics for the composite task." |
alexmillane
left a comment
alexmillane
left a comment
There was a problem hiding this comment.
Looks good. A few minor comments.
| self, | ||
| subtasks: list[TaskBase], | ||
| episode_length_s: float | None = None, | ||
| desired_subtask_success_state: list[bool | None] | None = None, |
There was a problem hiding this comment.
The meaning of this argument is not immediately obvious to me. Are there cases where we'd like to build a composite task, but that we don't want some of the task to be successful?
Suggestion to add a short docstring.
There was a problem hiding this comment.
Added a docstring to explain. It's an optional argument that can be used to specify the precise subtask success value at the end of the episode. When not provided, once a subtask flips True, it is deemed "completed" for the rest of the episode regardless of it's current success state is True or False at the end of the episode. This is desirable for cases when multiple subtask successes should not be true at the same time (e.g. bottle grasped, bottle in fridge)
This argument gives the user the flexibility to enforce when subtask successes "should" be true (e.g. bottle in fridge, fridge door closed).
| current_subtask_success_state[env_idx][subtask_idx] = True | ||
| env._subtask_success_state[env_idx][subtask_idx] = True |
There was a problem hiding this comment.
What's the difference between these two variables? I guess they must represent different things, but it's not clear from the naming. Suggestion to a) rename to make things clearer, and b) add a comment if "a)" is not possible.
There was a problem hiding this comment.
Hmm I think I see. One is if a sub-task has ever been successful and the other is if it is successful right now. Suggestion to rename :)
There was a problem hiding this comment.
You got it exactly right! 👍 _subtask_success_state tracks whether a subtask has ever succeeded (latched), while current_subtask_success_state is the live/instantaneous success status.
A rename to make that distinction clearer sounds reasonable — something like _subtask_ever_succeeded vs _subtask_currently_succeeding would be more self-documenting. Worth flagging to the author if you feel strongly about it!
There was a problem hiding this comment.
Updated the naming to "subtask_ever_succeeded" and "subtask_currently_succeeding" to be more clear.
| per_env_success = [ | ||
| all( | ||
| env._subtask_success_state[env_idx][i] and current_subtask_success_state[env_idx][i] == desired | ||
| for i, desired in enumerate(desired_subtask_success_state) | ||
| if desired is not None | ||
| ) | ||
| for env_idx in range(env.num_envs) | ||
| ] |
There was a problem hiding this comment.
Personally, and maybe I'm not Pythonic enough, I would break this nested list composition into for loops. It's quite hard to interpret what's happening here. I can't quite tell. I suspect it would be easier if this was formatted as a raw loop.
| # Check for duplicate fields across subtask scene configs and warn if found | ||
| duplicates = check_configclass_field_duplicates(*(subtask.get_scene_cfg() for subtask in self.subtasks)) | ||
| if duplicates: | ||
| import warnings |
There was a problem hiding this comment.
Any reason to import this locally? Suggestion to move this to the top if not
There was a problem hiding this comment.
moved to the top
| for subtask in self.subtasks[1:]: | ||
| subtask_eef_names_set = set(subtask.get_mimic_env_cfg(arm_mode).subtask_configs.keys()) | ||
| if subtask_eef_names_set != mimic_eef_names: | ||
| raise ValueError( | ||
| f"All subtasks must have the same Mimic eef_names.\nSubtask 0 has eef_names: {mimic_eef_names}, but" | ||
| f" subtask {self.subtasks.index(subtask)} has eef_names: {subtask_eef_names_set}." | ||
| ) |
There was a problem hiding this comment.
Suggestion to move out to another function.
| setattr(mimic_env_cfg.datagen_config, key, value) | ||
|
|
||
| mimic_env_cfg.subtask_configs = self.combine_mimic_subtask_configs(arm_mode) | ||
| return mimic_env_cfg |
There was a problem hiding this comment.
One thing about this class and (unfortunately, the framework), is that we have been inconsistent with the naming of functions with leading underscores. I feel that some of these functions should have leading underscores to indicate that they're internal functions... But admittedly we've not done that consistently, so up to you.
| env, | ||
| subtasks: list[TaskBase], | ||
| desired_subtask_success_state: list[bool] | None, | ||
| desired_subtask_success_state: list[bool | None] | None, |
There was a problem hiding this comment.
Suggestion to add an expanded docstring to indicate the meaning of this variable.
| # Compute the success state for the subtasks | ||
| for subtask_idx in subtasks_to_evaluate: | ||
| subtask_success_func = subtasks[subtask_idx].get_termination_cfg().success.func | ||
| subtask_success_params = subtasks[subtask_idx].get_termination_cfg().success.params | ||
| results = subtask_success_func(env, **subtask_success_params) | ||
| for env_idx in range(env.num_envs): | ||
| if results[env_idx]: | ||
| current_subtask_success_state[env_idx][subtask_idx] = True |
There was a problem hiding this comment.
This is a (near) duplicate of a loop used in the base class. Could we move this to a shared function?
| class CompositeOpenDoorTask(CompositeTaskBase): | ||
| def __init__( | ||
| self, | ||
| subtasks, | ||
| episode_length_s=None, | ||
| ): | ||
| super().__init__(subtasks=subtasks, episode_length_s=episode_length_s) | ||
|
|
||
| def get_metrics(self): | ||
| return [] | ||
|
|
||
| def get_prompt(self): | ||
| return "" | ||
|
|
||
| def get_mimic_env_cfg(self, embodiment_name: str): | ||
| return None |
There was a problem hiding this comment.
Do we need to subclass CompositeTaskBase here?
We don't really add anything to the base class. This structure makes constructing a composite task quite verbose.
Can we not do CompositeTaskBase([subtask_1, subtask_2])?
Also with the current approach we can do
pnp_1 = PickAndPlaceTask(apple, bowl)
pnp_2 = PickAndPlaceTask(banana, bowl)
open_door_composite = OpenDoorCompositeTask([pnp_1, pnp_2])
…d/composite_task_v03
2 participants
Summary
This PR refactors composite tasks to create two classes:
CompositeTaskBaseclass that enables "order-does-not-matter" subtask completion. Mostly copied over from existing SequentialTaskBase.SequentialTaskBasethat is dervied from CompositeTaskBase for "order-does-matter" tasks.Tests are updated as follows:
Other updates include:
desired_subtask_success_stateby setting subtask value toNone.API usage of composite tasks remain the same as before.