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

Raise error for pusher-v4 environment when mujoco>=3 #963

Merged
merged 7 commits into from
Apr 8, 2024

Conversation

Kallinteris-Andreas
Copy link
Collaborator

@Kallinteris-Andreas Kallinteris-Andreas commented Mar 12, 2024

Description

#950 noticed that pusher-v4 had collision issues which was isolated to a bug fixed in mujoco==3.0.0
This collision issue has a major implement on performance, therefore, raising an error for pusher-v4 when mujoco>=3
The issue will be fixed in pusher-v5

@Kallinteris-Andreas
Copy link
Collaborator Author

What do we do for testing?

@pseudo-rnd-thoughts
Copy link
Member

What do we do for testing?

Remove from the set of testing environments

We could add a specialist CI case where mujoco < 3 is installed just in case.
But with v5 fixing the issue, I don't imagine many people will use it.

Is there anyway that we can check that a similar bug doesn't exist in other environments?

@Kallinteris-Andreas
Copy link
Collaborator Author

  1. To test that no other environment was broken With Mujoco 3.0 . It should be sufficient to train a single episode. Render the trained policy. And ensure that everything looks fine

@Kallinteris-Andreas Kallinteris-Andreas marked this pull request as draft March 27, 2024 17:46
@pseudo-rnd-thoughts pseudo-rnd-thoughts changed the title pusher_v4 version control to mujoco<3 Disable pusher-v4 environment for mujoco<3 Apr 6, 2024
@pseudo-rnd-thoughts pseudo-rnd-thoughts changed the title Disable pusher-v4 environment for mujoco<3 Raise error for pusher-v4 environment when mujoco>=3 Apr 6, 2024
Copy link
Member

@pseudo-rnd-thoughts pseudo-rnd-thoughts left a comment

Choose a reason for hiding this comment

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

@Kallinteris-Andreas Were you planning on anymore changes?

@Kallinteris-Andreas
Copy link
Collaborator Author

Kallinteris-Andreas commented Apr 8, 2024

I have no idea why this is passing CI, pusher v4 should be failing its instanciacion

@pseudo-rnd-thoughts
Copy link
Member

It does but try_make_env catches the error and prevents the environment from being run

@Kallinteris-Andreas
Copy link
Collaborator Author

Kallinteris-Andreas commented Apr 8, 2024

Did not realize you commited to it, makes sense now

@Kallinteris-Andreas Kallinteris-Andreas marked this pull request as ready for review April 8, 2024 09:02
@Kallinteris-Andreas Kallinteris-Andreas merged commit 6047a8c into main Apr 8, 2024
27 checks passed
@pseudo-rnd-thoughts pseudo-rnd-thoughts deleted the Kallinteris-Andreas-patch-3 branch May 21, 2024 08:56
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.

2 participants