Yaml serialization includes std::filesystem::path and FileSource

[SeanCurtis-TRI]

Serializable structs can now have members that are either std::filesystem::path (or pathlib.Path) or drake::FileSource.

The FileSource support is limited. FileSource is a variant containing either a std::filesystem::path or MemoryFile. If it contains a path (its default configuration), serialization is fine. If it contains a MemoryFile it throws. The throwing behavior will be updated later when we've decided what we're going to do about bytestrings and the !!binary yaml tag.

---

This change is 

[SeanCurtis-TRI]

+(release notes: feature)

+@jwnimmer-tri for review.

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

[jwnimmer-tri]

On the right track, but missing test cases. I suggested some below.

Reviewed 13 of 13 files at r1, all commit messages.
Reviewable status: 8 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

common/yaml/yaml_write_archive.h line 236 at r1 (raw file):

      return;
    }
    auto make_scalar = [](const T& v) {

I would prefer to keep the per-static-type adjustments here more uniformly isolated (ala the float case above). For example, the is_same<path> and is_same<bool> are mutually exclusive branches, but the control flow doesn't reflect that.

Something like this:

if constexpr (std::is_same_v<T, std::filesystem::path>) {
  // We want a simple path: /path/stuff. No extra decorations.
  auto scalar = internal::Node::MakeScalar(value.string());
  root_.Add(nvp.name(), std::move(scalar));
  return;
}

---

common/yaml/test/yaml_write_archive_test.cc line 83 at r1 (raw file):

TEST_F(YamlWriteArchiveTest, Path) {
  const auto test = [](const std::filesystem::path& value,
                       const std::filesystem::path& expected) {

nit Expected should be a typed as a std::string, I think?

---

common/yaml/test/yaml_write_archive_test.cc line 89 at r1 (raw file):

  test("/absolute/path", "/absolute/path");
  test("relative/path", "relative/path");

nit We should also test saving an empty path.

---

common/test/file_source_test.cc line 41 at r1 (raw file):

};

/** The path value gets (de)serialized. */

nit Avoid /** unless Doxygen parses it.

Ditto on the next test case also.

---

common/test/memory_file_test.cc line 114 at r1 (raw file):

}

/** Serialization compiles but throws. */

nit Avoid /** unless Doxygen parses it.

---

common/yaml/test/yaml_read_archive_test.cc line 155 at r1 (raw file):

  EXPECT_EQ(x.value, kNominalDouble);
}

Here we should have new test cases for PathStruct to check in detail any special cases, similar to TEST_P(YamlReadArchiveTest, Double) ... above.

Cases I can think of off the top of my head:

• bare filename (no slashes)
• absolute path
• empty path
• non-lexically-normal path (e.g., /foo//bar).
• path with a double-quote as part of the path
• path that smells like an int (e.g., 1234)
• path that smells like a float (e.g., 1234.567)
• path that smells like a bool (e.g., true)

Likewise, we should have a missing test case along the lines of TEST_P(YamlReadArchiveTest, DoubleMissing) ... above.

---

common/yaml/test/yaml_read_archive_test.cc line 167 at r1 (raw file):

  some_uint64: 105
  some_string: foo
  some_path: "/path/to/nowhere"

This must be a value that's different from a default-constructed AllScalarsStruct, to prove that the loading did something.

The python test used /alternative/path so we should match that here (or change both places to something else).

---

bindings/pydrake/common/test/yaml_typed_test.py line 27 at r1 (raw file):

# define some dataclasses. These classes mimic
#  drake/common/yaml/test/example_structs.h
# and should be roughly kept in sync with the definitions in that file.

We need to add PathStruct as a dataclass, and replicate all of the new C++ test cases into this file.

[SeanCurtis-TRI]

Lots of extra fun to get consistent behavior. Great test cases!

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

common/yaml/test/yaml_read_archive_test.cc line 155 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Here we should have new test cases for PathStruct to check in detail any special cases, similar to TEST_P(YamlReadArchiveTest, Double) ... above.

Cases I can think of off the top of my head:

• bare filename (no slashes)
• absolute path
• empty path
• non-lexically-normal path (e.g., /foo//bar).
• path with a double-quote as part of the path
• path that smells like an int (e.g., 1234)
• path that smells like a float (e.g., 1234.567)
• path that smells like a bool (e.g., true)

Likewise, we should have a missing test case along the lines of TEST_P(YamlReadArchiveTest, DoubleMissing) ... above.

Done

---

bindings/pydrake/common/test/yaml_typed_test.py line 27 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

We need to add PathStruct as a dataclass, and replicate all of the new C++ test cases into this file.

Done.

[SeanCurtis-TRI]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

common/yaml/yaml_write_archive.h line 236 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I would prefer to keep the per-static-type adjustments here more uniformly isolated (ala the float case above). For example, the is_same<path> and is_same<bool> are mutually exclusive branches, but the control flow doesn't reflect that.

Something like this:

if constexpr (std::is_same_v<T, std::filesystem::path>) {
  // We want a simple path: /path/stuff. No extra decorations.
  auto scalar = internal::Node::MakeScalar(value.string());
  root_.Add(nvp.name(), std::move(scalar));
  return;
}

The problem, as it turns out, with this is that it attempts to compile fmt::format("{}", value) for T = std::filesystem::path. That makes mac and noble angry. So, to heighten the mutual exclusivity, I've added an else to get around it.

It's doesn't strengthen the pattern of:

if constexpr () {
  ... 
  return;
}

if constexpr () {
  ...
  return;
}

But at least it compiles everywhere.

[jwnimmer-tri]

I have several thoughts queued up, but for now they are on hold for the one new question below. Depending on how we decide that, my other suggestions will pivot.

Reviewed 6 of 8 files at r2, 1 of 1 files at r3.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

common/yaml/yaml_write_archive.cc line 347 at r2 (raw file):

  }();

  // Empty paths should become ".".

Why is this a requirement / invariant? If the value is actually an empty path, it seems to me like we should serialize it as as such? (I know that Python's pathlib.Path cannot represent an empty path, but that doesn't seem relevant here?)

[SeanCurtis-TRI]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

common/yaml/yaml_write_archive.cc line 347 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Why is this a requirement / invariant? If the value is actually an empty path, it seems to me like we should serialize it as as such? (I know that Python's pathlib.Path cannot represent an empty path, but that doesn't seem relevant here?)

It's because that's what pyyaml does. Not a big reason, but I figured symmetry would be nice.

[jwnimmer-tri]

Got it. I'll work on polishing up my other comments shortly.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

common/yaml/yaml_write_archive.cc line 347 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

It's because that's what pyyaml does. Not a big reason, but I figured symmetry would be nice.

Nothing to do with pyyaml actually. It's only pathlib.Path. If you call its Path("") constructor, it constructs Path(".") as the value up-front. Writing it out with pyyaml doesn't change the value. We can't really match that in C++ (having the actual struct contain a non-empty path), so we shouldn't try.

[SeanCurtis-TRI]

common/yaml/yaml_write_archive.cc line 347 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Nothing to do with pyyaml actually. It's only pathlib.Path. If you call its Path("") constructor, it constructs Path(".") as the value up-front. Writing it out with pyyaml doesn't change the value. We can't really match that in C++ (having the actual struct contain a non-empty path), so we shouldn't try.

Ah, I'd mistakenly conflated the two. I'm happy to undo this particular voodoo.

[jwnimmer-tri]

Reviewed all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

common/yaml/yaml_write_archive.h line 236 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

The problem, as it turns out, with this is that it attempts to compile fmt::format("{}", value) for T = std::filesystem::path. That makes mac and noble angry. So, to heighten the mutual exclusivity, I've added an else to get around it.

It's doesn't strengthen the pattern of:

if constexpr () {
  ... 
  return;
}

if constexpr () {
  ...
  return;
}

But at least it compiles everywhere.

I like it. But then please carry it all the way for uniformity -- make it if constexpr ... else if constexpr ... else ... as a single chain, i.e., join up the fs::pathbranch onto the float branch with an else and nix the return in the float branch. Or more likely, we should just merge #22299 first.

---

common/yaml/yaml_write_archive.cc line 342 at r2 (raw file):

  // We need to know if the path can be interpreted as a primitive (e.g.,
  // the file 1234.5).
  const bool is_primitive = [&path_str]() {

This is not the correct logic. See #22299 for the start of the correct way.

---

common/yaml/yaml_write_archive.h line 224 at r3 (raw file):

  // interpreted as a basic scalar value (e.g., bool, double, etc.) it needs to
  // be conditioned so it doesn't get misinterpreted when read.
  void VisitPathScalar(const char* name, std::filesystem::path value);

On further thought, we don't actually want this to be a path specific handler logic.

Instead, we should treat a fs::path in the struct as-if it were a std::string and use a self-call to handle that. The yaml logic for path and str should be exactly the same, so we should write it that way.

--- a/common/yaml/yaml_write_archive.h
+++ b/common/yaml/yaml_write_archive.h
@@ -236,11 +236,10 @@ class YamlWriteArchive final {
       auto scalar = internal::Node::MakeScalar(std::move(value_str));
       scalar.SetTag(internal::JsonSchemaTag::kFloat);
       root_.Add(nvp.name(), std::move(scalar));
-      return;
-    }
-
-    if constexpr (std::is_same_v<T, std::filesystem::path>) {
-      VisitPathScalar(nvp.name(), std::move(value));
+    } else if constexpr (std::is_same_v<T, std::filesystem::path>) {
+      // We'll treat fs::path exactly like a std::string.
+      std::string path_string = value.string();
+      VisitScalar(MakeNameValue(nvp.name(), &path_string));
     } else {
       auto scalar = internal::Node::MakeScalar(fmt::format("{}", value));
       if constexpr (std::is_same_v<T, bool>) {

This does mean some test cases won't pass anymore, but that's okay -- we can delete (stash) them for now and then circle back with a yaml-string tune up. There is already a TODO for that in some of the yaml tests.

And now that the path implementation would be trivially phrased as in terms of std::string, we don't need to check so many cases for it -- just enough of a sanity check to see that it is actually using the string logic. A single test case with a filename that looks like a float or int would be enough. The test cases we want for yaml fs::path handling should focus on relative path, absolute path, etc. -- not the string encoding details.

Also, I posted #22299 with a start on fixing the edge cases. We could try to merge that first and then revisit this PR.

---

common/yaml/yaml_write_archive.h line 243 at r2 (raw file):

    if constexpr (std::is_same_v<T, std::filesystem::path>) {
      VisitPathScalar(nvp.name(), std::move(value));

BTW The value is const, so a std::move here is a no-op. We should be using pass-by-const-reference for this. But that will probably end up being moot.

[jwnimmer-tri]

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

common/yaml/yaml_write_archive.h line 224 at r3 (raw file):
Let me change my note slightly ...

Instead, we should treat a fs::path in the struct as-if it were a std::string ...

Still true.

... and use a self-call to handle that.

Meh. I suppose the tactic doesn't matter very much. The point is only that fs::path should be handled exactly like a string, so any implementation with does so (and is clear and robust to future changes) will be fine. There are mechanisms other than a self-call that would be okay too.

[SeanCurtis-TRI]

I'll put this on hold while we wait for the string patch.

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

common/yaml/yaml_write_archive.h line 224 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Let me change my note slightly ...

Instead, we should treat a fs::path in the struct as-if it were a std::string ...

Still true.

... and use a self-call to handle that.

Meh. I suppose the tactic doesn't matter very much. The point is only that fs::path should be handled exactly like a string, so any implementation with does so (and is clear and robust to future changes) will be fine. There are mechanisms other than a self-call that would be okay too.

I vaguely recall treating fs::path as a string on my first pass and I didn't like what got emitted. I no longer exactly remember what that was, so I'll defer that concern to see if it rears its head again. On the other hand, I've got a lot more time reading yaml and yaml code since that first attempt, and I may now have different expectations.

---

common/yaml/yaml_write_archive.cc line 342 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

This is not the correct logic. See #22299 for the start of the correct way.

In what way is this "wrong"? Is it simply an expensive mechanism? Or would it actually produce the wrong results?

This relies on yaml_cpp's interpretation of yaml documentation. Whereas your solution relies on Drake explicitly interpreting yaml documentation. Furthermore, given that we use yaml_cpp to do the parsing, any disagreement between implementation of the regular expressions could lead to unexpected behavior.

[jwnimmer-tri]

Reviewable status: 4 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers

---

common/yaml/yaml_write_archive.cc line 342 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

In what way is this "wrong"? Is it simply an expensive mechanism? Or would it actually produce the wrong results?

This relies on yaml_cpp's interpretation of yaml documentation. Whereas your solution relies on Drake explicitly interpreting yaml documentation. Furthermore, given that we use yaml_cpp to do the parsing, any disagreement between implementation of the regular expressions could lead to unexpected behavior.

Well for one, this doesn't account for nulls.

I was also skeptical whether it would flag very large integers or very large/small reals as needing quoting. What does the yaml-cpp parser do with scalars that would overflow the requested C++ datatype? From a quick skim, it looks like ConvertStreamTo would return false if the scalar exceeds numeric_limits, and thus we would not quote.

I also didn't like this because actually parsing the string seems unnecessarily expensive. The minimum thing we need is just to check its shape, not actually deserialize the value.

[SeanCurtis-TRI]

Rebased and updated on #22299

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 @SeanCurtis-TRI)

---

common/yaml/yaml_write_archive.h line 236 at r1 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

I like it. But then please carry it all the way for uniformity -- make it if constexpr ... else if constexpr ... else ... as a single chain, i.e., join up the fs::pathbranch onto the float branch with an else and nix the return in the float branch. Or more likely, we should just merge #22299 first.

Done

---

common/yaml/yaml_write_archive.h line 224 at r3 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I vaguely recall treating fs::path as a string on my first pass and I didn't like what got emitted. I no longer exactly remember what that was, so I'll defer that concern to see if it rears its head again. On the other hand, I've got a lot more time reading yaml and yaml code since that first attempt, and I may now have different expectations.

I went ahead with a more literal string incantation.

---

common/yaml/yaml_write_archive.cc line 342 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Well for one, this doesn't account for nulls.

I was also skeptical whether it would flag very large integers or very large/small reals as needing quoting. What does the yaml-cpp parser do with scalars that would overflow the requested C++ datatype? From a quick skim, it looks like ConvertStreamTo would return false if the scalar exceeds numeric_limits, and thus we would not quote.

I also didn't like this because actually parsing the string seems unnecessarily expensive. The minimum thing we need is just to check its shape, not actually deserialize the value.

Thanks

---

common/yaml/yaml_write_archive.cc line 347 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Ah, I'd mistakenly conflated the two. I'm happy to undo this particular voodoo.

Done

[jwnimmer-tri]

Looks pretty good. I'll wait for the dust to settle on master before finalizing my review.

Reviewed 3 of 5 files at r4, all commit messages.
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 @SeanCurtis-TRI)

---

bindings/pydrake/common/yaml.py line 321 at r4 (raw file):

    # Handle pathlib.Path.
    if value_schema == Path:
        new_value = Path(yaml_value)

Here might be an unintentional difference from C++. We need to decide, if the user gives us float like 321.100 (either without a type tag, or with !!float 321.100) is that valid or invalid when the schema wants a path?

I think the sunny-day parsing matches between C++ and Python, but we probably want the rainy-day error conditions to match, if we can.

Though writing this out now, I suppose the problem is actually on the C++ side, so maybe this is fine for now.

Perhaps what we could do at least is add a test case for path: 321.100 to both C++ and Python, to capture the current behavior. If we don't like the current situation, we could add a TODO to the test case(s) remarking on it.

---

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

    some_int: int = 11
    some_str: str = "nominal_string"
    some_path: Path = "/path/to/nowhere"

BTW If this field declaration were alpha-sorted (between int and str), I think the test case and output would be ever so slightly easier to read.

---

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

    def test_read_path(self, *, options):
        cases = [
            ("no_directory.txt", None),

I can appreciate the desire not to copy-paste the paths which don't end up transforming during the parse (both for simplicity reasons, and perhaps as a way to highlight the "this is unchanged" facet of that test pair).

However, I think it is too obscure. Yaml test cases are sometimes extraordinarily hard to grok (and with Optional[] we have None taking on a real meaning). I think its important to stick with very boring input-output literal pairs as test points, avoiding extra for-loop magic as much as possible. We should spell out ("no_directory.txt", "no_directory.txt") literally to help out people reading this.

If we want to call attention to no-op pairs (and I do support that idea), we could use comment groupings to do that, like this:

# These plain strings are unchanged by parsing them into a Path.
(".", "."),
("no_directory.txt", "no_directory.txt"),
("/quoted\"/path", "/quoted\"/path"),
# These end up parsing differently, to a more or less degree.
("''", "."),
...

[jwnimmer-tri]

Reviewable status: 4 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 @SeanCurtis-TRI)

---

common/yaml/yaml_write_archive.h line 251 at r4 (raw file):

      tag = internal::JsonSchemaTag::kStr;
    }
    auto scalar = internal::Node::MakeScalar(std::move(text));

BTW transferred from a prior PR -- please switch this to internal::Node scalar = ... (spell out the auto).

[SeanCurtis-TRI]

Updated.

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

[jwnimmer-tri]

feature.

Reviewed 1 of 5 files at r4, 4 of 4 files at r5, all commit messages.
Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers

---

common/yaml/test/yaml_read_archive_test.cc line 172 at r5 (raw file):

TEST_P(YamlReadArchiveTest, Path) {
  const auto test_valid = [](const std::string& value,
                             const std::optional<std::string>& maybe_expected =

nit I think for utmost clarity, the type of maybe_expected should be std::optional<fs::path>, since that's what x.value actually is.

If that seems a bridge too far, that at least we should do EXPECT_EQ(x.value.string(), expected) to clarify the types.

---

common/yaml/test/yaml_read_archive_test.cc line 179 at r5 (raw file):

  };

  test_valid("no_directory.txt");

BTW I try to keep the C++ and Python test cases in exactly the same order, to make syncing them up easier, so ideally we should do that here also and probably add the "these are the same" and "these are changed" comments likewise.

The point I made in the Python tests ("I can appreciate the desire not to copy-paste the paths ...") also holds here, though C++ is messier so I won't strictly block on it. But I imagine that writing out the literals twice and not using optional::value_or is probably still a win.

---

common/yaml/test/yaml_read_archive_test.cc line 230 at r5 (raw file):

  some_uint64: 105
  some_string: foo
  some_path: "/alternative/path"

BTW The double quotes here are not strictly required. Possibly it is more clear to leave them off. (The write test doesn't emit them.)

---

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

            ("/absolute/path/file.txt", "/absolute/path/file.txt"),
            ("/quoted\"/path", "/quoted\"/path"),
            # These string end up changing to a greater or lesser degree.

typo

Suggestion:

strings

[SeanCurtis-TRI]

+@sammy-tri for platform review, come Monday.

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

---

common/yaml/test/yaml_read_archive_test.cc line 179 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

BTW I try to keep the C++ and Python test cases in exactly the same order, to make syncing them up easier, so ideally we should do that here also and probably add the "these are the same" and "these are changed" comments likewise.

The point I made in the Python tests ("I can appreciate the desire not to copy-paste the paths ...") also holds here, though C++ is messier so I won't strictly block on it. But I imagine that writing out the literals twice and not using optional::value_or is probably still a win.

Turns out it revealed a bug. :)

path == path comparison is pretty robust. A normalized path will test equal to a non-normalized path as long as they normalize to the same thing.

However, simply asking for the string exposed that it isn't, in fact, normalized. So, changing the test spelling (to clearly comparing strings) revealed that the test didn't actually show what it was intended to show.

[jwnimmer-tri]

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

---

common/yaml/yaml_read_archive.cc line 271 at r6 (raw file):

                                  std::filesystem::path* result) {
  DRAKE_DEMAND(result != nullptr);
  *result = std::filesystem::path(value).lexically_normal();

What is the thought process here? Trying to match what the python Path() constructor does? In any case we need a thorough comment explaining it to make it clear for future maintainers.