-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[models] Delete robot descriptions that now live in drake_models #21243
[models] Delete robot descriptions that now live in drake_models #21243
Conversation
ef7c70f
to
95677a8
Compare
+@xuchenhan-tri for both (?) reviews per schedule (tomorrow), please. If there are parts that seem like they deserve a second reviewer, or separate feature reviewers, just let me know. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform)
manipulation/models/jaco_description/README.md
line 0 at r1 (raw file):
FYI This PR purposely doesn't delete two of the files here. Those are tracked under #21216.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, I probably won't be able to get to it until tomorrow. I have back-to-back meetings all day today.
Reviewable status: LGTM missing from assignee xuchenhan-tri(platform)
No problem, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checkpoint for my sanity.
Reviewed 100 of 172 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri(platform)
examples/manipulation_station/BUILD.bazel
line 35 at r1 (raw file):
# anymore, but removing them breaks too many other things. We'll # revisit this later when we finally delete these paths entirely. "//manipulation/models/iiwa_description:models",
BTW, I want to make sure that we can properly delete these now because they are already included in @drake_models://wsg_50_description
and @drake_models:iiwa_description
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri(platform)
examples/manipulation_station/BUILD.bazel
line 35 at r1 (raw file):
Previously, xuchenhan-tri wrote…
BTW, I want to make sure that we can properly delete these now because they are already included in
@drake_models://wsg_50_description
and@drake_models:iiwa_description
, right?
It's slightly more nuanced than that.
For the purposes of all of the code in this directory, the pre-existing "@drake_models//:iiwa_description",
item (about 6 lines prior) was already sufficient -- both on master, and in this PR.
The problem (prior to this PR) was that other code that transitively depended on the manipulation station library code was relying on the manipulation station to declare "//manipulation/models/iiwa_description:models"
as a data dep. Code in other packages wasn't doing "DWYU" correctly. Trying to fix that in prior PRs was too much of a headache, especially because there used to be circular model file dependencies until #21241 landed.
Now that this PR finally adjusts all of the data paths project-wide to be correct and precise, we don't need this little hack anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee xuchenhan-tri(platform)
examples/manipulation_station/BUILD.bazel
line 35 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
It's slightly more nuanced than that.
For the purposes of all of the code in this directory, the pre-existing
"@drake_models//:iiwa_description",
item (about 6 lines prior) was already sufficient -- both on master, and in this PR.The problem (prior to this PR) was that other code that transitively depended on the manipulation station library code was relying on the manipulation station to declare
"//manipulation/models/iiwa_description:models"
as a data dep. Code in other packages wasn't doing "DWYU" correctly. Trying to fix that in prior PRs was too much of a headache, especially because there used to be circular model file dependencies until #21241 landed.Now that this PR finally adjusts all of the data paths project-wide to be correct and precise, we don't need this little hack anymore.
Nice, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 72 of 172 files at r1.
Reviewable status: 1 unresolved discussion
bindings/pydrake/examples/test/manipulation_station_test.py
line 5 at r1 (raw file):
import numpy as np from pydrake.common import FindResourceOrThrow
nit, no longer needed.
95677a8
to
7788898
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the slog.
Reviewable status: complete! all discussions resolved, LGTM from assignee xuchenhan-tri(platform)
Towards #13942.
Follow-up from #21218.
Anzu CI has been checked and passed.
This change is