[parsing] Add support for remote packages

[jwnimmer-tri]

Towards #9498. (It solves the feature request, but we need more docs before calling the issue closed.)

Towards #15774.

---

Manual testing for Ubuntu CMake:

$ bazel run //:install -- /home/jwnimmer/tmp/install
$ cd ~/tmp
$ env PYTHONPATH=install/lib/python3.10/site-packages python3
>>> from pydrake.all import *
>>> params = PackageMap.RemoteParams(
...     urls=['https://github.com/RobotLocomotion/models/archive/refs/tags/v0.0.20220714.zip'],
...     sha256='e19414d1e7e8ae4e3de610cb2456332bb3be79faeb0f6a8c1db8a2a5029eddb8',
...     strip_prefix='models-0.0.20220714')
>>> map = PackageMap()
>>> map.AddRemote("old_models", params)
>>> old = map.GetPath("old_models")
>>> with open(f"{old}/README.md") as f: f.read()
... 
'# models\n\nShareable large model files (meshes, etc) for use in RobotLocomotion Group\nresearch projects, including [Drake](https://drake.mit.edu/).\n\nPlease note that the files contained in the repository may have related files,\nsuch as URDF or SDF files, in the Drake repository.\n\n**NOTE**: As of 2019-01-29, the branch `old_master` contains the original\nmodels for the Fall 2018 session of\n[MIT 6.881](http://manipulation.csail.mit.edu/). This branch may soon be\nremoved.\n'

---

Manual testing for Ubuntu wheel:

$ cd tmp
$ python3 -m venv env && pip install drake-0.0.0-cp310-cp310-manylinux_2_31_x86_64.whl
$ env/bin/python3
>>> from pydrake.all import *
>>> map = PackageMap()
>>> params = PackageMap.RemoteParams(
    urls=['https://github.com/RobotLocomotion/models/archive/refs/tags/v0.0.20220714.zip'],
    sha256='e19414d1e7e8ae4e3de610cb2456332bb3be79faeb0f6a8c1db8a2a5029eddb8',
    strip_prefix='models-0.0.20220714')
>>> map.AddRemote("old_models", params)
>>> old = map.GetPath("old_models")
>>> with open(f"{old}/README.md") as f: f.read()
... 
'# models\n\nShareable large model files (meshes, etc) for use in RobotLocomotion Group\nresearch projects, including [Drake](https://drake.mit.edu/).\n\nPlease note that the files contained in the repository may have related files,\nsuch as URDF or SDF files, in the Drake repository.\n\n**NOTE**: As of 2019-01-29, the branch `old_master` contains the original\nmodels for the Fall 2018 session of\n[MIT 6.881](http://manipulation.csail.mit.edu/). This branch may soon be\nremoved.\n'

---

Manual testing for macOS Wheel (pending):

% which python3
/opt/homebrew/bin/python3
% python3 -m venv env && env/bin/pip install https://drake-packages.csail.mit.edu/drake/experimental/drake-0.0.2023.3.20.19.9.52%2Bgit90394d48-cp311-cp311-macosx_12_0_arm64.whl
% env/bin/python3
>>> from pydrake.all import *
>>> params = PackageMap.RemoteParams(
...     urls=['https://github.com/RobotLocomotion/models/archive/refs/tags/v0.0.20220714.zip'],
...     sha256='e19414d1e7e8ae4e3de610cb2456332bb3be79faeb0f6a8c1db8a2a5029eddb8',
...     strip_prefix='models-0.0.20220714')
>>> map = PackageMap()
>>> map.AddRemote("old_models", params)
>>> old = map.GetPath("old_models")
>>> with open(f"{old}/README.md") as f: f.read()
... 
'# models\n\nShareable large model files (meshes, etc) for use in RobotLocomotion Group\nresearch projects, including [Drake](https://drake.mit.edu/).\n\nPlease note that the files contained in the repository may have related files,\nsuch as URDF or SDF files, in the Drake repository.\n\n**NOTE**: As of 2019-01-29, the branch `old_master` contains the original\nmodels for the Fall 2018 session of\n[MIT 6.881](http://manipulation.csail.mit.edu/). This branch may soon be\nremoved.\n'

---

This change is 

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge" (waiting on @jwnimmer-tri)

---

multibody/parsing/package_map.cc line 222 at r3 (raw file):

  // If this is a remote package and we haven't fetched it yet, do that now.
  if (!package_data.is_fetched()) {
    FetchContent(package_name, const_cast<PackageData*>(&package_data));

Working

This is the place that's not threadsafe.

[jwnimmer-tri]

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)

---

multibody/parsing/package_map.cc line 222 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

This is the place that's not threadsafe.

Done (in theory). Probably deserves more thorough testing.

[jwnimmer-tri]

@drake-jenkins-bot mac-arm-monterey-clang-bazel-experimental-release please

[jwnimmer-tri]

Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @jwnimmer-tri)

a discussion (no related file):
Working

macOS sanity check:

https://drake-jenkins.csail.mit.edu/view/Mac%20ARM/job/mac-arm-monterey-clang-bazel-experimental-release/89/console

---

a discussion (no related file):
Working

Explain how I've manually tested this.
Including wheel testing.

---

[rpoyner-tri]

Reviewed 1 of 2 files at r9, 1 of 1 files at r10, 4 of 4 files at r11, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri and @jwnimmer-tri)

[rpoyner-tri]

Reviewed 3 of 3 files at r12, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri and @jwnimmer-tri)

[SeanCurtis-TRI]

Reviewed 3 of 3 files at r12, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri and @jwnimmer-tri)

[jwnimmer-tri]

Your ubuntu CMake instructions seem to be broken.

Thanks, I've edited in the correct path now.

Reviewable status: 2 unresolved discussions, LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri and @SeanCurtis-TRI)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

@drake-jenkins-bot linux-focal-clang-bazel-experimental-valgrind-memcheck please

All good now.

---

---

multibody/parsing/package_map.h line 117 at r6 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This looks good to me. The purpose and meaning is now clear. Thanks.

While I'm still not in love with the parameter name, the documentation does all the necessary heavy lifting.

One final thought: bazilisms might not be the right language for this kind of API. It's a public C++ API for roboticists based on a ROS concept. Is that a bazel-centric audience?

I don't know of any ROS conventions that contradict this, but I'm also no expert.

Things like tar have a "strip components" flag which is like this except a count of directories instead of their names, and "prefix" is a pretty canonical term itself.

(Honestly I think Python is in the wrong here. "Remove if you see it" is trim, "Remove a thing that is already there" is strip. Python's use of "strip" to mean "trim" annoys me.)

---

multibody/parsing/package_map.cc line 492 at r6 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

It's probably representative of our documenting styles. You make true, accurate, and, above all, succinct statements with strongly intended implications; implications left as an exercise for the reader. I tend to be explicit so that implications don't have to be inferred over and over.

Concrete example: documentation of RemoteParams::urls:

    /** The list of remote URLs for this resource. The urls are used in the
    other they appear here, so preferred mirror(s) should come first. Valid
    methods are "http://" or "https://" or "file://". */

There is nothing about a "valid" RemoteParams containing a non-empty list of urls. Certainly, a meaningful instance would have a non-empty list. But "meaningful" is not necessarily the same as "valid". Furthermore, AddRemote() doesn't suggest that it will validate and/or throw.

This is representative of what I mean when i say that the validation and throwing in this function doesn't seem to be supported by any of the documentation. In the original post, you've given some guidance to "valid" (e.g., you've specified that when a URL is present, it must have a proper prefix of http://, etc.)

I don't think telling people "you must give me at least 1 url if you want me to download something" is worth the paper it's printed on. It forces people to wade through extra text with no benefit to them (or us).

If there are other examples of caveats that are missing or too-brief in the docs let me know, but I don't think "empty list of urls" is one of them.

---

multibody/parsing/package_map.cc line 210 at r7 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

So, if someone has redirected spdlog, they'll get the C++-side error messages but not the package_downloader.py messages? (That would be regrettable as that program only has important error messages.)

Correct.

I also agree it's regrettable -- but not regrettable enough to try to capture stderr of a subprocess. At some point, we could enhance the downloader to write errors to a text file, and for the map to read back that file. A journey for another day.

---

multibody/parsing/test/package_map_remote_test.cc line 204 at r10 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

This test's purpose/methodology isn't quite clear to me.

I'm not sure what is being shown. I believe that the significance of an already-fetched package being merged is that the already-fetched status should be preserved. I'm not seeing how the test shows that.

Alternatively, it could simply be showing that regardless of what happens to the internal bits on the remote data, calling GetPath() on the new PackageMap doesn't cause problems (maybe it downloads redundantly, maybe it doesn't). Maybe it takes the early exit (based on needs_fetch_) maybe it doesn't. All we care about is not having problems.

I can see the test supports this simpler assertion, but can't see that it supports the more complex assertion. Therefore, I need some elaboration on what is being tested.

---

On a tangential note:

This call to GetPath() is supposed to fetch compressed and that serves as the precondition to the next paragraph. It'd be nice if that pre-condition were actually asserted. (But this is a tiny detail; currently we assume if this isn't true, then a different test would fail.)

In prior iterations of the code, the logic for AddMap was a lot more brittle, so the test made a bit more sense then. Subsequent to that, I did the "encapsulate" refactoring which made it more difficult to imagine a buggy implementation.

In reviewing this, I did think of one invariant that I had in mind but didn't write into a test, which I've added here now.

---

bindings/pydrake/multibody/test/parsing_test.py line 88 at r6 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW It seems that if we're testing the binding of AddRemote() it would be sufficient to pass an ill-formed set of parameters and confirm that it fails the validation of AddRemote(). Actually calling GetPath() is a step removed. Or is this supposed to give us extra assurance that the values of the RemoteParams is properly transported?

Done.

[SeanCurtis-TRI]

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

---

multibody/parsing/package_map.cc line 492 at r6 (raw file):
I probably didn't help my case with that example. I picked something trivial and my point has been correspondingly trivialized.

If I were to attempt to boil this down to my key issue:

A user-facing method throws with nothing in the documentation suggesting it throws.

That said, I happily stipulate that all non-trivial conditions for throwing are supplied as a definition of "correct" values.

[jwnimmer-tri]

Reviewable status: LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri, @rpoyner-tri, and @SeanCurtis-TRI)

---

multibody/parsing/package_map.cc line 492 at r6 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I probably didn't help my case with that example. I picked something trivial and my point has been correspondingly trivialized.

If I were to attempt to boil this down to my key issue:

A user-facing method throws with nothing in the documentation suggesting it throws.

That said, I happily stipulate that all non-trivial conditions for throwing are supplied as a definition of "correct" values.

Aha! The proposed defect is that the doc doesn't mention the possibility of throwing at all, not that the valid/invalid predicate for RemoteParams is unclear.

I've updated the doc now.

[rpoyner-tri]

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

[ggould-tri]

Checkpointing while I go get lunch.

Reviewed 1 of 12 files at r4, 1 of 2 files at r9, 4 of 4 files at r11.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee ggould-tri(platform) (waiting on @jwnimmer-tri)

---

multibody/parsing/package_map.h line 120 at r11 (raw file):

    PackageMap. It is an error if the archive does not contain any diectory with
    this prefix, but if there are files outside of this directory they will be
    silently discarded. */

minor: Because this is a purely passthrough parameter, IMO this should have a more or less the identical documentation comment as package_downloader.py:56-58. Since this version of the comment is more complete, it would probably be more formally correct if you copied this text to the downloader's docstring (so you are not documenting here the undocumented behaviour of another tool).

Also you typoed the word "directory".

Suggestion:

    PackageMap. It is an error if the archive does not contain any directory with
    this prefix, but if there are files outside of this directory they will be
    silently discarded. */

---

multibody/parsing/package_map.h line 127 at r11 (raw file):

  will be downloaded from the internet (with local caching). The data will not
  be downloaded until necessary, i.e., when GetPath() is first called for the
  `package_name`. Downloading requires a valid `/usr/bin/python3` interpreter,

BTW: I'm amazed that although we document a dependency on python3, this is the only place that drake documents a runtime requirement it to be at any particular path!

Code quote:

Downloading requires a valid `/usr/bin/python3` interpreter

---

multibody/parsing/package_map.cc line 70 at r11 (raw file):

  void store(bool value) { needs_fetch_ = value; }

  /* Returns a lock_guard for our encapsulated mutex. */

What operations require holding this lock? Given that none of the methods can assert it in this design, I think it needs to be spelled out -- I can't work out what the invariant is meant to be.

Possibly this lock may be in the wrong place -- it seems like it's only ever used to gate access to PackageData, which makes putting it in NeedFetch somewhat obscure.

---

multibody/parsing/package_map.cc line 74 at r11 (raw file):

 private:
  std::atomic<bool> needs_fetch_;

minor: This field name is a source of confusion, since it's identical to the object is stored with (i.e., there is a 'NeedsFetchcalledneeds_fetch_that has a member calledneeds_fetch_`.)

---

multibody/parsing/package_map.cc line 82 at r11 (raw file):

 - A deprecation status that can be added after construction.
 - For remote packages, whether it's been fetched locally yet.
 - Atomic access and a mutex to guard the above.

minor: The atomicity and mutex are in the NeedsFetch, not in or visible from the public API of this class at all.

---

multibody/parsing/package_map.cc line 173 at r11 (raw file):

  NeedsFetch needs_fetch_;

  /* Directory in which the manifest resides. When needs_fetch() is true, it is

typo?

Suggestion:

needs_fetch_

---

multibody/parsing/package_map.cc line 174 at r11 (raw file):

  /* Directory in which the manifest resides. When needs_fetch() is true, it is
  NOT ALLOWED to access this member field (to avoid race conditions). */

minor: If access to this member is gated on your fetch tracker structure, perhaps it belongs inside of that structure? Particularly in view of the mutex semantics on checking needs_fetch_.
(or possibly, as noted above, it is the mutex that's in the wrong place?)

---

multibody/parsing/package_map.cc line 518 at r11 (raw file):

  if (params.archive_type.has_value()) {
    const std::initializer_list<const char*> known_types = {
        "zip", "tar", "gztar", "bztar", "xztar"};

minor: Consider documenting the provenance here, i.e., that this list is shutil.get_unpack_formats().

---

multibody/parsing/test/package_downloader_stress_test.py line 57 at r13 (raw file):

                children.append(subprocess.Popen([
                    sys.executable, "multibody/parsing/package_downloader.py",
                    kwargs, ""]))

minor: A mandatory empty-string command line argument is going to be very hard to debug if this test ever fails. Consider some vgreppable junk string.

Suggestion:

"UNUSED_ARGUMENT"

---

multibody/parsing/test/package_downloader_test.py line 73 at r11 (raw file):

        # Wrap a call to main.
        try:
            mut._main([filename, ""])

minor: A mandatory empty-string command line argument is going to be very hard to debug if it ever goes wrong. Consider some vgreppable junk string.

Suggestion:

"UNUSED_ARGUMENT"

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r13, 4 of 4 files at r14, all commit messages.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee ggould-tri(platform) (waiting on @jwnimmer-tri)

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r15, all commit messages.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee ggould-tri(platform) (waiting on @jwnimmer-tri)

[jwnimmer-tri]

Reviewable status: 3 unresolved discussions, LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri)

---

multibody/parsing/package_map.h line 127 at r11 (raw file):

Previously, ggould-tri wrote…

BTW: I'm amazed that although we document a dependency on python3, this is the only place that drake documents a runtime requirement it to be at any particular path!

Yes! In all other cases we allow the user to choose where it is. But here it seemed too difficult to try to obey a configuration option.

---

multibody/parsing/package_map.cc line 70 at r11 (raw file):

Previously, ggould-tri wrote…

What operations require holding this lock? Given that none of the methods can assert it in this design, I think it needs to be spelled out -- I can't work out what the invariant is meant to be.

Possibly this lock may be in the wrong place -- it seems like it's only ever used to gate access to PackageData, which makes putting it in NeedFetch somewhat obscure.

Working

I'm going to take a pass at making this more clear in the code, but here's the summary:

If NeedsFetch is true, then the mutex must be held before accessing path_ in any way.

---

multibody/parsing/package_map.cc line 74 at r11 (raw file):

Previously, ggould-tri wrote…

minor: This field name is a source of confusion, since it's identical to the object is stored with (i.e., there is a 'NeedsFetchcalledneeds_fetch_that has a member calledneeds_fetch_`.)

Done.

---

multibody/parsing/package_map.cc line 82 at r11 (raw file):

Previously, ggould-tri wrote…

minor: The atomicity and mutex are in the NeedsFetch, not in or visible from the public API of this class at all.

Indeed, from the perspective of the caller of PackageData those concepts are encapsulated just like this comment says. I've reworded to aim the comment more at the call of this class than the implementer of this class.

[rpoyner-tri]

Reviewed 3 of 4 files at r14, 1 of 1 files at r15, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri and @jwnimmer-tri)

[ggould-tri]

Reviewed 1 of 3 files at r12, 1 of 1 files at r13, 3 of 4 files at r14, 1 of 1 files at r15, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee ggould-tri(platform) (waiting on @jwnimmer-tri)

[jwnimmer-tri]

Reviewable status: LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri, @rpoyner-tri, and @SeanCurtis-TRI)

---

multibody/parsing/package_map.cc line 70 at r11 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

I'm going to take a pass at making this more clear in the code, but here's the summary:

If NeedsFetch is true, then the mutex must be held before accessing path_ in any way.

Done.

I'd tried this earlier on, but it didn't help then. I guess in the meantime things have gotten better-organized enough that it's a nice improvement!

---

multibody/parsing/package_map.cc line 174 at r11 (raw file):

Previously, ggould-tri wrote…

minor: If access to this member is gated on your fetch tracker structure, perhaps it belongs inside of that structure? Particularly in view of the mutex semantics on checking needs_fetch_.
(or possibly, as noted above, it is the mutex that's in the wrong place?)

Done.

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r16, all commit messages.
Reviewable status: LGTM missing from assignee ggould-tri(platform) (waiting on @jwnimmer-tri)

[ggould-tri]

Reviewed 1 of 9 files at r2, 5 of 12 files at r5, 1 of 2 files at r6, 1 of 1 files at r7, 2 of 3 files at r12.
Reviewable status: LGTM missing from assignee ggould-tri(platform) (waiting on @jwnimmer-tri)

[ggould-tri]

Reviewed 1 of 1 files at r16, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),ggould-tri(platform),SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri)

---

multibody/parsing/package_map.cc line 70 at r11 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Done.

I'd tried this earlier on, but it didn't help then. I guess in the meantime things have gotten better-organized enough that it's a nice improvement!

Thanks!

[jwnimmer-tri]

Here goes nothing...