mujoco icon indicating copy to clipboard operation
mujoco copied to clipboard

[Draft] Add option MUJOCO_USE_SINGLE_PRECISION to enable compilation of MuJoCo with single precision

Open traversaro opened this issue 1 year ago • 4 comments

[!NOTE]
This is a draft pull request as it requires the changes proposed in https://github.com/google-deepmind/mujoco/pull/2276

This PR adds a MUJOCO_USE_SINGLE_PRECISION CMake option (default value set to OFF) that can be used to compile MuJoCo with single precision, i.e. ensuring that mujoco and any downstream library or binary linked with CMake will defined mjUSESINGLE.

This should simplify the use of MuJoCo compiled with single precision for CMake users, see for example https://github.com/google-deepmind/mujoco/issues/2070 .

As many tests do not compile or run correctly if MuJoCo is compiled with single precision, we annotate those tests with the nofloat32 tag, and we ensure that they are not compiled if the MUJOCO_USE_SINGLE_PRECISION option is enabled. To permit this, the PR adds a TAGS argument to the mujoco_test CMake routine, and it changes it from macro to function to avoid counterintuitive behavior of the return() command inside it.

To ensure that the option continues to work in the future, the PR adds two additional GitHub Actions jobs:

  • ubuntu-22.04-gcc-12-single-precision: like ubuntu-22.04-gcc-12, but with the MUJOCO_USE_SINGLE_PRECISION option enabled,
  • ubuntu-22.04-clang-14-single-precision: like ubuntu-22.04-gcc-12, but with the MUJOCO_USE_SINGLE_PRECISION option enabled.

traversaro avatar Dec 07 '24 16:12 traversaro

Another open point is that Python tests are currently failing. Is the case single precision + Python bindings supported in MuJoCo? If yes, I need to understand why the CI fails now, if not instead we need to make sure that it is impossible to compile the python bindings against a MuJoCo compiled with MUJOCO_USE_SINGLE_PRECISION option set to ON (to early fail instead of failing at runtime).

traversaro avatar Dec 07 '24 16:12 traversaro

Ok, I think I understood one reason why the Python test fail. What is happening is that the Python bindings consume the library by manually adding the mujoco imported target (see https://github.com/google-deepmind/mujoco/blob/3.2.6/python/mujoco/CMakeLists.txt#L94-L97), so the target_compile_definitions(mujoco PUBLIC mjUSESINGLE) in our code is effectively ignored.

As the find_package(mujoco) is effectively not supported by official C/C++ binaries and undocumented, this may be the case of many downstream consumers of mujoco binaries. If we can't assume that downstream consumers of mujoco consume it via find_package, to ensure that the definition of mjUSESINGLE is consistently set in all downstream compilation units, the other alternative is to add a block:

#ifndef mjUSESINGLE
#define mjUSESINGLE
#endif

in some installed header. This is typically done in C/C++ projects by creating at build time a config.h that contains macros to describe all the compile-time time options that affect the ABI of the installed artifact, but that is not trivial to do by just modifying the CMake (and without modifying the underlying blaze rules of mujoco). A possible CMake-only alternative is that we patch the installed headers to contain #define mjUSESINGLE in case MuJoCo is compiled with the MUJOCO_USE_SINGLE_PRECISION . That is a bit brittle as the installed header would not be exactly the one contained in the source directory, but I guess it is the best possible compromise without modifications to the blaze internal rules of mujoco.

traversaro avatar Dec 15 '24 14:12 traversaro

A possible CMake-only alternative is that we patch the installed headers to contain #define mjUSESINGLE in case MuJoCo is compiled with the MUJOCO_USE_SINGLE_PRECISION . That is a bit brittle as the installed header would not be exactly the one contained in the source directory, but I guess it is the best possible compromise without modifications to the blaze internal rules of mujoco.

I implemented this in https://github.com/google-deepmind/mujoco/blob/b26d6f0466f003849dfe0981874b4f743aeaf0e8/python/mujoco/serialization.h#L33 . Indeed, thanks to this I now discovered that Python bindings explicitly only support double scalar, see https://github.com/google-deepmind/mujoco/blob/b26d6f0466f003849dfe0981874b4f743aeaf0e8/python/mujoco/serialization.h#L33 . So we should simply skip testing on Python bindings, and document that MUJOCO_USE_SINGLE_PRECISION is not supported with Python bindings.

traversaro avatar Dec 15 '24 18:12 traversaro

Well, we could support float32 for the bindings, however it's not clear that this is needed by anyone right now. So I think for now we'll document that it's not supported and tell people to ask for it if they need it.

yuvaltassa avatar Dec 17 '24 12:12 yuvaltassa