Skip to content
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

Fixes Fetch and Robotic environments initial state issues in 1.3.0 #256

Merged
merged 8 commits into from
Dec 19, 2024

Conversation

wjxgeorge
Copy link
Contributor

@wjxgeorge wjxgeorge commented Nov 26, 2024

Adding back the lines described in #251 to fix the initial state changes introduced in gymnasium-robotics>=1.3.0, <=1.3.1.

Description

Adding back removed data setting lines removed in 1.3.0. Following Gymnasium's mujoco_env.py workflow, the procedure to reset an environment for Fetch and Robotic environments are:

  1. mujoco._mj_resetData(self.model, self.data)
  2. Update self.data to the saved initial data.
  3. mujoco.mf_forward(self.model, self.data)

The second step changes for different environments. Changes in 1.3.0 removed the second step. The third step is called in set_state() in mujoco_env.py.

Update impacted Fetch environments to V4 and HandReach environment to V3. Other Shadow Dexterous Hand environments are not impacted since they inherit MujocoManipulateEnv which has a overridden _reset_sim().

Fixes #251

Type of change

Please delete options that are not relevant.

  • Documentation only change (no code changed)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Screenshots

Please attach before and after screenshots of the change if applicable.

Checklist:

  • I have run the pre-commit checks with pre-commit run --all-files (see CONTRIBUTING.md instructions to set it up)
  • I have commented my code, particularly in hard-to-understand areas
  • 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
  • New and existing unit tests pass locally with my changes

@wjxgeorge
Copy link
Contributor Author

Should I update the test file as well? The one I'm using to test it is comparing the self.data.qpos and self.initial_qpos. Same goes for the qvel. This seems a little bit redundant.

@Kallinteris-Andreas
Copy link
Collaborator

In gymnasium we have this test, but I do not believe this applies to these environments because we do not have reset noise scale parameter.
https://github.com/Farama-Foundation/Gymnasium/blob/13230f4a120336b45c880b644585056d31f4c956/tests/envs/mujoco/test_mujoco_v5.py#L680-L712

@wjxgeorge
Copy link
Contributor Author

In gymnasium we have this test, but I do not believe this applies to these environments because we do not have reset noise scale parameter. https://github.com/Farama-Foundation/Gymnasium/blob/13230f4a120336b45c880b644585056d31f4c956/tests/envs/mujoco/test_mujoco_v5.py#L680-L712

Specific to the impacted environments, the problem would be the randomized object location in Push, PickAndPLace and Slide. I think it's fair to ignore the object location since they are computed from the initial gripper position. If that's ok, I can modify the function and add a similar test function for Fetch environments.

@Kallinteris-Andreas
Copy link
Collaborator

Add the test

@Kallinteris-Andreas
Copy link
Collaborator

also tests/envs/hand/test_reach.py:11 should be updated to v3

@wjxgeorge
Copy link
Contributor Author

I added the test in test_envs.py and update test_reach.py's test version.

@ChanJoon
Copy link

ChanJoon commented Dec 4, 2024

I'm currently looking into how to fix starting position of the "Fetch" environment.
Is this PR valid? Could you let me know if this version is appropriate to apply in the latest source code, or should I stick with the current version? (if so, is this asymmetrical starting position is okay to achieve proper rewards?)

@wjxgeorge
Copy link
Contributor Author

wjxgeorge commented Dec 4, 2024

I'm currently looking into how to fix starting position of the "Fetch" environment. Is this PR valid? Could you let me know if this version is appropriate to apply in the latest source code, or should I stick with the current version? (if so, is this asymmetrical starting position is okay to achieve proper rewards?)

This PR is for fixing the Fetch-V3 environments' incorrect starting position introduced in the deterministic rollout fix in 1.3.0. I'm working with the maintainer to incorporate the fix to the main branch.

Are you facing the "asymmetrical starting position" in gymnasium-robotics<1.3.0? If not, then likely the issue is caused by the fix in 1.3.0 as well and you are welcome to try the code in this PR.

If you are still facing that issue in gymnasium-robotics<1.3.0, then it would be better for you to open a new bug report and provide more details so maintainers are aware of the issue.

Copy link
Collaborator

@Kallinteris-Andreas Kallinteris-Andreas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking Good, we now update the changelog of the environmets

example:

## Version History
* v3: Fixed bug: `env.reset()` not properly resetting the internal state. Fetch environments now properly reset their state (related [GitHub issue](https://github.com/Farama-Foundation/Gymnasium-Robotics/issues/207)).
* v2: the environment depends on the newest [mujoco python bindings](https://mujoco.readthedocs.io/en/latest/python.html) maintained by the MuJoCo team in Deepmind.
* v1: the environment depends on `mujoco_py` which is no longer maintained.
"""

Copy link
Collaborator

@Kallinteris-Andreas Kallinteris-Andreas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apply the change to all the environments, I think this is the last change

gymnasium_robotics/envs/fetch/pick_and_place.py Outdated Show resolved Hide resolved
tests/test_envs.py Outdated Show resolved Hide resolved
tests/test_envs.py Show resolved Hide resolved
tests/test_envs.py Show resolved Hide resolved
@Kallinteris-Andreas Kallinteris-Andreas merged commit 0ade016 into Farama-Foundation:main Dec 19, 2024
6 checks passed
@kuds
Copy link

kuds commented Dec 22, 2024

Thank you for fixing this issue! I successfully trained a PPO agent on Fetch Reach by installing gymnasium robotics from source. Do you know when this might be available via pip?

FetchReachDense-v4
best_model_ppo_fetch_reach-step-0-to-step-100001-ezgif com-video-to-gif-converter

@Kallinteris-Andreas
Copy link
Collaborator

Thanks at @kuds For the validation.

The next release should be around New Years.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug Report] Fetch V3 initial state problem
4 participants