[Mujoco parser] Silently ignore some attributes/elements. #22493

RussTedrake · 2025-01-18T20:50:04Z

Now that MuJoCo has space in parsing_doxygen.h where we can list "silently ignored" attributes and elements, this PR goes through and silences those elements that we will never support (and reasonable users would not expected us to support).

This will help cut down a bit on the substantial noise coming out of the parser.

It also adds a few new warnings for new attributes and elements that have managed to spring up in the mujoco documentation.

+@rpoyner-tri for feature review, please.

This change is

rpoyner-tri

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers

multibody/parsing/parsing_doxygen.h line 39 at r1 (raw file):

@subsection mjcf_ignored_silent Tags ignored silently

The following attributes are specific to the MuJoCo solver; we have no plans to support them:

minor: recommend alpha-sorting these long lists.

multibody/parsing/parsing_doxygen.h line 80 at r1 (raw file):

- `/equality/connect/@solimp`

The following elements and attributes will not be supported by the MultibodyPlant parser (the require changes outside of MultibodyPlant):

typo

Suggestion:

they

multibody/parsing/detail_mujoco_parser.cc line 399 at r1 (raw file):

    }
    // Silently ignored: plugin.
    WarnUnsupportedElement(*node, "general");

minor: consider alpha-sorting the long lists of WarnUnsupportedElement() and WarnUnsupportedAttribute()

jwnimmer-tri

Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers

multibody/parsing/parsing_doxygen.h line 80 at r1 (raw file):

- `/equality/connect/@solimp`

The following elements and attributes will not be supported by the MultibodyPlant parser (the require changes outside of MultibodyPlant):

I disagree with silently discarding many of these. The parser should be adding these. It is a shortcoming in our Parser that it does not, so we must warn when we're ignoring them.

In #19155 we started discussions how to parse the similar things in SDFormat. Yes, the parser will need to gain access to a DiagramBuilder in order to implement the feature. There's no reason we can't do that.

Perhaps things like "plugin" are correct to silently ignore, under the same reasoning as the "mujoco solver options" above -- they are not general-purpose elements, but rather specific only to MuJoCo's runtime instead of describing the scene.

However, cameras and lights and sensors are squarely in the "describing the model", not "configuring MuJoCo" and must remain as warning(s).

RussTedrake · 2025-01-22T01:15:35Z

multibody/parsing/parsing_doxygen.h line 80 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I disagree with silently discarding many of these. The parser should be adding these. It is a shortcoming in our Parser that it does not, so we must warn when we're ignoring them.

In #19155 we started discussions how to parse the similar things in SDFormat. Yes, the parser will need to gain access to a DiagramBuilder in order to implement the feature. There's no reason we can't do that.

Perhaps things like "plugin" are correct to silently ignore, under the same reasoning as the "mujoco solver options" above -- they are not general-purpose elements, but rather specific only to MuJoCo's runtime instead of describing the scene.

However, cameras and lights and sensors are squarely in the "describing the model", not "configuring MuJoCo" and must remain as warning(s).

In fact, I'm already working on trying to support all of those options, but was starting to do it with a separate entry point (much like our ApplyCameraConfig, etc). The two designs I was teasing was an ApplyMujocoConfig that tries to do all, vs a preprocessor that kicks out cameraconfig, multibodyplantconfig, etc.

I didn't know about #19155, but it didn't get very far?

It's actually precisely because I was going to support those options elsewhere that I don't want them warning here.

If you do think we should be expanding the entry point of the multibody parsing to take diagrams, i'd be open to that. But that feels like a conceptual change (it's not even a multibody parser any more?) that is bigger than I was initially thinking.

RussTedrake

Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers

multibody/parsing/detail_mujoco_parser.cc line 399 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

minor: consider alpha-sorting the long lists of WarnUnsupportedElement() and WarnUnsupportedAttribute()

They are current sorted by the order they appear in the mujoco docs. That's my preference.

multibody/parsing/parsing_doxygen.h line 39 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

minor: recommend alpha-sorting these long lists.

same as above.

jwnimmer-tri

Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers

multibody/parsing/parsing_doxygen.h line 80 at r1 (raw file):

If you do think we should be expanding the entry point of the multibody parsing to take diagrams, i'd be open to that.

I actually don't know which will be the best design, but I also don't think we need to decide today. If we're going to start loading cameras(?) from MuJoCo files, we should definitely have a design discussion first and play around with some different approaches.

It's actually precisely because I was going to support those options elsewhere that I don't want them warning here.

Okay, that context helps me understand a bit more. Still, I think it seems premature to silence the warnings here, when the other sensor-loading code hasn't been landed yet? It's totally plausible that even if the camera loader is a separate class, it still owns or controls a mulbody::Parser and affirmatively disables or preempts the unknown element errors, so that the parser when used alone without the camera loader would still warn. (For example, the camera loader could chop the cameras out of the XML and then hand that remaining XML to the multibody parser to load.)

The fundamental invariant of our parser is that all input data is either loaded into our model (plant, scene graph, or whatever), or else is covered by a diagnostic.

Breaking that invariant for MuJoCo elements which specify semantically void data like the solver fine-tuning flags is probably the right trade-off, but it doesn't make sense to me why we'd break that invariant for semantically meaningful elements.

rpoyner-tri

Reviewable status: 3 unresolved discussions, needs at least two assigned reviewers

multibody/parsing/parsing_doxygen.h line 39 at r1 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

same as above.

Fair enough. Maybe this section should include a link to relevant mujoco docs, and indicate that the ordering follows that source?

RussTedrake

+@jwnimmer-tri for platform review, please, since you're already here?

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform)

multibody/parsing/parsing_doxygen.h line 39 at r1 (raw file):

Previously, rpoyner-tri (Rick Poyner (rico)) wrote…

Fair enough. Maybe this section should include a link to relevant mujoco docs, and indicate that the ordering follows that source?

Done.

multibody/parsing/parsing_doxygen.h line 80 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

If you do think we should be expanding the entry point of the multibody parsing to take diagrams, i'd be open to that.

I actually don't know which will be the best design, but I also don't think we need to decide today. If we're going to start loading cameras(?) from MuJoCo files, we should definitely have a design discussion first and play around with some different approaches.

It's actually precisely because I was going to support those options elsewhere that I don't want them warning here.

Okay, that context helps me understand a bit more. Still, I think it seems premature to silence the warnings here, when the other sensor-loading code hasn't been landed yet? It's totally plausible that even if the camera loader is a separate class, it still owns or controls a mulbody::Parser and affirmatively disables or preempts the unknown element errors, so that the parser when used alone without the camera loader would still warn. (For example, the camera loader could chop the cameras out of the XML and then hand that remaining XML to the multibody parser to load.)

The fundamental invariant of our parser is that all input data is either loaded into our model (plant, scene graph, or whatever), or else is covered by a diagnostic.

Breaking that invariant for MuJoCo elements which specify semantically void data like the solver fine-tuning flags is probably the right trade-off, but it doesn't make sense to me why we'd break that invariant for semantically meaningful elements.

Done. I've replaced the semantically meaningful ones.

rpoyner-tri

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform)

jwnimmer-tri

Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 4 unresolved discussions

multibody/parsing/parsing_doxygen.h line 62 at r2 (raw file):

  - `@sdf_tolerance`
  - `@sdf_initpoints`
  - <a href=https://mujoco.readthedocs.io/en/stable/XMLreference.html#option-flag>`flag`</a>

It doesn't seem to me like flag as a whole is properly out of scope for Drake? For example, models can use it to disable gravity, which our multibody engine can likewise do. A bunch of the other flags also seem at least plausibly in-scope for Drake's engine.

multibody/parsing/parsing_doxygen.h line 65 at r2 (raw file):

- <a href=https://mujoco.readthedocs.io/en/stable/XMLreference.html#compiler>`/compiler/`</a>
  - `@usethread`
  - `@alignfree`

I don't get this one. This is just a default value for /body/joint(free)@align when the joint says "auto", but we don't disclaim support for align in the first place?

multibody/parsing/parsing_doxygen.h line 78 at r2 (raw file):

  - `@solref`
  - `@solimp`
  - `@margin`

BTW I'm a little bit worried that silently ignoring margin and gap could lead to our simulation of MuJoCo models being very different from MuJoCo's, leading to confused users, but okay with trying it and seeing what happens.

multibody/parsing/detail_mujoco_parser.cc line 522 at r2 (raw file):

    WarnUnsupportedAttribute(*node, "group");
    WarnUnsupportedAttribute(*node, "springdamper");
    WarnUnsupportedAttribute(*node, "solreflimit");

Down below, we have comments like Silently ignored: ... that affirm to future maintainers that we are skipping certain elements/attributes on purpose, but here we don't have any comment for these removed warnings (and similar for a few more groups of changes, below).

It seems to me like affirmative comments in the code that list out the actively-ignore elements/attributes are important to keep this file understandable by future maintainers? Shouldn't we leave breadcrumbs everywhere we've removed warnings?

BTW If you like, the way I would have implemented this feature would have been:

LogIgnoredAttribute("solreflimit");  // Only relevant for MuJoCo solver.
...

where LogIgnoredAttribute writes to log()->debug(...). That has perhaps a small benefit that when we are helping out confused users, we can tell them to turn on debug logging to see what's going on, but mainly it just keeps the parser's spelling more uniform for improved readability / maintainability. For example then the ignored elements can be marked inline in all of the lists, so the order always 1:1 matches the MuJoCo documentation.

RussTedrake · 2025-01-25T19:16:30Z

multibody/parsing/parsing_doxygen.h line 78 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I'm a little bit worried that silently ignoring margin and gap could lead to our simulation of MuJoCo models being very different from MuJoCo's, leading to confused users, but okay with trying it and seeing what happens.

our dynamics are very different. that is inevitable, I think. the cassie model shows this immediately. mujoco is extremely soft with the constraints by default... maybe we could make drake's engine do that but I think we are philosophically different.

RussTedrake

Reviewable status: 3 unresolved discussions

multibody/parsing/detail_mujoco_parser.cc line 522 at r2 (raw file):
Done.

so the order always 1:1 matches the MuJoCo documentation.

fwiw -- i've stopped treating that as a goal. every time i've looked back at the mujoco docs, the attributes and elements have appeared, disappeared, and even been reordered. it's more dynamic of a spec than I had originally understood.

multibody/parsing/parsing_doxygen.h line 62 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It doesn't seem to me like flag as a whole is properly out of scope for Drake? For example, models can use it to disable gravity, which our multibody engine can likewise do. A bunch of the other flags also seem at least plausibly in-scope for Drake's engine.

Done. switched it back to warn.

multibody/parsing/parsing_doxygen.h line 65 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I don't get this one. This is just a default value for /body/joint(free)@align when the joint says "auto", but we don't disclaim support for align in the first place?

Done. switched it back to align. (the tricks they play with inertias for numerical stability are very antithetical to drake, but i guess technically we could support it)

jwnimmer-tri

Reviewed 1 of 2 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion

multibody/parsing/detail_mujoco_parser.cc line 522 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Done.

so the order always 1:1 matches the MuJoCo documentation.

fwiw -- i've stopped treating that as a goal. every time i've looked back at the mujoco docs, the attributes and elements have appeared, disappeared, and even been reordered. it's more dynamic of a spec than I had originally understood.

Maybe I need to take a closer look to find some understanding I've missed, but in the diff from r_base (master) to the latest revision, I still see dozens of WarnUnsupportedAttribute lines that seem to be completely removed, instead of replaced by Log...?

Some of these here, and e.g. "solmix" below, etc.

multibody/parsing/parsing_doxygen.h line 65 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Done. switched it back to align. (the tricks they play with inertias for numerical stability are very antithetical to drake, but i guess technically we could support it)

This is OK -- but maybe just in case I was unclear -- I do think it's okay for us to not implement their tricks, my only concern here was treating @align and @alignfree consistently.

RussTedrake

Reviewable status: 3 unresolved discussions

multibody/parsing/detail_mujoco_parser.cc line 522 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Maybe I need to take a closer look to find some understanding I've missed, but in the diff from r_base (master) to the latest revision, I still see dozens of WarnUnsupportedAttribute lines that seem to be completely removed, instead of replaced by Log...?

Some of these here, and e.g. "solmix" below, etc.

Oops. I missed this bunch. Good catch.

multibody/parsing/parsing_doxygen.h line 65 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This is OK -- but maybe just in case I was unclear -- I do think it's okay for us to not implement their tricks, my only concern here was treating @align and @alignfree consistently.

Yep. I understood that. It forced me to read again and pick between the two.

multibody/parsing/detail_mujoco_parser.cc line 1387 at r3 (raw file):

    WarnUnsupportedAttribute(*node, "noslip_tolerance");
    WarnUnsupportedAttribute(*node, "mpr_iterations");
    WarnUnsupportedAttribute(*node, "mpr_tolerance");

btw -- these mpr_ options have disappeared from their docs

multibody/parsing/detail_mujoco_parser.cc line 1482 at r3 (raw file):

        break;
    }
    WarnUnsupportedAttribute(*node, "exactmeshinertia");

btw -- exactmeshinertia has disappeared from their docs

RussTedrake · 2025-01-25T20:53:32Z

multibody/parsing/detail_mujoco_parser.cc line 522 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

Oops. I missed this bunch. Good catch.

hold one more time for these...

RussTedrake

Reviewable status: 3 unresolved discussions

multibody/parsing/detail_mujoco_parser.cc line 522 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

hold one more time for these...

Done.

jwnimmer-tri

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion

multibody/parsing/detail_mujoco_parser.cc line 1228 at r4 (raw file):

    WarnUnsupportedElement(*node, "site");
    WarnUnsupportedElement(*node, "camera");
    WarnUnsupportedElement(*node, "light");

BTW default/camera and default/light seem to be missing from logging (or at least have explanatory text). Or do we actually implement it and the old warning was spurious?

RussTedrake

Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform)

multibody/parsing/detail_mujoco_parser.cc line 1228 at r4 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW default/camera and default/light seem to be missing from logging (or at least have explanatory text). Or do we actually implement it and the old warning was spurious?

Done. you're right; i forgot to put those back. 🤦

Now that MuJoCo has space in `parsing_doxygen.h` where we can list "silently ignored" attributes and elements, this PR goes through and silences those elements that we will never support (and reasonable users would not expected us to support). This will help cut down a bit on the substantial noise coming out of the parser. It also adds a few new warnings for new attributes and elements that have managed to spring up in the mujoco documentation.

RussTedrake

The CI failure (//tutorials:py/hydroelastic_contact_basics_test) is completely unrelated, i believe.

Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform)

jwnimmer-tri

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),jwnimmer-tri(platform)

rpoyner-tri · 2025-01-27T16:12:14Z

@drake-jenkins-bot linux-jammy-clang-bazel-experimental-everything-release please.

rpoyner-tri

Reviewed 1 of 2 files at r3, 1 of 1 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion

RussTedrake assigned rpoyner-tri Jan 18, 2025

RussTedrake added the release notes: fix This pull request contains fixes (no new features) label Jan 18, 2025

rpoyner-tri reviewed Jan 21, 2025

View reviewed changes

jwnimmer-tri reviewed Jan 21, 2025

View reviewed changes

RussTedrake commented Jan 22, 2025

View reviewed changes

jwnimmer-tri reviewed Jan 22, 2025

View reviewed changes

rpoyner-tri reviewed Jan 22, 2025

View reviewed changes

RussTedrake force-pushed the silent_mujoco branch from 44974c0 to af137f1 Compare January 25, 2025 10:13

RussTedrake assigned jwnimmer-tri Jan 25, 2025

RussTedrake commented Jan 25, 2025

View reviewed changes

rpoyner-tri reviewed Jan 25, 2025

View reviewed changes

jwnimmer-tri reviewed Jan 25, 2025

View reviewed changes

RussTedrake force-pushed the silent_mujoco branch from af137f1 to a692143 Compare January 25, 2025 19:40

RussTedrake commented Jan 25, 2025

View reviewed changes

jwnimmer-tri reviewed Jan 25, 2025

View reviewed changes

RussTedrake force-pushed the silent_mujoco branch from a692143 to 45c1f76 Compare January 25, 2025 20:52

RussTedrake commented Jan 25, 2025

View reviewed changes

RussTedrake force-pushed the silent_mujoco branch from 45c1f76 to 9bebcda Compare January 25, 2025 20:57

RussTedrake commented Jan 25, 2025

View reviewed changes

jwnimmer-tri reviewed Jan 25, 2025

View reviewed changes

RussTedrake commented Jan 25, 2025

View reviewed changes

RussTedrake force-pushed the silent_mujoco branch from 9bebcda to d05eace Compare January 25, 2025 23:35

RussTedrake commented Jan 26, 2025

View reviewed changes

jwnimmer-tri reviewed Jan 26, 2025

View reviewed changes

rpoyner-tri reviewed Jan 27, 2025

View reviewed changes

jwnimmer-tri merged commit e8ae3b9 into RobotLocomotion:master Jan 27, 2025
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Mujoco parser] Silently ignore some attributes/elements. #22493

[Mujoco parser] Silently ignore some attributes/elements. #22493

RussTedrake commented Jan 18, 2025 •

edited by sherm1

Loading

rpoyner-tri left a comment

jwnimmer-tri left a comment

RussTedrake commented Jan 22, 2025

RussTedrake left a comment

jwnimmer-tri left a comment

rpoyner-tri left a comment

RussTedrake left a comment

rpoyner-tri left a comment

jwnimmer-tri left a comment

RussTedrake commented Jan 25, 2025

RussTedrake left a comment

jwnimmer-tri left a comment

RussTedrake left a comment

RussTedrake commented Jan 25, 2025

RussTedrake left a comment

jwnimmer-tri left a comment

RussTedrake left a comment

RussTedrake left a comment

jwnimmer-tri left a comment

rpoyner-tri commented Jan 27, 2025

rpoyner-tri left a comment

[Mujoco parser] Silently ignore some attributes/elements. #22493

[Mujoco parser] Silently ignore some attributes/elements. #22493

Conversation

RussTedrake commented Jan 18, 2025 • edited by sherm1 Loading

rpoyner-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

RussTedrake commented Jan 22, 2025

RussTedrake left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

RussTedrake left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

RussTedrake commented Jan 25, 2025

RussTedrake left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

RussTedrake left a comment

Choose a reason for hiding this comment

RussTedrake commented Jan 25, 2025

RussTedrake left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

RussTedrake left a comment

Choose a reason for hiding this comment

RussTedrake left a comment

Choose a reason for hiding this comment

jwnimmer-tri left a comment

Choose a reason for hiding this comment

rpoyner-tri commented Jan 27, 2025

rpoyner-tri left a comment

Choose a reason for hiding this comment

RussTedrake commented Jan 18, 2025 •

edited by sherm1

Loading