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

fix: automatically download MuJoCo tar #19

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

JeanElsner
Copy link
Contributor

@JeanElsner JeanElsner commented Oct 11, 2023

The MuJoCo .tar file path is always set by default and therefore never downloaded. I fixed this in 633bcfd. Additionally, there are some references to MJLIB_PATH which is no longer used. Specifically, build.sh will look for the MuJoCo library in the home directory and fail if it's not found. I removed this old code in 68da273

Hi, I'm not sure what your update-plans are, maybe this proves useful to you.
This PR builds dm_robotics against the current MuJoCo (2.3.7) and corresponding dm_control (1.0.14) versions. Updating those dependencies drops support for Python 3.7 but is compatible with Python 3.8 up to 3.11. Optionally, I could also add support for tox >= 4 if this is something you're interested in.

@josechenf
Copy link
Collaborator

Hey Jean! Thank you, we'll review this soon.

Currently, there's some ongoing work to start migrating to build this library by linking MuJoCo statically. This is still experimental/in progress however, but we can try to speed it up!
We'll keep you updated!

@JeanElsner
Copy link
Contributor Author

Thanks, sounds great :)
Did you also consider using a more modern build system like scikit-build-core? I love the controllers package of this repository but I feel like the entire build process could be more streamlined.

@josechenf
Copy link
Collaborator

josechenf commented Oct 24, 2023

Hi Jean, please take a look at the latest commit. We should now be using MuJoCo 3.0.0 and the associated dm_control version.

In regards to a more modern build system, unfortunately we haven't had the time to consider other build systems, as CMake is the system that most of us are comfortable with. However, we'd be happy to consider pull requests that add support for other build systems (as long as it doesn't break the CMake build)!

Edit: Also note that MuJoCo is now downloaded and extracted automatically (unless the user specifies a path for the .tar file).

@JeanElsner JeanElsner reopened this Oct 30, 2023
@JeanElsner
Copy link
Contributor Author

Looks great, thank you so much! And it's MuJoCo 3.0.0, too! I managed to build all the packages and ran all the tests successfully in different environments and Python versions.

However, I saw that the MuJoCo .tar file path is always set by default and therefore never downloaded. I fixed this in 633bcfd. Additionally, there are some references to MJLIB_PATH which is no longer used. Specifically, build.sh will look for the MuJoCo library in the home directory and fail if it's not found. I removed this old code in 68da273

By the way, I wasn't precise before, I didn't mean to suggest to replace the CMake build system but rather the packaging system. Currently you use setuptools and call setup.py directly, which is no longer the recommended way to build Python packages. It's not a big issue or anything. You could also just add a minimal pyproject.toml and stick with setuptools (dm_control does this).

@shacklestone
Copy link
Collaborator

Thanks, Jean.

I (or someone else) will take a look at this, but with the holidays it will be early next year.

@JeanElsner JeanElsner changed the title Update to current MuJoCo / dm_control fix: automatically download MuJoCo tar Jun 18, 2024
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.

3 participants