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

[build] Fully retire the non-lcmtypes drake module #22265

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Dec 4, 2024

Towards #20731. See #21453 for prior art.


This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: low release notes: none This pull request should not be mentioned in the release notes labels Dec 4, 2024
@jwnimmer-tri
Copy link
Collaborator Author

+@xuchenhan-tri for feature review, please (low priority).

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.

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


a discussion (no related file):
Working

Ugh, this breaks certain particular kinds of downstream projects (e..g, one of the drake-external-examples). I will need to rework this a little bit.

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.

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


a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

Ugh, this breaks certain particular kinds of downstream projects (e..g, one of the drake-external-examples). I will need to rework this a little bit.

OK fixed now, by reverting ~20% of it with some new comments.

Tested via:

cd drake-external-examples/drake_bazel_external
env EXAMPLES_LOCAL_DRAKE_PATH=/path/to/drake bazel test //...

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: with a big picture question.

Reviewed 4 of 7 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers


-- commits line 5 at r2:
Could you say why (perhaps in reviewable and not necessarily in the commit message).
I have a vague sense that this has something to do with lowered visibility of the packages? but I don't fully get it.

Code quote:

With bzlmod this is no longer practical.

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.

Reviewed 4 of 7 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: 1 unresolved discussion, needs at least two assigned reviewers


-- commits line 5 at r2:

Previously, xuchenhan-tri wrote…

Could you say why (perhaps in reviewable and not necessarily in the commit message).
I have a vague sense that this has something to do with lowered visibility of the packages? but I don't fully get it.

Previously, when the developer's WORKSPACE had the line workspace(name = "foo") the Python sys.path provided by rules_python would be organized to provide package names like from foo.common import thing.

Later, Bazel people decided this was a Really Bad Move and the paths should have been from common import thing. This is important because "name of an external" with bzlmod is more complicated now -- globally there can be multiple copies of an external, so "my copy of foo" is now a local concept, rather than part of a (necessarily global) sys.path.

This is also more consistent with non-Bazelized Python, where the parent of the directory you cloned the git repository into is never on your sys.path. Take for instance https://github.com/urllib3/urllib3, it has a subdir named src which is (notionally) on the sys.path, and then inside of src there is a directory named urllib3 with a file __init__.py, so users would do from urllib import .... In other words, for anything spelled as from xyz import ... there should be a directory in source control named xyz; the git repository's name to clone doesn't count, since that is not actually a constant (the user can name it anything they want). So that's how Python in the wider universe works, this is just bringing rules_python up to speed to match that.

@jwnimmer-tri
Copy link
Collaborator Author

+@SeanCurtis-TRI for platform review, please.

@xuchenhan-tri
Copy link
Contributor

-- commits line 5 at r2:

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Previously, when the developer's WORKSPACE had the line workspace(name = "foo") the Python sys.path provided by rules_python would be organized to provide package names like from foo.common import thing.

Later, Bazel people decided this was a Really Bad Move and the paths should have been from common import thing. This is important because "name of an external" with bzlmod is more complicated now -- globally there can be multiple copies of an external, so "my copy of foo" is now a local concept, rather than part of a (necessarily global) sys.path.

This is also more consistent with non-Bazelized Python, where the parent of the directory you cloned the git repository into is never on your sys.path. Take for instance https://github.com/urllib3/urllib3, it has a subdir named src which is (notionally) on the sys.path, and then inside of src there is a directory named urllib3 with a file __init__.py, so users would do from urllib import .... In other words, for anything spelled as from xyz import ... there should be a directory in source control named xyz; the git repository's name to clone doesn't count, since that is not actually a constant (the user can name it anything they want). So that's how Python in the wider universe works, this is just bringing rules_python up to speed to match that.

Thanks for the explanation. It helps somewhat but not completely. Do you happen to have some reference on this from bzlmod that you can share?

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.

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform)


-- commits line 5 at r2:

Previously, xuchenhan-tri wrote…

Thanks for the explanation. It helps somewhat but not completely. Do you happen to have some reference on this from bzlmod that you can share?

Maybe these help:
bazelbuild/rules_python#1679
bazelbuild/bazel#18128

Copy link
Contributor

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

Reviewed 4 of 7 files at r1, 6 of 6 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions


__init__.py line 1 at r2 (raw file):

# It brittle to have both drake-the-workspace and drake-the-lcmtypes-package

nit: grammar

(If you've ever seen the movie, "Murder By Death", you should be able to imagine Truman Capote shouting this.)

Suggestion:

It is brittle

In the past, we allowed 'import drake...' to refer to Python libraries
inside the Drake source tree. With bzlmod this is no longer practical.
Prior commits already removed most uses; this commit finishes the job.
From now on, imports should say e.g., 'from common...', instead.

Note that lcmtypes like 'from drake import lcmt_geometry_data' still
retain the 'drake' package name.
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.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion

Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion

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.

Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),xuchenhan-tri(platform)


-- commits line 5 at r2:

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Maybe these help:
bazelbuild/rules_python#1679
bazelbuild/bazel#18128

Happy to keep chatting, but for now I'll merge this to keep the train going.

@jwnimmer-tri jwnimmer-tri merged commit cea8a38 into RobotLocomotion:master Dec 5, 2024
9 of 10 checks passed
@jwnimmer-tri jwnimmer-tri deleted the bzlmod-imports branch December 5, 2024 16:06
RussTedrake pushed a commit to RussTedrake/drake that referenced this pull request Dec 15, 2024
…2265)

In the past, we allowed 'import drake...' to refer to Python libraries
inside the Drake source tree. With bzlmod this is no longer practical.
Prior commits already removed most uses; this commit finishes the job.
From now on, imports should say e.g., 'from common...', instead.

Note that lcmtypes like 'from drake import lcmt_geometry_data' still
retain the 'drake' package name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium release notes: none This pull request should not be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants