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

[workspace] Add drake_models regression test #19986

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Aug 12, 2023

Required for #13942. Related to #17061.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri force-pushed the external-models-parse-test branch from 0f39588 to 1f165bf Compare August 14, 2023 03:29
@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review August 14, 2023 03:29
@jwnimmer-tri
Copy link
Collaborator Author

+@rpoyner-tri for feature review, please.

@jwnimmer-tri jwnimmer-tri added the release notes: none This pull request should not be mentioned in the release notes label Aug 14, 2023
Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

I see you've noted elsewhere that this strategy requires PRs on two repos to get a model tested. It's fine as long as we're aware -- we can always revisit that.

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


tools/workspace/drake_models/test/parse_test.py line 21 at r1 (raw file):

    @staticmethod
    def inventory():

BTW consider annotations to indicate this is a generator: https://stackoverflow.com/questions/27264250/how-to-annotate-a-generator-in-python3


tools/workspace/drake_models/test/parse_test.py line 33 at r1 (raw file):

    @staticmethod
    def get_all_models():

BTW consider annotations to indicate this is a generator: https://stackoverflow.com/questions/27264250/how-to-annotate-a-generator-in-python3

Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

+@ggould-tri for platform review per schedule, please.

Reviewable status: LGTM missing from assignee ggould-tri(platform)

Copy link
Contributor

@rpoyner-tri rpoyner-tri left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

:lgtm:

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


tools/workspace/drake_models/test/parse_test.py line 29 at r2 (raw file):

        result = Path(manifest.Rlocation(
            "drake/tools/workspace/drake_models/inventory.txt"))
        with open(result, encoding="utf-8") as f:

nit: "result" is a generic name; consider a descriptive one.

Suggestion:

        inventory_path = Path(manifest.Rlocation(
            "drake/tools/workspace/drake_models/inventory.txt"))
        with open(inventory_path, encoding="utf-8") as f:

tools/workspace/drake_models/test/parse_test.py line 48 at r2 (raw file):

                # We can't tell a MuJoCo file just by its suffix.
                runfiles = TestDrakeModels.package_xml().parent.parent
                if "<mujoco" in (runfiles / path).open().read():

BTW: "What the Zalgo!?!" I exclaimed upon reading this. Which caused me to look up mujoco files and... oh. Oh.
They don't even have a DTD or any processing directives or... :hangs head: Well, Zalgo away I guess.

Code quote:

                if "<mujoco" in (runfiles / path).open().read():

tools/workspace/drake_models/test/parse_test.py line 58 at r2 (raw file):

        all_models = list(TestDrakeModels.get_all_models())

        # Allow warnings on these models until they are repaired.

BTW, here and below, is/are there issue/s open for these files that could be cited in these comments?

@jwnimmer-tri jwnimmer-tri force-pushed the external-models-parse-test branch from 17f42be to cb0babc Compare August 14, 2023 18:37
Copy link
Collaborator Author

@jwnimmer-tri jwnimmer-tri left a comment

Choose a reason for hiding this comment

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

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


tools/workspace/drake_models/test/parse_test.py line 48 at r2 (raw file):

Previously, ggould-tri wrote…

BTW: "What the Zalgo!?!" I exclaimed upon reading this. Which caused me to look up mujoco files and... oh. Oh.
They don't even have a DTD or any processing directives or... :hangs head: Well, Zalgo away I guess.

Yup. The "We can't tell ..." comment was my attempt to be civil. In my head, there was a lot more cursing.


tools/workspace/drake_models/test/parse_test.py line 58 at r2 (raw file):

Previously, ggould-tri wrote…

BTW, here and below, is/are there issue/s open for these files that could be cited in these comments?

I'm not sure that any action is required, but anyway filed #19992 to discuss.

Copy link
Contributor

@ggould-tri ggould-tri left a comment

Choose a reason for hiding this comment

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

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

@jwnimmer-tri jwnimmer-tri merged commit df1a344 into RobotLocomotion:master Aug 14, 2023
@jwnimmer-tri jwnimmer-tri deleted the external-models-parse-test branch August 14, 2023 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: none This pull request should not be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants