-
Notifications
You must be signed in to change notification settings - Fork 5
Add SequentialTaskBase class #289
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
LTGM. Maybe can think about how to get rid of those for env_id in env_ids loop in the future. |
alexmillane
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great work. This is going to be a big upgrade in capabilities.
I have a few comments. Happy to take another look!
| A base class for tasks composed sequentially from multiple subtasks. | ||
| The sequential task takes a list of TaskBase instances (subtasks), | ||
| and automatically collects configs to form a composite task. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion to expand the definition of a sequential task. I guess "sequential" is a specific form of composite, in that the atomic tasks must be completed in order, one after another. Once completed, a sub-tasks can then go back to being not-true, and the sequential task still completes. This is in contrast to something like a composite task where atomic tasks can be completed in any order, or a task where all atomic tasks must be true at once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I've expanded the description with a more detailed definition to what "sequential" here means
| @staticmethod | ||
| def _add_suffix_to_configclass_fields(cfg_instance: Any, suffix: str) -> Any: | ||
| """Create a new configclass instance with all field names appended with a suffix.""" | ||
| if cfg_instance is None: | ||
| return None | ||
|
|
||
| fields = dataclasses.fields(cfg_instance) | ||
| new_fields = [] | ||
| field_values = {} | ||
|
|
||
| # Rename the fields with suffix | ||
| for field in fields: | ||
| new_name = f"{field.name}{suffix}" | ||
| value = getattr(cfg_instance, field.name) | ||
| new_fields.append((new_name, field.type, value)) | ||
| field_values[new_name] = value | ||
|
|
||
| # Create a new configclass with renamed fields | ||
| new_cfg_class = make_configclass(type(cfg_instance).__name__, new_fields) | ||
| return new_cfg_class(**field_values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion to move this to isaaclab_arena.utils.configclass. We have a few other utilities in that file for modifying config classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment below on move to unified configclass transform func.
| @staticmethod | ||
| def _remove_configclass_fields(cfg_instance: Any, exclude_fields: set[str]) -> Any: | ||
| "Remove all fields from the configclass instance that are in the exclude_fields set." | ||
| if cfg_instance is None: | ||
| return None | ||
|
|
||
| fields = dataclasses.fields(cfg_instance) | ||
| new_fields = [] | ||
| field_values = {} | ||
|
|
||
| # Remove the fields that are in the exclude_fields set | ||
| for field in fields: | ||
| if field.name in exclude_fields: | ||
| continue | ||
| value = getattr(cfg_instance, field.name) | ||
| new_fields.append((field.name, field.type, value)) | ||
| field_values[field.name] = value | ||
|
|
||
| if not new_fields: | ||
| return None | ||
|
|
||
| # Create a new configclass without the excluded fields | ||
| new_cfg_class = make_configclass(type(cfg_instance).__name__, new_fields) | ||
| return new_cfg_class(**field_values) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As above, suggestion to move this to isaaclab_arena.utils.configclass.
Additionally, the two functions above have a very similar form. They loop through the fields, build a new config class by performing some mutation on each field. I wonder if we could write them in terms of a function transform_config_class which takes a Callable, loops through the fields, calling the callable on each and adding the resulting field to a new configclass if the return is not None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with that. Moving to to a generic "transform_config_class" function would be cleaner and more reusable by other parts of the code down the line. I've added the following:
- A
transform_configclass_instancefunction inside ofutils/configclass.py. This function performs the "loop" seen in the original functions and takes a Callable to perform the transform - Added simple Callables in the SequentialTaskBase class to perform the transforms
- Updated SequentialTaskBase class tests to use this format
| success = TerminationTermCfg( | ||
| func=self.sequential_task_success_func, | ||
| params={ | ||
| "task_instance": self, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this construction of a static method which takes self a bit unexpected. Arguably the defining characteristic of a static method is that it doesn't take self.
Is it possible that we make sequential_task_success_func non-static, and then do
from functools import partial
...
success = TerminationTermCfg(
func=partial(SequentialTaskBase.sequential_task_success_func, self)
)
We can then simplify SequentialTaskBase.sequential_task_success_func, as it will have the form of a normal method, with accecss to self etc.
Maybe there's something I'm missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm I think setting the success/event functions to be non-static and use "self" would present an issue with other parts of Lab. Namely in some scripts like data recording, robomimic replay, mimic etc, they extract and use these functions directly to take full control of the env. For those, they depend on having the termination/event functions follow the standard function signature for Lab which is something like func(env, params...). If we define these as normal class methods then it would cause an issue with a lot of downstream scripts.
In the current setup, the "self" that is passed to this function is essentially a copy of the as Lab makes a deep copy of all configs before instantiating the managers. However, I do agree that it's really not ideal to be passing "self" to a static method.
I've made some changes which preserves these functions as staticmethods to keep the standard Lab function signature, but changed the argument to be the list of subtasks instead of an instance of SequentialTaskBase so that it's not directly using a copy of "self". What do you think of this solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanation. I hadn't considered that Lab expects a certain signature.
With that said, I feel that the using partial should result in the correct signature no?
Maybe I'm mistake but I thought that:
class MyClass:
def my_method(self, env, param)
my_class = MyClass()
my_partial = partial(my_class.my_method, self)
results in my_partial having the signature of func(env, param). Perhaps I'm mistaken.
With all of that said. Let's merge this all is. It's not that important.
| "Make combined scene cfg from all subtasks." | ||
| scene_cfg = combine_configclass_instances("SceneCfg", *(subtask.get_scene_cfg() for subtask in self.subtasks)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should add something in isaaclab_arena.utils.configclass to detect if there are repeated config class members. Here we could check and warn (before proceeding).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make's sense, I've added a function to utils.configclass to scan and return any duplicates: check_configclass_field_duplicates
If duplicates are found it'll give users a warning showing the names of them.
| if __name__ == "__main__": | ||
| test_add_suffix_to_configclass_fields() | ||
| test_remove_configclass_fields() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great!
…d/sequential_task_base
|
Thanks for the great feedback @alexmillane @xyao-nv. I've pushed new changes that address each comment. Please take a look to see if you have any further comments. |
…d/sequential_task_base
alexmillane
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
| success = TerminationTermCfg( | ||
| func=self.sequential_task_success_func, | ||
| params={ | ||
| "task_instance": self, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the explanation. I hadn't considered that Lab expects a certain signature.
With that said, I feel that the using partial should result in the correct signature no?
Maybe I'm mistake but I thought that:
class MyClass:
def my_method(self, env, param)
my_class = MyClass()
my_partial = partial(my_class.my_method, self)
results in my_partial having the signature of func(env, param). Perhaps I'm mistaken.
With all of that said. Let's merge this all is. It's not that important.
viiik-inside
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really cool!
Summary
This PR adds initial support for composite sequential tasks via the SequentialTaskBase class. The SequentialTaskBase class takes a list of atomic tasks (TaskBase) and automatically assembles them into a composite task with unified termination/event configs.
Adds:
isaac_arena/utils/configclass.pyto perform config transformation and duplicate checking