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

[pydrake] Begin adding typed (i.e., schema-based) YAML loading #18564

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Jan 11, 2023

Towards #18566 (and therefore #17112).


This change is Reviewable

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.

Reviewed 2 of 4 files at r1.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge"

@jwnimmer-tri
Copy link
Collaborator Author

This is ready for feature review, either @SeanCurtis-TRI or @rpoyner-tri?

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: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @jwnimmer-tri)


bindings/pydrake/common/yaml.py line 152 at r2 (raw file):

    fields = getattr(obj, "__fields__", None)
    if fields is not None:
        return dict([

This branch isn't unit tested yet. (Coming in the future). I should either TODO it for unit testing, or yank it until the next PR.

@jwnimmer-tri jwnimmer-tri force-pushed the pydrake-yaml-typed-load branch from c199fdd to 45146ac Compare January 11, 2023 15:12
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: needs platform reviewer assigned, needs at least two assigned reviewers


bindings/pydrake/common/yaml.py line 152 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This branch isn't unit tested yet. (Coming in the future). I should either TODO it for unit testing, or yank it until the next PR.

Done.

@rpoyner-tri rpoyner-tri self-assigned this Jan 11, 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.

+@rpoyner-tri feature.

Reviewable status: LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers

@jwnimmer-tri
Copy link
Collaborator Author

bindings/pydrake/common/yaml.py line 141 at r3 (raw file):

def _enumerate_field_types(obj):
    """Returns a Mapping[str, type] of the schema-based field names and types
    of the given object `obj`. (Note that `obj` is a value, not a type.)

I think this comment might be wrong. I'll work on improving / confirming.

Code quote:

(Note that `obj` is a value, not a type.)

@jwnimmer-tri jwnimmer-tri force-pushed the pydrake-yaml-typed-load branch from 45146ac to 0cb01a5 Compare January 11, 2023 18:58
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: LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers (waiting on @rpoyner-tri)


bindings/pydrake/common/yaml.py line 141 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I think this comment might be wrong. I'll work on improving / confirming.

Done.

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:

Reviewed 1 of 4 files at r1, 4 of 5 files at r2, 1 of 1 files at r4, all commit messages.
Reviewable status: 2 unresolved discussions, needs at least two assigned reviewers (waiting on @jwnimmer-tri)


bindings/pydrake/common/test/yaml_typed_test.py line 283 at r4 (raw file):

    @run_with_multiple_values(_all_typed_read_options())
    def test_read_optional(self, *, options):
        # The test case numberes here (1..12) reference the specification as

typo

Suggestion:

numbers

bindings/pydrake/common/test/yaml_typed_test.py line 395 at r4 (raw file):

class TestYamlTypedReadAccepatance(unittest.TestCase):

typo

Suggestion:

Acceptance

@jwnimmer-tri jwnimmer-tri force-pushed the pydrake-yaml-typed-load branch from 0cb01a5 to 5304ef9 Compare January 11, 2023 21:53
@jwnimmer-tri
Copy link
Collaborator Author

+@xuchenhan-tri for platform review per schedule, please. (Feel free to spill to Eric tomorrow if need be.)

Copy link
Contributor

@xuchenhan-tri xuchenhan-tri left a comment

Choose a reason for hiding this comment

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

Platform :lgtm:

I gave a best-effort review. Feel free to tag a reviewer with more Python experience if in doubt.

Reviewed 1 of 4 files at r1, 3 of 5 files at r2, 2 of 2 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)


bindings/pydrake/common/test/yaml_typed_test.py line 303 at r5 (raw file):

                    else:
                        amended_data = "foo: bar"
                    actual = yaml_load_typed(

nit, typo

Suggestion:

                    actual = yaml_load_typed(
                        schema=schema, data=amended_data, **options)

@jwnimmer-tri
Copy link
Collaborator Author

jwnimmer-tri commented Jan 12, 2023

I gave a best-effort review. Feel free to tag a reviewer with more Python experience if in doubt.

I am going to keep the PR train moving, but \CC @EricCousineau-TRI in case you'd like to give it a once-over. In any case I'll plan to ping you (Eric) once the new subsystem is complete, so you can take a look. It will be similar to the Python schema stuff in Anzu, except that here I don't plan to enforce anything with numpy types yet. (I plan to use numpy.typing to that effect, eventually; just not this quarter.)

@jwnimmer-tri jwnimmer-tri force-pushed the pydrake-yaml-typed-load branch from 5304ef9 to 511bdee Compare January 12, 2023 01:52
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),xuchenhan-tri(platform) (waiting on @xuchenhan-tri)


bindings/pydrake/common/test/yaml_typed_test.py line 303 at r5 (raw file):

Previously, xuchenhan-tri wrote…

nit, typo

Yikes! Thanks, fixed.

Copy link
Contributor

@xuchenhan-tri xuchenhan-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 r6, all commit messages.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),xuchenhan-tri(platform) (waiting on @jwnimmer-tri)

@jwnimmer-tri jwnimmer-tri merged commit 643caa1 into RobotLocomotion:master Jan 12, 2023
@jwnimmer-tri jwnimmer-tri deleted the pydrake-yaml-typed-load branch January 12, 2023 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: feature This pull request contains a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants