doc: Add instructions for adding model artifacts #10811
Conversation
There was a problem hiding this comment.
+@jwnimmer-tri for feature review, please.
Added arbitrary suggestion on file sizes; feel free to kick those out!
Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @jwnimmer-tri)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @EricCousineau-TRI)
doc/models.rst, line 255 at r1 (raw file):
If you are adding model artifacts, such as textures or meshes, that are small enough (<100KB), and there are <10MB of such files in the source tree at any time, then add them directly to Git.
nit I don't think "total in tree at any time" is very easy for a contributor to measure.
I ran some summary reports on what's in the tree now. I think I'd propose that if the directory the meshes are in has <100KiB of data then it's okay to place the data directly in drake git.
That allows for examples/contact_model to stay intact, but all of the robots (iiwa, pr2, irb140, atlas, val, etc.) have to move their meshes + etc out-of-tree. What do you think?
doc/models.rst, line 260 at r1 (raw file):
`RobotLocomotion/models <https://github.com/RobotLocomotion/models>`_. These artifacts SHOULD be things that are large and should change infrequently,
nit No need to SCREAM; to emphasize text, prefer rst markup.
doc/models.rst, line 270 at r1 (raw file):
----------------------- #. Clone ``RobotLocomotion/models`` locally
nit Here we probably should mention "create a git branch in your local checkouts of both models and drake for your changes.
doc/models.rst, line 271 at r1 (raw file):
#. Clone ``RobotLocomotion/models`` locally #. Update ``tools/workspace/models/repository.bzl`` to point to your checkout
nit Consider spelling out drake/tools instead of just tools for extra clarity, since there are two repositories in play here?
doc/models.rst, line 279 at r1 (raw file):
``manipulation/models/ycb/BUILD.bazel``. #. Write your unittest that uses these files. Do NOT submit PRs that only adds models but has zero intent to use them; Drake is not a model repository!
nit The "what is appropriate for a PR" caution should appear (or at least be mentioned with an xref) before the sequence of steps begins -- otherwise the reader could start preparing and PR only later to realize it won't be accepted.
doc/models.rst, line 285 at r1 (raw file):
#. Push your changes to your fork of ``RobotLocomotion/models``. Make a PR. #. Update ``.../models/repository.bzl`` to use the commit you pushed.
nit We might as well write out the entire path here (to match exactly however it's spelled above).
doc/models.rst, line 286 at r1 (raw file):
#. Push your changes to your fork of ``RobotLocomotion/models``. Make a PR. #. Update ``.../models/repository.bzl`` to use the commit you pushed. #. Submit a PR to Drake, label it as ``do not merge``. Reference your ``models``
nit Many contributors can't label their PRs.
There are probably a few ways to adjust the language here, but perhaps suggest posting a reviewable thread "Working temporary sha until the models PR is merged." -- this will automatically assign the discussion to the author, but keep it visible to reviewers. (Reviewers could opt-in to blocking on that thread as well.)
doc/models.rst, line 287 at r1 (raw file):
#. Update ``.../models/repository.bzl`` to use the commit you pushed. #. Submit a PR to Drake, label it as ``do not merge``. Reference your ``models`` PR in this PR.
nit "Reference your models PR in this PR" is unclear. I think what you're asking for is the Drake PR description to mention the models PR number?
doc/models.rst, line 295 at r1 (raw file):
#) Update Drake's ``.../models/repository.bzl`` to the latest merge commit on ``master`` for ``RobotLocomotion/models``. #) Ensure your PR fully passes CI, then have your PR merged.
nit "Ensure ... passes CI" is a bit confusing -- we generally don't (can't) merge PRs that fail CI. Maybe this last bullet is just "Merge your drake PR."? Or is there something in particular to clarify here?
There was a problem hiding this comment.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @EricCousineau-TRI)
a discussion (no related file):
This file got removed recently (which I'm totes happy about); should I move this stuff to developers.rst or somewhere else?
There was a problem hiding this comment.
Reviewable status: 10 unresolved discussions, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @EricCousineau-TRI)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
This file got removed recently (which I'm totes happy about); should I move this stuff to
developers.rstor somewhere else?
Yes, we should add this new text somewhere. Either developers.rst, or a sub-page of developers.rst.
|
@EricCousineau-TRI I would like to see this text land again. I was looking for it a couple days ago and couldn't find it. |
EricCousineau-TRI
force-pushed
the
feature/doc_model_adding
branch
from
0af8e4b
to
34defc0
Compare
EricCousineau-TRI
force-pushed
the
feature/doc_model_adding
branch
from
34defc0
to
1385393
Compare
There was a problem hiding this comment.
Will address comments on original text.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Yes, we should add this new text somewhere. Either developers.rst, or a sub-page of developers.rst.
OK Moved to sub-page, under "Version Control".
There was a problem hiding this comment.
Reviewed 1 of 3 files at r2.
Reviewable status: 9 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @EricCousineau-TRI and @jwnimmer-tri)
There was a problem hiding this comment.
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 @EricCousineau-TRI and @jwnimmer-tri)
a discussion (no related file):
Should either migrate robot meshes to models, or create an issue to do so.
doc/models.rst, line 255 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit I don't think "total in tree at any time" is very easy for a contributor to measure.
I ran some summary reports on what's in the tree now. I think I'd propose that if the directory the meshes are in has <100KiB of data then it's okay to place the data directly in drake git.
That allows for
examples/contact_modelto stay intact, but all of the robots (iiwa, pr2, irb140, atlas, val, etc.) have to move their meshes + etc out-of-tree. What do you think?
Done. Created blocking item move meshes out of tree.
doc/models.rst, line 260 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit No need to SCREAM; to emphasize text, prefer rst markup.
Done.
doc/models.rst, line 270 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Here we probably should mention "create a git branch in your local checkouts of both
modelsanddrakefor your changes.
Done.
doc/models.rst, line 271 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Consider spelling out
drake/toolsinstead of justtoolsfor extra clarity, since there are two repositories in play here?
Done.
doc/models.rst, line 285 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit We might as well write out the entire path here (to match exactly however it's spelled above).
Done.
doc/models.rst, line 286 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Many contributors can't label their PRs.
There are probably a few ways to adjust the language here, but perhaps suggest posting a reviewable thread "Working temporary sha until the models PR is merged." -- this will automatically assign the discussion to the author, but keep it visible to reviewers. (Reviewers could opt-in to blocking on that thread as well.)
Done.
doc/models.rst, line 287 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit "Reference your models PR in this PR" is unclear. I think what you're asking for is the Drake PR description to mention the models PR number?
Done.
doc/models.rst, line 295 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit "Ensure ... passes CI" is a bit confusing -- we generally don't (can't) merge PRs that fail CI. Maybe this last bullet is just "Merge your drake PR."? Or is there something in particular to clarify here?
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r3.
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 @EricCousineau-TRI)
EricCousineau-TRI
force-pushed
the
feature/doc_model_adding
branch
from
8c4a453
to
f3b950b
Compare
There was a problem hiding this comment.
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 @EricCousineau-TRI and @jwnimmer-tri)
doc/models.rst, line 279 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit The "what is appropriate for a PR" caution should appear (or at least be mentioned with an xref) before the sequence of steps begins -- otherwise the reader could start preparing and PR only later to realize it won't be accepted.
Done.
There was a problem hiding this comment.
The fixups look good. Should I do a clean re-read of the whole thing now, or wait for other amendments first?
Reviewed 1 of 3 files at r2, 1 of 1 files at r4.
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 @EricCousineau-TRI)
|
Finished my amendments, go for it! |
There was a problem hiding this comment.
As we work through models PRs perhaps we'll find more tweaks, but this is a good baseline.
Reviewable status: 4 unresolved discussions, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)
doc/model_version_control.rst, line 21 at r4 (raw file):
Before you decide to submit models, please ensure that you have tests that will use them. Do not submit a PR that adds models but has zero intent to use
nit Maybe will need them instead of will use them to be more strongly worded?
doc/model_version_control.rst, line 32 at r4 (raw file):
#. Create a Git branch in your local checkouts of *both* ``models`` and ``drake``. #. Update ``tools/workspace/models/repository.bzl`` to point to your checkout
nit drake/tools/workspace/models/repository.bzl (leading drake/) for consistency.
doc/model_version_control.rst, line 32 at r4 (raw file):
#. Create a Git branch in your local checkouts of *both* ``models`` and ``drake``. #. Update ``tools/workspace/models/repository.bzl`` to point to your checkout
nit "your checkout" => "your models checkout" for clarify?
EricCousineau-TRI
changed the title
EricCousineau-TRI
changed the title
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)
There was a problem hiding this comment.
+@soonho-tri for platform review, please.
Let me know if you'd like us to review this with all platform reviewers, since it's a policy update.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee soonho-tri(platform), commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @soonho-tri)
There was a problem hiding this comment.
Reviewed 1 of 3 files at r2, 1 of 1 files at r5.
Reviewable status: 3 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)
doc/model_version_control.rst, line 16 at r5 (raw file):
These artifacts *should* be things that are large
It is a bit ambiguous if "these artifacts" indicates those should go to RobotLocomotion/models or artifacts in general. If it's the former, then merging these sentences into the previous paragraph is a way to resolve the ambiguity.
doc/model_version_control.rst, line 39 at r5 (raw file):
#. Ensure that you use ``forward_files`` to make the files available inside the Drake bazel workspace. For an example, see ``manipulation/models/ycb/BUILD.bazel``.
nit, for consistency let's use drake/manipulation/models/ycb/BUILD.bazel. Can we also make this a hyperlink (so that people can see the example immediately)
EricCousineau-TRI
force-pushed
the
feature/doc_model_adding
branch
from
c2c5ea6
to
6dc2bb2
Compare
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI, @jwnimmer-tri, and @soonho-tri)
doc/model_version_control.rst, line 16 at r5 (raw file):
Previously, soonho-tri (Soonho Kong) wrote…
These artifacts *should* be things that are largeIt is a bit ambiguous if "these artifacts" indicates those should go to
RobotLocomotion/modelsor artifacts in general. If it's the former, then merging these sentences into the previous paragraph is a way to resolve the ambiguity.
Done. (Tried to simplify wording a bit?)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6.
Reviewable status: 2 unresolved discussions, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI and @soonho-tri)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r6.
Reviewable status: 1 unresolved discussion, commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @EricCousineau-TRI)
There was a problem hiding this comment.
Reviewable status: commits need curation (https://drake.mit.edu/reviewable.html#curated-commits)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Should either migrate robot meshes to
models, or create an issue to do so.
OK Filed issue: #11913
EricCousineau-TRI
added
the
status: squashing now
Modeled after workflow in #10532 + RobotLocomotion/models#5
Relates #6124 in that it is a workaround for large data storage specific to models.
(We are using Girder for Anzu, but are not yet ready to commit to this for use in Drake.)
This change is