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

[Improvement] Best way to set MUJOCO_GL #55

Open
aliberts opened this issue Mar 27, 2024 · 5 comments
Open

[Improvement] Best way to set MUJOCO_GL #55

aliberts opened this issue Mar 27, 2024 · 5 comments
Assignees
Labels
✨ Enhancement New feature or request 🧪 Simulation Something simulation-related

Comments

@aliberts
Copy link
Collaborator

aliberts commented Mar 27, 2024

Right now, some online training won't work when the env uses mujoco if the environment variable MUJOCO_GL has not been set to egl.

Since there's no indication for that in the current README, I'm creating this issue to discuss the best way to address this moving forward. Here are the 2 ways that I see for how we could solve this:

1. Explicitly tell the user in the README to set it:

export MUJOCO_GL=egl

2. Set it directly in the code (and make it invisible to the user):

os.environ["MUJOCO_GL"] = "egl"

I don't really know what's the best way here. What do you think @Cadene @alexander-soare?

@aliberts aliberts self-assigned this Mar 27, 2024
@aliberts aliberts changed the title export MUJOCO_GL=egl [Improvement] Best way to set MUJOCO_GL Mar 27, 2024
@alexander-soare
Copy link
Collaborator

IMO we should avoid making the user export env variables at almost all costs (it's cumbersome, and if you have a policy of allowing it, there will be more).

Questions:

  1. Is it potentially intrusive or damaging to set it silently? Would the user really want to know about it?
  2. Can we check under what conditions it needs to be set and only set it under those (Gemini tells me it's needed when running serverless, and I bet there's a way to detect that).

@aliberts
Copy link
Collaborator Author

aliberts commented Mar 27, 2024

IMO we should avoid making the user export env variables at almost all costs (it's cumbersome, and if you have a policy of allowing it, there will be more).

I agree, it's probably not good practice

  1. Is it potentially intrusive or damaging to set it silently? Would the user really want to know about it?

Env variables set in the script only live in the script's execution environment and don't persist in the shell after execution so this should be fine.

  1. Can we check under what conditions it needs to be set and only set it under those (Gemini tells me it's needed when running serverless, and I bet there's a way to detect that).

For sure, I'll try to narrow down these conditions to see where it would be best to set it (in case we go with option 2)

@aliberts
Copy link
Collaborator Author

aliberts commented Apr 6, 2024

Something like this should do ok: huggingface/gym-xarm@4c65f3e
I'll add it to the other envs if needed

@Cadene
Copy link
Collaborator

Cadene commented Apr 11, 2024

Is it addressed? @aliberts

@aliberts
Copy link
Collaborator Author

aliberts commented Apr 12, 2024

Yes, in gym-xarm (the other envs don't use the same rendering engine)
Reopening this as tests don't pass without MUJOCO_GL=egl

@aliberts aliberts reopened this Apr 12, 2024
@aliberts aliberts added ✨ Enhancement New feature or request 🧪 Simulation Something simulation-related labels Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Enhancement New feature or request 🧪 Simulation Something simulation-related
Projects
None yet
Development

No branches or pull requests

3 participants