Add mujoco recipe #19049
Merged
Add mujoco recipe #19049
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( |
|
Unfortunately don't have a lot to add right now:
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
I was wrong: the compilation works fine with google-deepmind/mujoco@70b19d5 (i.e. current master branch) with both apt's and conda-forge's gcc, but if I switch to |
|
The history is a bit confusing as 2.2.0 is not a tag in main's history, and the "Initial open sourcing of MuJoCo." commit is different between main and 2.2.0 :
However, by manually running a diff between in So 0da0b39 should fix the problem. |
|
Another fix that was in |
|
The recipe tests are now failing due to some additional differences between 2.2.0 and main. I will fix this by backporting google-deepmind/mujoco@098b125, but in this case I opened an issue upstream: google-deepmind/mujoco#296 . |
|
Windows build are failing with: I tried to fix this by switching to clang-cl . |
This worked. So macOS and Linux C++ works fine, so we can start the missing points:
|
|
I've just moved the deepmind/[email protected] tag to google-deepmind/mujoco@098b125 so the source code archive (mujoco-2.2.0.zip and mujoco-2.2.0.tar.gz) should now build and install correctly. Sorry about the confusion. |
Thanks @saran-t ! |
saran-t
reviewed
May 24, 2022
| -DMUJOCO_BUILD_TESTS:BOOL=ON \ | ||
| -DMUJOCO_BUILD_EXAMPLES:BOOL=OFF \ | ||
| -DMUJOCO_ENABLE_AVX:BOOL=OFF \ | ||
| -DMUJOCO_ENABLE_AVX_INTRINSICS:BOOL=OFF \ |
There was a problem hiding this comment.
Please consider adding -DCMAKE_INTERPRODEDURAL_OPTIMIZATION:BOOL=ON as well.
There was a problem hiding this comment.
typo: should be -DCMAKE_INTERPROCEDURAL_OPTIMIZATION
There was a problem hiding this comment.
Thanks, done in traversaro@7575534 and traversaro@a5957f4 .
|
Latest failures are due to the use of |
|
Latest error (reduced): |
|
Given the usage of |
|
New error: Possibly related to the fact that mujoco C++ is compiled with GCC and the bindingw with Python? That symbol is: that indeed suggested that there is a problem with symbols in C++ standard library. Perhaps we should tell the bindings to compile with clang but link with |
This was fixed by patching the Python bindings to avoid the use of The crash seems to be related to |
|
Patches from AUR related to de-vendoring: |
Actually no copy of |
|
Windows is failing with: |
|
Linux is finally working, points currently missing (that can be also done on the feedstock if anyone is interested in having this soon):
|
|
You cannot devendor |
Ack, I did not noticed that the symbols are also modified to avoid conflicts. I modified the bullet point to address that, thanks! |
On this, there are quite a scary Warnings from libccd use |
For this, we probably will swithc libccd to use double as discussed in conda-forge/libccd-feedstock#4 and proposed in conda-forge/libccd-feedstock#5 . |
It seems that the problem is still there even with |
|
The symbol seems to be in the library: So the problem is probably elsewhere. |
I was unable to solve the problem. However, as that symbol is used just to access a |
|
Python bindings on Windows are failing with: |
traversaro
force-pushed
the
patch-14
branch
3 times, most recently
from
e71095f to
75aefaa
Compare
|
The recipe is ready for review @conda-forge/help-c-cpp @conda-forge/help-python-c . Some additional notes/points that is better to explain before:
|
carterbox
requested changes
Jun 20, 2022
There was a problem hiding this comment.
In regards to revendoring tinyobjlibrary. I believe this is OK because the release candidate version should be published on a conda-forge rc channel which means that would probably require end users to add a special channel in order for the package solver to find a solution. I have questions though:
- tinyobjlibrary is statically linked into the mujoco shared library?
- Where is the license for tinyobjlibrary since it is being statically linked into mujoco?
| # abseil-cpp is used only for tests, we can | ||
| # safely ignore its run_exports | ||
| ignore_run_exports: | ||
| - abseil-cpp |
There was a problem hiding this comment.
This library is still being exported on Windows. Maybe you need to add this ignore_run_exports to the python package build section too?
There was a problem hiding this comment.
On Python (differently from C++) the library is used in the bindings themselfs, not on tests, see for example the Linux logs:
2022-06-18T14:40:23.0083572Z INFO (mujoco-python,lib/python3.8/site-packages/mujoco/_structs.cpython-38-x86_64-linux-gnu.so): Needed DSO lib/libabsl_hash.so.2103.0.1 found in conda-forge::abseil-cpp-20210324.2-h9c3ff4c_0
2022-06-18T14:40:23.0147391Z INFO (mujoco-python,lib/python3.8/site-packages/mujoco/_structs.cpython-38-x86_64-linux-gnu.so): Needed DSO lib/libabsl_raw_hash_set.so.2103.0.1 found in conda-forge::abseil-cpp-20210324.2-h9c3ff4c_0
2022-06-18T14:40:23.0176888Z INFO (mujoco-python,lib/python3.8/site-packages/mujoco/_structs.cpython-38-x86_64-linux-gnu.so): Needed DSO x86_64-conda-linux-gnu/sysroot/lib64/libpthread.so.0 found in CDT/compiler package conda-forge::sysroot_linux-64-2.12-he073ed8_15
2022-06-18T14:40:23.0238563Z INFO (mujoco-python,lib/python3.8/site-packages/mujoco/_structs.cpython-38-x86_64-linux-gnu.so): Needed DSO lib/libmujoco.so.2.2.0 found in home/conda/staged-recipes/build_artifacts::mujoco-cxx-2.2.0-h696428e_0
2022-06-18T14:40:23.0298191Z INFO (mujoco-python,lib/python3.8/site-packages/mujoco/_structs.cpython-38-x86_64-linux-gnu.so): Needed DSO lib/libabsl_throw_delegate.so.2103.0.1 found in conda-forge::abseil-cpp-20210324.2-h9c3ff4c_0
This is the reason why I did not check for that warning on Windows. However, the Windows warning is indeed fishy. By quickly looking at the abseil Windows package, in particular the size of the shipped .lib, at a first glance it seems that a library .lib is shipped, instead of a import library, however I need to check better.
There was a problem hiding this comment.
Ok, I checked a bit the situation. abseil-cpp is still pinned to a version that shipped static libraries on Windows (i.e. before conda-forge/abseil-cpp-feedstock#25), see https://github.com/conda-forge/conda-forge-pinning-feedstock/blob/a7759497a85d3c3a0e929583e5babacc922950c8/recipe/conda_build_config.yaml#L347 . Once an abseil with shared libraries is available, it will be used, and no modifications are required on the mujoco recipe. At this point, I think leaving avoiding to add the ignore_run_exports on the python side is probably the best/simplest thing to do.
There was a problem hiding this comment.
Agreed. If a future version provides non-static linking it's fine.
Co-authored-by: Daniel Ching <[email protected]>
Yes, |
|
CI is happy and I think I address all the review points, the PR is ready for a new review. @conda-forge/help-c-cpp |
github-actions
bot
added
review-requested
and removed
Awaiting author contribution
labels
5 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MuJoCo is a widely used multibody dynamics simulator used in robotics and reinforcement learning. It has been open-sourced on 2022/05/23 : https://github.com/deepmind/mujoco/releases/tag/2.2.0 .
Checklist
url) rather than a repo (e.g.git_url) is used in your recipe (see here for more details).