-
Notifications
You must be signed in to change notification settings - Fork 849
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
base: main
Are you sure you want to change the base?
Conversation
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.
…o with single precision
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). |
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 As the #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 |
…ION CMake option is enabled
I implemented this in mujoco/python/mujoco/serialization.h Line 33 in b26d6f0
mujoco/python/mujoco/serialization.h Line 33 in b26d6f0
MUJOCO_USE_SINGLE_PRECISION is not supported with Python bindings.
|
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. |
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 toOFF
) 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 definedmjUSESINGLE
.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 theMUJOCO_USE_SINGLE_PRECISION
option is enabled. To permit this, the PR adds aTAGS
argument to themujoco_test
CMake routine, and it changes it frommacro
tofunction
to avoid counterintuitive behavior of thereturn()
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
: likeubuntu-22.04-gcc-12
, but with theMUJOCO_USE_SINGLE_PRECISION
option enabled,ubuntu-22.04-clang-14-single-precision
: likeubuntu-22.04-gcc-12
, but with theMUJOCO_USE_SINGLE_PRECISION
option enabled.