Conversation
There was a problem hiding this comment.
Pull request overview
This pull request fixes a bug where the task success status was being evaluated after reset_objects_state() was called, resulting in incorrect success detection. The fix saves the success status before resetting objects and uses the saved value during episode initialization.
Changes:
- Added
is_task_success()method toBaseEnvclass for checking task success across environments - Modified
reset()to save task success status before callingreset_objects_state() - Updated
_initialize_episode()inEmbodiedEnvto use the pre-saved success status instead of callingis_task_success()after reset
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| embodichain/lab/gym/envs/base_env.py | Adds is_task_success() method and saves task success status before resetting objects in reset() method |
| embodichain/lab/gym/envs/embodied_env.py | Updates _initialize_episode() to use saved _task_success attribute instead of calling is_task_success() after objects are reset |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self._task_success is None: | ||
| logger.log_warning("task_success is not defined, nothing to save.") | ||
| self._task_success = torch.zeros( | ||
| self.num_envs, dtype=torch.bool, device=self.device | ||
| ) |
There was a problem hiding this comment.
This None check and fallback initialization is unreachable. Since the base class (BaseEnv) now initializes _task_success as a torch.Tensor in line 136-138 of base_env.py, _task_success will never be None. This dead code should be removed, keeping only the use of self._task_success[env_id].item() on line 395.
| if self._task_success is None: | |
| logger.log_warning("task_success is not defined, nothing to save.") | |
| self._task_success = torch.zeros( | |
| self.num_envs, dtype=torch.bool, device=self.device | |
| ) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| gym_env_cfg = config_to_cfg( | ||
| gym_config_data, manager_modules=DEFAULT_MANAGER_MODULES | ||
| ) | ||
|
|
There was a problem hiding this comment.
The num_envs override logic has been removed but not replaced. Previously, if num_envs was specified in the trainer config, it would override the value from the gym config. Now this override is lost, which means the trainer config's num_envs setting is ignored.
The removed code was:
if num_envs is not None:
gym_env_cfg.num_envs = num_envsThis should be re-added after the config_to_cfg call to maintain the ability to override num_envs from the training configuration.
| # Allow trainer config to override number of environments | |
| num_envs = trainer_cfg.get("num_envs") | |
| if num_envs is not None: | |
| gym_env_cfg.num_envs = num_envs |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def is_task_success(self, **kwargs) -> torch.Tensor: | ||
| """Check if the task is successful for each environment. | ||
|
|
||
| This method should be overridden in subclasses to implement task-specific success criteria. | ||
|
|
||
| Args: | ||
| **kwargs: Additional keyword arguments. | ||
|
|
||
| Returns: | ||
| A boolean tensor indicating success for each environment. | ||
| """ | ||
|
|
||
| raise NotImplementedError( | ||
| "BaseEnv.is_task_success must be implemented in subclasses to define " | ||
| "task-specific success criteria." | ||
| ) |
There was a problem hiding this comment.
The is_task_success method added to BaseEnv raises NotImplementedError. This is a breaking change because BaseEnv is now calling this method in the reset() method (line 559). Any environment that inherits from BaseEnv (instead of EmbodiedEnv which provides a default implementation) will now fail when reset() is called. Consider making this method return a default value (like torch.zeros or torch.ones) instead of raising NotImplementedError, or document that this is a required override for BaseEnv subclasses.
2 participants
Description
buf fix: save success status before reseting objects status