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] Elevate drake_models to public visibility #19104

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Mar 30, 2023

Towards #15774.

Towards #19094 -- this fixes the drake_bazel_installed but not rules_python.

(Sorry, I know this is like the third time I've rewritten this code in the past two weeks.)


This change is Reviewable

Update version control documentation to match.

Simplify its package.BUILD.bazel, to better match how a bare ROS
package would work (just a pile of bare files).

Enhance the drake_bazel_installed workflow to provide drake_models as
an external repository to avoid extra downloading at runtime.

Remove the models pre-fetching from the drake_bazel_installed_test.
We still meet the DRAKE_ALLOW_NETWORK=none criterion (i.e., PackageMap
does not download anything) because the bazel repository rules fetch
the models now, which isn't part of what's governed.
@jwnimmer-tri jwnimmer-tri added priority: medium release notes: fix This pull request contains fixes (no new features) labels Mar 30, 2023
@jwnimmer-tri jwnimmer-tri marked this pull request as ready for review March 30, 2023 18:14
@jwnimmer-tri
Copy link
Collaborator Author

+@rpoyner-tri for feature review, please. (We'll tap Xuchen for platform after feature is 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: Moderate regret for statically listing all the files, but I realize that's probably the right interface for a public dependency.

Reviewed 29 of 29 files at r1, all commit messages.
Reviewable status: needs at least two assigned reviewers (waiting on @jwnimmer-tri)

@jwnimmer-tri
Copy link
Collaborator Author

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

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.

:lgtm: platform

Moderate regret for statically listing all the files, but I realize that's probably the right interface for a public dependency.

Can someone explain why statically listing all files is better than globbing everything in a directory, even when the list is tediously long?

Reviewed 29 of 29 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)


-- commits line 10 at r1:
BTW, what about non-bazel workflows. When does model downloading for those happen?

Code quote:

  Enhance the drake_bazel_installed workflow to provide drake_models as
  an external repository to avoid extra downloading at runtime.

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.

Can someone explain why statically listing all files is better than globbing everything in a directory, ...

You can only glob within your own bazel package, i.e., where your BUILD.bazel file lives. Trying to glob someone else's package is an error.

... even when the list is tediously long?

Hah. This is nothing. Personally, I think listing out a dozen files when writing an example program is way easier than trying to debug why someone's glob() has gone awry and not found what they wanted it to find. BUILD files should be direct and explicit, for clarity.

Reviewable status: 1 unresolved discussion (waiting on @xuchenhan-tri)


-- commits line 10 at r1:

Previously, xuchenhan-tri wrote…

BTW, what about non-bazel workflows. When does model downloading for those happen?

When building Drake from source via Bazel (e.g., in Anzu), the model downloading happens via repository rules at "bazel fetch" time (which itself happens implicitly as part of "bazel build"). Our C++ code never downloads anything in this case.

In most other cases, package://drake_models is downloaded on demand the first time someone calls package_map.GetPath("drake_models"), and then cached for future use so there are no more downloads (of that git version pin). This is what happens for apt or pip users -- the download cached into their $HOME/.cache directory or alike.

The problem this PR solves (in part) is when someone is using a Drake binary release but doing it via Bazel. In that case, tests cannot write to $HOME/.cache (because of sandboxing) so even though the download happens and the test passes, there is no caching so it's downloaded over and over again, for any test that happens to use those models. We solve it here for anyone using the drake_bazel_external pattern from drake-external-examples. What still remains to be solved is how to solve it for drake binary + bazel users who are not using that ruleset (e.g., if they are using rules_python with pip instead).

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.

You can only glob within your own bazel package, i.e., where your BUILD.bazel file lives. Trying to glob someone else's package is an error.

Thanks for the reference. Is it possible for the model directory to glob the mtl/obj/other files and export them for easy consumption of the drake BUILD file?

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),xuchenhan-tri(platform) (waiting on @jwnimmer-tri)


-- commits line 10 at r1:

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

When building Drake from source via Bazel (e.g., in Anzu), the model downloading happens via repository rules at "bazel fetch" time (which itself happens implicitly as part of "bazel build"). Our C++ code never downloads anything in this case.

In most other cases, package://drake_models is downloaded on demand the first time someone calls package_map.GetPath("drake_models"), and then cached for future use so there are no more downloads (of that git version pin). This is what happens for apt or pip users -- the download cached into their $HOME/.cache directory or alike.

The problem this PR solves (in part) is when someone is using a Drake binary release but doing it via Bazel. In that case, tests cannot write to $HOME/.cache (because of sandboxing) so even though the download happens and the test passes, there is no caching so it's downloaded over and over again, for any test that happens to use those models. We solve it here for anyone using the drake_bazel_external pattern from drake-external-examples. What still remains to be solved is how to solve it for drake binary + bazel users who are not using that ruleset (e.g., if they are using rules_python with pip instead).

The explanation is very helpful. Thank you!

@jwnimmer-tri
Copy link
Collaborator Author

Is it possible for the model directory to glob the mtl/obj/other files and export them for easy consumption of the drake BUILD file?

Yes it's possible in Bazel in general, but I am purposefully avoiding it. The design I am aiming for is that drake_models will be like a ROS package. As such, it will not have a BUILD file and will not provide any filegroup shortcuts.

@jwnimmer-tri jwnimmer-tri merged commit 3bf1bf4 into RobotLocomotion:master Apr 3, 2023
@jwnimmer-tri jwnimmer-tri deleted the drake-models-repository branch April 3, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants