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

Use Clang to build MJX #534

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

Use Clang to build MJX #534

wants to merge 8 commits into from

Conversation

DwarKapex
Copy link
Contributor

@DwarKapex DwarKapex commented Feb 13, 2024

I got a compilation issue while compiling mujoco-mpc with GNU c++ compiler:

#10 912.6         /opt/mujoco-mpc/mjpc/../mjpc/spline/spline.h:106:9: error: expected unqualified-id before ‘const’
#10 912.6           106 |         const IteratorT<SplineType, NodeType>& other) = default;
#10 912.6               |         ^~~~~
#10 912.6         /opt/mujoco-mpc/mjpc/../mjpc/spline/spline.h:105:37: error: expected ‘)’ before ‘const’
#10 912.6           105 |     IteratorT<SplineType, NodeType>(
#10 912.6               |                                    ~^
#10 912.6               |                                     )
#10 912.6           106 |         const IteratorT<SplineType, NodeType>& other) = default;
#10 912.6               |         ~~~~~
#10 912.6         /opt/mujoco-mpc/mjpc/../mjpc/spline/spline.h:109:68: error: expected ‘)’ before ‘&&’ token
#10 912.6           109 |     IteratorT<SplineType, NodeType>(IteratorT<SplineType, NodeType>&& other) =
#10 912.6               |                                    ~                               ^~
#10 912.6               |                                                                    )

The developers of mujoco-mpc use Clang to compile (https://github.com/google-deepmind/mujoco_mpc/blob/main/.github/workflows/build.yml#L19).
From now on, the default compiler for MJX is Clang, but can be changed via docker arguments.

@DwarKapex DwarKapex requested review from yhtang and nouiz February 13, 2024 04:21
@DwarKapex DwarKapex self-assigned this Feb 13, 2024
ARG CC
ARG CXX

ENV CC=${CC}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of enforcing the compiler toolchain globally, can we set it only for the MPC installation step?

@yhtang yhtang marked this pull request as draft February 13, 2024 06:11
RUN CC=${CC_COMPILER} CXX=${CXX_COMPILER} pip-finalize.sh /opt/pip-tools.d/requirements-mpc.in

# compiler the rest dependencies
RUN pip-finalize.sh /opt/pip-tools.d/requirements-mjx.in /opt/pip-tools.d/requirements-l2r.in
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to run pip-finalize twice here? Due to the use of pip-sync in the script, the second run will erase what the first run installed.

@yhtang
Copy link
Collaborator

yhtang commented Feb 15, 2024

The plan coming out of an offline discussion between @DwarKapex and me is to use multi-stage build to create the MPC wheel using clang and then bring it into the mealkit/final image to avoid interfering with the default compiler toolchain. This is similar to how we build the jaxlib wheel in the JAX core image and also how we build the tensorflow-text/lingvo packages in the T5X/Pax arm64 images.

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