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

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

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

Conversation

traversaro
Copy link
Contributor

Note

This is a draft pull request as it requires the changes proposed in #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 #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.

All instances of mujoco_test in the code base were followed by
a call to  target_link_libraries(<testname> fixture gmock).

As anyhow mujoco_test already was calling target_link_libraries
to some predefined list of targets (mujoco and gtest_main),
this PR adds to the list of default linked targets also fixture
and gmock, to reduce the boilerplate.

Furthermore, to completly remove the need for calling
target_link_libraries after a call to mujoco_test, this PR also
add to the mujoco_test macro the ADDITIONAL_LINK_LIBRARIES
argument, that can be used if a given test needs to link some
additional targets beside the default ones.

To permit to use these new features also for mujoco benchmarks,
this PR adds a MAIN_TARGET parameter to mujoco_test, to select
if gtest_main or another target is used to provide the main
entry point to the test executable.
@traversaro
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

static_assert(sizeof(mjtNum) == 8);
. Indeed, thanks to this I now discovered that Python bindings explicitly only support double scalar, see
static_assert(sizeof(mjtNum) == 8);
. So we should simply skip testing on Python bindings, and document that MUJOCO_USE_SINGLE_PRECISION is not supported with Python bindings.

@yuvaltassa yuvaltassa requested a review from saran-t December 16, 2024 17:58
@yuvaltassa
Copy link
Collaborator

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.

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