[yaml] Add support for variant default type promotion

[jwnimmer-tri]

Closes #24064.

---

This change is 

[jwnimmer-tri]

@dmcconachie-lbm could you check if this solves your problem, and if you believe the new logic as best you can tell?

[dmcconachie-lbm]

bindings/pydrake/common/yaml.py line 476 at r1 (raw file):

        yaml_value_type = type(yaml_value)
        if yaml_value_type in list(_PRIMITIVE_YAML_TYPES) + [type(None)]:
            # Check if the scalar is one of the allowed union types.

We still have problems if the promoted type is not first in the union. For example

@dc.dataclass
class ExampleClass
    some_path: FloatStruct | Path = ...

even with !!str in the yaml file; this is where I paused yesterday.

In my particular use case I can put the promotable type up front, but in general I don't think it's solved. I have a thing for 3 hours but I'll send a reproducer in a bit.

[dmcconachie-lbm]

bindings/pydrake/common/yaml.py line 476 at r1 (raw file):

Previously, dmcconachie-lbm (Dale McConachie) wrote…

We still have problems if the promoted type is not first in the union. For example

@dc.dataclass
class ExampleClass
    some_path: FloatStruct | Path = ...

even with !!str in the yaml file; this is where I paused yesterday.

In my particular use case I can put the promotable type up front, but in general I don't think it's solved. I have a thing for 3 hours but I'll send a reproducer in a bit.

It's possible that we don't want to support the above pattern and the bug fix is one of documentation.

[jwnimmer-tri]

@jwnimmer-tri reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri).

---

bindings/pydrake/common/yaml.py line 476 at r1 (raw file):

Previously, dmcconachie-lbm (Dale McConachie) wrote…

It's possible that we don't want to support the above pattern and the bug fix is one of documentation.

The typing rule is that if the type you want to end up with isn't the first one in the variant, then you must use a tag to specify it during parsing. The !!str tag doesn't help since that's not the type you want.

To load a path as a non-initial variant option, you'll probably need to wrap it into a sub struct so the yaml has my_variant_field: !Path { path: /foo }.

[dmcconachie-lbm]

bindings/pydrake/common/yaml.py line 476 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The typing rule is that if the type you want to end up with isn't the first one in the variant, then you must use a tag to specify it during parsing. The !!str tag doesn't help since that's not the type you want.

To load a path as a non-initial variant option, you'll probably need to wrap it into a sub struct so the yaml has my_variant_field: !Path { path: /foo }.

It rejects !Path; "wasn't expecting a mapping" - and elsewhere in this test to get Path we have to spell it !!str.

[dmcconachie-lbm]

bindings/pydrake/common/yaml.py line 476 at r1 (raw file):

Previously, dmcconachie-lbm (Dale McConachie) wrote…

It rejects !Path; "wasn't expecting a mapping" - and elsewhere in this test to get Path we have to spell it !!str.

Ah; as you; say; yes; sub-struct.

The alternative would be to make the auto-promotion work for any promotable anywhere in the union, not just generic_args[0].

[jwnimmer-tri]

@jwnimmer-tri made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri).

---

bindings/pydrake/common/yaml.py line 476 at r1 (raw file):

Previously, dmcconachie-lbm (Dale McConachie) wrote…

Ah; as you; say; yes; sub-struct.

The alternative would be to make the auto-promotion work for any promotable anywhere in the union, not just generic_args[0].

General promotion is difficult (we need to make C++ and Python both accomplish it). However, possibly we could special case Path and std::filesystem::path to tag-match !!str since we already treats paths as special in a bunch of the code.

[jwnimmer-tri]

@jwnimmer-tri made 1 comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri).

---

bindings/pydrake/common/yaml.py line 476 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

General promotion is difficult (we need to make C++ and Python both accomplish it). However, possibly we could special case Path and std::filesystem::path to tag-match !!str since we already treats paths as special in a bunch of the code.

I tried implementing this but it gets pretty complicated and magical, fast. The only syntax we could support is my_variant_field: !!str /path/to/somewhere with the exact tag spelling !!str. A quoted string like my_variant_field: "/path/to/somewhere" doesn't work (in C++) and of course neither does a non-quoted string literal. I think the answer will need to be a substruct. WDYT about landing this PR as-is?

[dmcconachie-lbm]

@dmcconachie-lbm reviewed 4 files and made 4 comments.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri).

---

bindings/pydrake/common/yaml.py line 476 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I tried implementing this but it gets pretty complicated and magical, fast. The only syntax we could support is my_variant_field: !!str /path/to/somewhere with the exact tag spelling !!str. A quoted string like my_variant_field: "/path/to/somewhere" doesn't work (in C++) and of course neither does a non-quoted string literal. I think the answer will need to be a substruct. WDYT about landing this PR as-is?

That's the conclusion I've been working my way around to. As is; this PR is to the good; so it should land. And the things you ran into are the same reason I didn't have a PR up myself a couple of days ago.

Maybe we "fix" with docs - if you want auto promotion it has to be the first entry in the union/variant - "If you want something else, try not wanting it if reasonable."

Path and np.ndarray are I think the 2 that might have trouble as they have are bespoke promoters for non-union non-primitive non-dataclass/schema types that we support anyway that then might need special handling if they are not the first element in the union. I.e.

@dc.dataclass:
class MyNotQuiteAnNpArray
    i_really_want_some_other_type_as_the_first_type_but_if_you_really_want_ndarray_then_fine: SomeTypeThatIsntAList | np.ndarray

I can't think of a use case where we would actually USE that spelling however; pretty sure we could respell and put np.ndarray first and get the behaviour that we want.

TLDR: yes; land as is with possibly a docs update.

---

bindings/pydrake/common/yaml.py line 499 at r1 (raw file):

                setter(default_typed_value)
                return
            # The yaml_value didn't match any allowed Union type.

nit: I think the comment is subtly wrong per the above discussion.

Suggestion:

# The yaml_value didn't match any allowed Union type nor could it be 
# promoted to the first type in the Union.

---

bindings/pydrake/common/test/yaml_typed_test.py line 850 at r2 (raw file):

                self.assertEqual(x.truthy_type, True)
                self.assertIsInstance(x.truthy_type, bool)

minor: Can we add a test where the defaults of the PromotionVariantStruct are the second type?

@dc.dataclass
class PromotionVariantTypeSwitchStruct
    float_type: float | FloatStruct = dc.field(default_factory=FloatStruct)
    np_type: np.ndarray | FloatStruct = dc.field(default_factory=FloatStruct)
    path_type: Path | FloatStruct = dc.field(default_factory=FloatStruct)
    truthy_type: bool | FloatStruct = dc.field(default_factory=FloatStruct)

This is our actual use case:

@dc.dataclass
class SomeSchema
    log_folder: Path | AutoLogFolder = dc.field(default_factory=AutoLogFolder)
    ...

I.e.; if you give me an absolute/relative Path I'll use it; otherwise I'll do things like lookup if I'm running under bazel test, etc. It reads better and is more clear than other sentinels like Path | None which could easily imply "don't log" if log_folder is None.

99% sure it all works still with your version but I have not tested directly.

[dmcconachie-lbm]

@dmcconachie-lbm resolved 1 discussion.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri).

[jwnimmer-tri]

+a:@rpoyner-tri for platform review per schedule, please.

@jwnimmer-tri reviewed 4 files and all commit messages, made 3 comments, and resolved 2 discussions.
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),rpoyner-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri).

---

bindings/pydrake/common/yaml.py line 476 at r1 (raw file):

Previously, dmcconachie-lbm (Dale McConachie) wrote…

That's the conclusion I've been working my way around to. As is; this PR is to the good; so it should land. And the things you ran into are the same reason I didn't have a PR up myself a couple of days ago.

Maybe we "fix" with docs - if you want auto promotion it has to be the first entry in the union/variant - "If you want something else, try not wanting it if reasonable."

Path and np.ndarray are I think the 2 that might have trouble as they have are bespoke promoters for non-union non-primitive non-dataclass/schema types that we support anyway that then might need special handling if they are not the first element in the union. I.e.

@dc.dataclass:
class MyNotQuiteAnNpArray
    i_really_want_some_other_type_as_the_first_type_but_if_you_really_want_ndarray_then_fine: SomeTypeThatIsntAList | np.ndarray

I can't think of a use case where we would actually USE that spelling however; pretty sure we could respell and put np.ndarray first and get the behaviour that we want.

TLDR: yes; land as is with possibly a docs update.

Added docs.

---

bindings/pydrake/common/test/yaml_typed_test.py line 850 at r2 (raw file):

Previously, dmcconachie-lbm (Dale McConachie) wrote…

minor: Can we add a test where the defaults of the PromotionVariantStruct are the second type?

@dc.dataclass
class PromotionVariantTypeSwitchStruct
    float_type: float | FloatStruct = dc.field(default_factory=FloatStruct)
    np_type: np.ndarray | FloatStruct = dc.field(default_factory=FloatStruct)
    path_type: Path | FloatStruct = dc.field(default_factory=FloatStruct)
    truthy_type: bool | FloatStruct = dc.field(default_factory=FloatStruct)

This is our actual use case:

@dc.dataclass
class SomeSchema
    log_folder: Path | AutoLogFolder = dc.field(default_factory=AutoLogFolder)
    ...

I.e.; if you give me an absolute/relative Path I'll use it; otherwise I'll do things like lookup if I'm running under bazel test, etc. It reads better and is more clear than other sentinels like Path | None which could easily imply "don't log" if log_folder is None.

99% sure it all works still with your version but I have not tested directly.

Done (for both C++ and Python).

[dmcconachie-lbm]

@dmcconachie-lbm reviewed 4 files and all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri).

[rpoyner-tri]

@rpoyner-tri reviewed 5 files and all commit messages, and made 2 comments.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri).

---

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

            # the first type in the Union) when no type tag has been given. If
            # the default type is a primitive, we must be careful to use the
            # safe converion routine.

typo

Suggestion:

conversion

[jwnimmer-tri]

@jwnimmer-tri reviewed 1 file and all commit messages, and resolved 1 discussion.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri).

[rpoyner-tri]

@rpoyner-tri reviewed 1 file and all commit messages.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @jwnimmer-tri).