search

google-deepmind / dm_robotics

Libraries, tools and tasks created and used at DeepMind Robotics.
Apache License 2.0
332 stars 32 forks source link

fix: automatically download MuJoCo tar #19

Open JeanElsner opened 10 months ago

JeanElsner commented 10 months ago

The MuJoCo .tar file path is always set by default and therefore never downloaded. I fixed this in https://github.com/google-deepmind/dm_robotics/pull/19/commits/633bcfd74dbea91ce30229f35e94f4a4dde4c771. 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 https://github.com/google-deepmind/dm_robotics/pull/19/commits/68da2735371b74e5b3387d2eef32da07a39ca456

~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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 https://github.com/google-deepmind/dm_robotics/pull/19/commits/633bcfd74dbea91ce30229f35e94f4a4dde4c771. 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 https://github.com/google-deepmind/dm_robotics/pull/19/commits/68da2735371b74e5b3387d2eef32da07a39ca456

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 commented 8 months ago

Thanks, Jean.

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