Install .pyi files #17709
Install .pyi files #17709
Conversation
|
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-snopt-mosek-release please. |
|
The macOS wheel build is https://drake-jenkins.csail.mit.edu/job/mac-big-sur-unprovisioned-clang-wheel-experimental-snopt-mosek-release/17/. (The build in the list of checks is an aborted staging; we're still having the issue with the staging build getting conflated with the experimental build.) |
mwoehlke-kitware
added
the
release notes: feature
|
+@jwnimmer-tri for feature review, please. Note: I don't think we need to note (i.e. release notes) the "new" dependencies, as the usual setup scripts consider the run-time dependencies as a strict subset of the compile-time dependencies. The new addition of numpy and matplotlib mainly affects wheels, which don't use the setup scripts and so were only installing actual build-time dependencies. |
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @mwoehlke-kitware)
bindings/pydrake/BUILD.bazel line 683 at r1 (raw file):
) PYI_FILES = [
nit This list appears to be in a random order? If the order is irrelevant (and I think it probably is?) then it should be alpha-sorted for clarity and easier ongoing maintenance.
bindings/pydrake/BUILD.bazel line 683 at r1 (raw file):
) PYI_FILES = [
nit Please add a TODO here to auto-calculate this list. Asking our developers keep this list up-to-date manually is not a reasonable maintenance burden over the long term.
tools/wheel/image/provision-python.sh line 20 at r1 (raw file):
lxml \ matplotlib \ numpy \
As we add more and more packages here, I am less and less comfortable with leaving them unpinned, making our wheel builds less and less reproducible.
Is now the time to switch this to using a requirements.txt file? Possibly with foo==#.#.# fully-resolved lockfile (using one of Python's typical tools to manage that)? Or add a TODO?
mwoehlke-kitware
force-pushed
the
install-python-stubs
branch
from
93476a7 to
286565d
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri)
bindings/pydrake/BUILD.bazel line 683 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit This list appears to be in a random order? If the order is irrelevant (and I think it probably is?) then it should be alpha-sorted for clarity and easier ongoing maintenance.
It appears to be in the order that find produced. Sorted, now.
bindings/pydrake/BUILD.bazel line 683 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Please add a TODO here to auto-calculate this list. Asking our developers keep this list up-to-date manually is not a reasonable maintenance burden over the long term.
Done. (Though I have no idea how that's going to happen.)
tools/wheel/image/provision-python.sh line 20 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
As we add more and more packages here, I am less and less comfortable with leaving them unpinned, making our wheel builds less and less reproducible.
Is now the time to switch this to using a
requirements.txtfile? Possibly withfoo==#.#.#fully-resolved lockfile (using one of Python's typical tools to manage that)? Or add a TODO?
I added a TODO, but none of the other builds are pinned either; they're using "whatever version Ubuntu/Brew ships".
There was a problem hiding this comment.
(Seems like this is missing a push, still.)
Reviewed all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri)
mwoehlke-kitware
force-pushed
the
install-python-stubs
branch
from
286565d to
189384c
Compare
|
Ack. I absolutely pushed it last night. It appears I failed to commit changes though. 😩 |
mwoehlke-kitware
force-pushed
the
install-python-stubs
branch
from
189384c to
c38ba56
Compare
|
It appears that |
There was a problem hiding this comment.
Reviewed 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @mwoehlke-kitware)
bindings/pydrake/BUILD.bazel line 785 at r4 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
It appears that
genruleis incorrectly addinga+xpermissions to the.pyis. (The files produced bybazel run //bindings/pydrake:stubgenare correctly not executable.)
Is it a problem in practice? If not, then it could be a TODO. If yes, then one way to hack around it is to teach install.py to notice *.pyi files being copied and to unset a-x in that case.
tools/wheel/image/provision-python.sh line 20 at r1 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
I added a TODO, but none of the other builds are pinned either; they're using "whatever version Ubuntu/Brew ships".
Homebrew is a lost cause, no possibility for reproducible builds there.
Ubuntu typically only pushes out security patches, not major version upgrades, so while not 100% reproducible it was pretty close, and unlikely to be a problem in practice. By using bleeding-edge pypi files here, we're definitely making the hazard acutely worse, and a reproducibility problem in practice is a real possibility.
The upgrade process is easy -- update the monthly dependencies playbook documentation and/or tooling to cover regular upgrades of the lockfile here (e.g., with pip-compile).
|
Yikes, by using I'll see if I can suggest a patch to resolve that. |
mwoehlke-kitware
force-pushed
the
install-python-stubs
branch
from
c38ba56 to
0608e2f
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri)
bindings/pydrake/BUILD.bazel line 785 at r4 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Is it a problem in practice? If not, then it could be a TODO. If yes, then one way to hack around it is to teach
install.pyto notice*.pyifiles being copied and to unseta-xin that case.
It's... not ideal. The "intent" of my comment was that I was hoping someone knew how to fix this, but apparently it's a Bazel misfeature. So, yeah, TODO for now... 😢
tools/wheel/image/provision-python.sh line 20 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Homebrew is a lost cause, no possibility for reproducible builds there.
Ubuntu typically only pushes out security patches, not major version upgrades, so while not 100% reproducible it was pretty close, and unlikely to be a problem in practice. By using bleeding-edge pypi files here, we're definitely making the hazard acutely worse, and a reproducibility problem in practice is a real possibility.
The upgrade process is easy -- update the monthly dependencies playbook documentation and/or tooling to cover regular upgrades of the lockfile here (e.g., with pip-compile).
🤷 There's a TODO now; is that sufficient for the moment?
There was a problem hiding this comment.
Reviewed all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers (waiting on @jwnimmer-tri and @mwoehlke-kitware)
bindings/pydrake/BUILD.bazel line 757 at r4 (raw file):
"pydrake/solvers/sdpa_free_format.pyi", "pydrake/solvers/snopt.pyi", "pydrake/stubgen.pyi",
nit I don't think we want "stubgen.pyi" installed? The stubgen.py is not installed, is it?
bindings/pydrake/BUILD.bazel line 790 at r4 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Yikes, by using
stubgenas atools = [..], it's rebuilding all of pydrake in host mode, in addition to target mode, nearly doubling the compile time for an install.I'll see if I can suggest a patch to resolve that.
--- a/bindings/pydrake/BUILD.bazel
+++ b/bindings/pydrake/BUILD.bazel
@@ -673,7 +673,6 @@ generate_pybind_coverage(
drake_py_binary(
name = "stubgen",
srcs = ["stubgen.py"],
- args = ["--package=pydrake"],
deps = [
":all_py",
"@mypy_internal//:mypy",
@@ -784,10 +783,9 @@ PYI_FILES = [
genrule(
name = "pydrake_pyi",
- srcs = [],
+ srcs = [":stubgen"],
outs = PYI_FILES,
- cmd = "$(location :stubgen) --package=pydrake --output=$(RULEDIR)",
- tools = [":stubgen"],
+ cmd = "$(locations :stubgen) --package=pydrake --output=$(RULEDIR)",
)
TYPED_FILES = ["pydrake/py.typed"]
@@ -797,7 +795,6 @@ genrule(
srcs = [],
outs = TYPED_FILES,
cmd = "touch $@",
- tools = [":stubgen"],
)
install(
diff --git a/bindings/pydrake/stubgen.py b/bindings/pydrake/stubgen.py
index 2d95744469..bbcf469ded 100644
--- a/bindings/pydrake/stubgen.py
+++ b/bindings/pydrake/stubgen.py
@@ -10,4 +10,10 @@ from mypy import stubgen
import pydrake.all
if __name__ == "__main__":
+ # Remove the stray "path/to/stubgen.py" due to $(locations ...) leaking a
+ # second filename into argv.
+ assert sys.argv[1].endswith("/stubgen.py")
+ del sys.argv[1]
+
+ # Run the mypy.stubgen tool.
sys.exit(stubgen.main())
mwoehlke-kitware
force-pushed
the
install-python-stubs
branch
from
0608e2f to
ca72c1b
Compare
|
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Removing the |
mwoehlke-kitware
force-pushed
the
install-python-stubs
branch
from
ca72c1b to
ce824e1
Compare
|
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Done. That's... interesting; I have no idea why that's being generated in the first place. That's probably going to make for fun times needing to remember to not include that. (Meh, I already pushed, but I'm tempted to leave it in the list, but commented out, for that reason. Or, probably after this PR, move the whole lot into |
mwoehlke-kitware
force-pushed
the
install-python-stubs
branch
from
ce824e1 to
cb244e0
Compare
|
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Meh, since I had a missing comma, did that along with the other fix. |
There was a problem hiding this comment.
master, the only changes to the install footprint are the addition of the py.typed and *.pyi files.
+@EricCousineau-TRI for platform review per schedule, please.
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee EricCousineau-TRI(platform) (waiting on @EricCousineau-TRI and @mwoehlke-kitware)
bindings/pydrake/BUILD.bazel line 790 at r4 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Removing the
argsfrom:stubgenis no ideal; it means users manually generating stubs viabazel runwould need to know the argument. The hack instubgen.pyis also inelegant. IIUC the gist of the change, the patch I pushed should have the same effect without needing that hack and without removing useful functionality.
OK I confirmed that the bash spelling here still works -- the PR no longer spuriously compiles host-mode pydrake.
bindings/pydrake/BUILD.bazel line 676 at r6 (raw file):
name = "stubgen", srcs = ["stubgen.py"], args = ["--package=pydrake"],
Having args appear here is misleading now, because this args is not propagated through to actual use of this program as part of the genrule() below, which is the only use-site we care about.
In other words, this args is dead code -- it could say --package=does_not_exist here and CI would still pass.
There is no use case for running this program on the command-line, so the "sugar" here to save people the trouble of typing --package=pydrake into bazel run is not worth the extra confusion factor.
|
Aha, apparently my patch suggestion was not sound wrt a clean build. I'll see if I can figure out the right fix. |
There was a problem hiding this comment.
Reviewed 3 of 5 files at r1, 1 of 2 files at r3, 1 of 1 files at r6, all commit messages.
Reviewable status: 3 unresolved discussions (waiting on @mwoehlke-kitware)
bindings/pydrake/BUILD.bazel line 683 at r6 (raw file):
) # TODO(jwnimmer-tri): This list should be automatically generated.
BTW Ja, +1... if only Bazel made it easy to do this somehow at analysis time :(
bindings/pydrake/BUILD.bazel line 785 at r6 (raw file):
] # TODO(mwoehlke-kitware) Yell at someone to actually fix, rather than just
nit "yell at someone" is already done, consider rewording ;)
Consider just stating "Note: Due to , we scrub execute permission in "
bindings/pydrake/BUILD.bazel line 801 at r6 (raw file):
genrule( name = "pydrake_typed",
nit What is this for? It appears unused, and seems to contain no useful information?
Consider adding comment, or TODO, or whatevs.
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions (waiting on @EricCousineau-TRI and @mwoehlke-kitware)
bindings/pydrake/BUILD.bazel line 801 at r6 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit What is this for? It appears unused, and seems to contain no useful information?
Consider adding comment, or TODO, or whatevs.
See https://peps.python.org/pep-0561/#packaging-type-information for background.
Suggestion: don't use an empty file, rather fill the file with a comment
# Marker file for PEP 561.Using https://github.com/RobotLocomotion/drake/blob/master/tools/workspace/generate_file.bzl is one way to do that, or else echo instead of touch would also work.
mwoehlke-kitware
force-pushed
the
install-python-stubs
branch
from
cb244e0 to
167fe16
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: 4 unresolved discussions (waiting on @EricCousineau-TRI and @mwoehlke-kitware)
bindings/pydrake/BUILD.bazel line 815 at r6 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit It was a bit confusing to see the constants here, rather than the targets produced by
genrules.
(it was hard to deduce the purpose ofpydrake_typedfor that reason).Can this refer to the targets, not the constants, for clarity?
Could you elaborate? I can't think of any different implementation here that would add clarity. Those two constants actually list the filenames that will be installed, which seems like exactly what we want here?
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions (waiting on @EricCousineau-TRI, @jwnimmer-tri, and @mwoehlke-kitware)
bindings/pydrake/BUILD.bazel line 815 at r6 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Could you elaborate? I can't think of any different implementation here that would add clarity. Those two constants actually list the filenames that will be installed, which seems like exactly what we want here?
Having a bazel genrule target that outputs a full maniest, but then not using the bazel target but instead using the full manifest is confusing, IMO.
That in conjunction with the touch above w/o comment made me rather confused.
mwoehlke-kitware
force-pushed
the
install-python-stubs
branch
from
a8b30f5 to
de190f2
Compare
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),EricCousineau-TRI(platform) (waiting on @jwnimmer-tri)
bindings/pydrake/BUILD.bazel line 676 at r6 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Having
argsappear here is misleading now, because thisargsis not propagated through to actual use of this program as part of thegenrule()below, which is the only use-site we care about.In other words, this
argsis dead code -- it could say--package=does_not_existhere and CI would still pass.There is no use case for running this program on the command-line, so the "sugar" here to save people the trouble of typing
--package=pydrakeintobazel runis not worth the extra confusion factor.
Done.
bindings/pydrake/BUILD.bazel line 785 at r6 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
nit "yell at someone" is already done, consider rewording ;)
Consider just stating "Note: Due to , we scrub execute permission in "
Rewording done. Right now these (and who knows what other genrule outputs) get installed with a+x. We can revisit in a follow-up if upstream (Bazel) can't be fixed.
bindings/pydrake/BUILD.bazel line 801 at r6 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
See https://peps.python.org/pep-0561/#packaging-type-information for background.
Suggestion: don't use an empty file, rather fill the file with a comment
# Marker file for PEP 561.Using https://github.com/RobotLocomotion/drake/blob/master/tools/workspace/generate_file.bzl is one way to do that, or else
echoinstead oftouchwould also work.
Done (comment). The standard appears to be to use an empty file, and in fact the content is relevant (note), so I think we'd best not buck the trend.
bindings/pydrake/BUILD.bazel line 815 at r6 (raw file):
Can this refer to the targets, not the constants, for clarity?
🤷 Does that work? I don't know how targets get interpreted here...
Done. I'm actually with Eric on this, but I didn't know that worked...
There was a problem hiding this comment.
Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status:
bindings/pydrake/BUILD.bazel line 801 at r6 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Done (comment). The standard appears to be to use an empty file, and in fact the content is relevant (note), so I think we'd best not buck the trend.
FYI my suggestion was based on https://github.com/python/mypy/blob/master/mypy/py.typed.
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform) (waiting on @mwoehlke-kitware)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: LGTM missing from assignee jwnimmer-tri(platform) (waiting on @mwoehlke-kitware)
bindings/pydrake/BUILD.bazel line 785 at r6 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Rewording done. Right now these (and who knows what other
genruleoutputs) get installed witha+x. We can revisit in a follow-up if upstream (Bazel) can't be fixed.
OK Thanks!
bindings/pydrake/BUILD.bazel line 801 at r6 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
FYI my suggestion was based on https://github.com/python/mypy/blob/master/mypy/py.typed.
OK Thanks!
bindings/pydrake/BUILD.bazel line 815 at r6 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Can this refer to the targets, not the constants, for clarity?
🤷 Does that work? I don't know how targets get interpreted here...
Done. I'm actually with Eric on this, but I didn't know that worked...
OK Yup! Don't have succinct explanation, but here are some maybe-not-too-noisy links:
first-party docs for attr.label_list
https://bazel.build/rules/lib/attr#label_list - see allow_files
usage:
https://github.com/RobotLocomotion/drake/blob/v1.5.0/tools/install/install.bzl#L543
https://github.com/RobotLocomotion/drake/blob/v1.5.0/tools/install/install.bzl#L424-L430
https://github.com/RobotLocomotion/drake/blob/v1.5.0/tools/install/install.bzl#L149-L206
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @mwoehlke-kitware)
bindings/pydrake/BUILD.bazel line 795 at r9 (raw file):
"\"$$stubgen\" --package=pydrake --output=$(RULEDIR)", ]), )
This fixes the CI error, while keeping the import pydrake.all using the target configuration.
--- a/bindings/pydrake/BUILD.bazel
+++ b/bindings/pydrake/BUILD.bazel
@@ -24,6 +24,7 @@ load(
"get_pybind_package_info",
)
load("//bindings/pydrake:pydrake.bzl", "add_lint_tests_pydrake")
+load(":stubgen.bzl", "stubgen")
package(default_visibility = [
"//bindings:__subpackages__",
@@ -784,14 +785,11 @@ PYI_FILES = [
# TODO(mwoehlke-kitware) genrule inappropriately gives execute permission to
# all its outputs; see https://github.com/bazelbuild/bazel/issues/3359.
# (Applies to both :pydrake_pyi and, below, :pydrake_typed.)
-genrule(
+stubgen(
name = "pydrake_pyi",
- srcs = [":stubgen"],
+ tool = ":stubgen",
+ package = "pydrake",
outs = PYI_FILES,
- cmd = "; ".join([
- "stubgen=($(execpaths :stubgen))",
- "\"$$stubgen\" --package=pydrake --output=$(RULEDIR)",
- ]),
)
# PEP 561 marker file; tells tools that type information is available.
diff --git a/bindings/pydrake/stubgen.bzl b/bindings/pydrake/stubgen.bzl
new file mode 100644
index 0000000000..891cc5315f
--- /dev/null
+++ b/bindings/pydrake/stubgen.bzl
@@ -0,0 +1,32 @@
+load("//tools/skylark:pathutils.bzl", "dirname")
+
+def _impl(ctx):
+ outs = ctx.outputs.outs
+ init_pyi = outs[0].path # e.g., "/path/to/pydrake/__init__.pyi"
+ outdir = dirname(dirname(init_pyi)) # e.g., "/path/to/"
+ ctx.actions.run(
+ mnemonic = "GenerateMypyStubs",
+ executable = ctx.executable.tool,
+ arguments = [
+ "--quiet",
+ "--package=" + ctx.attr.package,
+ "--output=" + outdir,
+ ],
+ outputs = outs,
+ )
+
+
+stubgen = rule(
+ implementation = _impl,
+ attrs = {
+ "tool": attr.label(
+ mandatory = True,
+ executable = True,
+ # We use "target" config so that we will use the to-be-installed
+ # pydrake binaries in order to populate the pyi stubs.
+ cfg = "target",
+ ),
+ "package": attr.string(mandatory = True),
+ "outs": attr.output_list(mandatory = True),
+ },
+)bindings/pydrake/BUILD.bazel line 823 at r9 (raw file):
drake_py_unittest( name = "stubgen_test",
This test formulation seems superfluous now -- running stubgen twice seems wasteful. If you think it's important to keep the test condition intact (the check for "getDrakePath"), then we should evaluate it against the already-built stubgen output, instead of re-running it.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @jwnimmer-tri and @mwoehlke-kitware)
bindings/pydrake/BUILD.bazel line 823 at r9 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This test formulation seems superfluous now -- running stubgen twice seems wasteful. If you think it's important to keep the test condition intact (the check for "getDrakePath"), then we should evaluate it against the already-built stubgen output, instead of re-running it.
ERROR: ~/drake/bindings/pydrake/BUILD.bazel:820:18: in deps attribute of py_test rule //bindings/pydrake:py/pyi_sanity_test: '//bindings/pydrake:pydrake_pyi' does not have mandatory providers: 'py' or 'PyInfo'.
That's not great. We probably want to be able to write tests against the .pyis. Thoughts? (BTW, the error is "wrong", I'm using data, not deps.)
|
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Weird, suddenly the error is gone (AFAIK I didn't change anything!), but now the files are just... not there. If I use |
mwoehlke-kitware
force-pushed
the
install-python-stubs
branch
from
de190f2 to
5353492
Compare
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @EricCousineau-TRI and @jwnimmer-tri)
bindings/pydrake/BUILD.bazel line 795 at r9 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
This fixes the CI error, while keeping the
import pydrake.allusing the target configuration.--- a/bindings/pydrake/BUILD.bazel +++ b/bindings/pydrake/BUILD.bazel @@ -24,6 +24,7 @@ load( "get_pybind_package_info", ) load("//bindings/pydrake:pydrake.bzl", "add_lint_tests_pydrake") +load(":stubgen.bzl", "stubgen") package(default_visibility = [ "//bindings:__subpackages__", @@ -784,14 +785,11 @@ PYI_FILES = [ # TODO(mwoehlke-kitware) genrule inappropriately gives execute permission to # all its outputs; see https://github.com/bazelbuild/bazel/issues/3359. # (Applies to both :pydrake_pyi and, below, :pydrake_typed.) -genrule( +stubgen( name = "pydrake_pyi", - srcs = [":stubgen"], + tool = ":stubgen", + package = "pydrake", outs = PYI_FILES, - cmd = "; ".join([ - "stubgen=($(execpaths :stubgen))", - "\"$$stubgen\" --package=pydrake --output=$(RULEDIR)", - ]), ) # PEP 561 marker file; tells tools that type information is available. diff --git a/bindings/pydrake/stubgen.bzl b/bindings/pydrake/stubgen.bzl new file mode 100644 index 0000000000..891cc5315f --- /dev/null +++ b/bindings/pydrake/stubgen.bzl @@ -0,0 +1,32 @@ +load("//tools/skylark:pathutils.bzl", "dirname") + +def _impl(ctx): + outs = ctx.outputs.outs + init_pyi = outs[0].path # e.g., "/path/to/pydrake/__init__.pyi" + outdir = dirname(dirname(init_pyi)) # e.g., "/path/to/" + ctx.actions.run( + mnemonic = "GenerateMypyStubs", + executable = ctx.executable.tool, + arguments = [ + "--quiet", + "--package=" + ctx.attr.package, + "--output=" + outdir, + ], + outputs = outs, + ) + + +stubgen = rule( + implementation = _impl, + attrs = { + "tool": attr.label( + mandatory = True, + executable = True, + # We use "target" config so that we will use the to-be-installed + # pydrake binaries in order to populate the pyi stubs. + cfg = "target", + ), + "package": attr.string(mandatory = True), + "outs": attr.output_list(mandatory = True), + }, +)
Done. I rewrote the brittle inspect-the-outputs logic to instead hard-code my best attempt at the equivalent of $(RULEDIR), which is The Correct Directory.
bindings/pydrake/BUILD.bazel line 823 at r9 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Weird, suddenly the error is gone (AFAIK I didn't change anything!), but now the files are just... not there. If I use
data = [":pydrake:types"], that file shows up as expected, but:pydrake_pyidoes not... 😕
Done. Mind the TODO; ideas welcomed!
There was a problem hiding this comment.
Reviewed 1 of 3 files at r10, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @EricCousineau-TRI and @jwnimmer-tri)
bindings/pydrake/BUILD.bazel line 823 at r9 (raw file):
Previously, mwoehlke-kitware (Matthew Woehlke) wrote…
Done. Mind the TODO; ideas welcomed!
You can fix it as so:
diff --git a/bindings/pydrake/BUILD.bazel b/bindings/pydrake/BUILD.bazel
index 4a7cc98b2d..d695c6bd79 100644
--- a/bindings/pydrake/BUILD.bazel
+++ b/bindings/pydrake/BUILD.bazel
@@ -820,7 +820,7 @@ install(
drake_py_unittest(
name = "pyi_sanity_test",
# TODO(mwoehlke-kitware): Why does ":pydrake_pyi" not work here?
- data = PYI_FILES,
+ data = [":pydrake_pyi"],
deps = ["@bazel_tools//tools/python/runfiles"],
)
diff --git a/bindings/pydrake/stubgen.bzl b/bindings/pydrake/stubgen.bzl
index ef02752931..7c191c3e94 100644
--- a/bindings/pydrake/stubgen.bzl
+++ b/bindings/pydrake/stubgen.bzl
@@ -14,6 +14,10 @@ def _impl(ctx):
],
outputs = ctx.outputs.outs,
)
+ return [DefaultInfo(
+ files = depset(ctx.outputs.outs),
+ data_runfiles = ctx.runfiles(files = ctx.outputs.outs),
+ )]
generate_python_stubs = rule(
implementation = _impl,
mwoehlke-kitware
force-pushed
the
install-python-stubs
branch
from
5353492 to
4b1ae10
Compare
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform) (waiting on @EricCousineau-TRI and @jwnimmer-tri)
bindings/pydrake/BUILD.bazel line 801 at r6 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
OK Thanks!
Done.
bindings/pydrake/BUILD.bazel line 823 at r9 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
You can fix it as so:
diff --git a/bindings/pydrake/BUILD.bazel b/bindings/pydrake/BUILD.bazel index 4a7cc98b2d..d695c6bd79 100644 --- a/bindings/pydrake/BUILD.bazel +++ b/bindings/pydrake/BUILD.bazel @@ -820,7 +820,7 @@ install( drake_py_unittest( name = "pyi_sanity_test", # TODO(mwoehlke-kitware): Why does ":pydrake_pyi" not work here? - data = PYI_FILES, + data = [":pydrake_pyi"], deps = ["@bazel_tools//tools/python/runfiles"], ) diff --git a/bindings/pydrake/stubgen.bzl b/bindings/pydrake/stubgen.bzl index ef02752931..7c191c3e94 100644 --- a/bindings/pydrake/stubgen.bzl +++ b/bindings/pydrake/stubgen.bzl @@ -14,6 +14,10 @@ def _impl(ctx): ], outputs = ctx.outputs.outs, ) + return [DefaultInfo( + files = depset(ctx.outputs.outs), + data_runfiles = ctx.runfiles(files = ctx.outputs.outs), + )] generate_python_stubs = rule( implementation = _impl,
Done. (Yay! 🙂)
|
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-snopt-mosek-release please. Given macOS is currently borked, I suspect we can't very well test those wheels right now. 😢 |
There was a problem hiding this comment.
Reviewed 1 of 3 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @mwoehlke-kitware)
bindings/pydrake/stubgen.bzl line 3 at r11 (raw file):
# -*- python -*- load("//tools/lint:lint.bzl", "add_lint_tests")
nit This line is spurious.
Add rules to generate (as part of the build) and install type stubs for pydrake. Modify wheel build to include these as well. Note that this makes numpy and matplotlib build-time dependencies. (Previously they were only required to test/run pydrake.) Note also that there are issues remaining with the .pyi's; in particular, they are completely unusable by mypy. Co-authored-by: Jeremy Nimmer <[email protected]>
mwoehlke-kitware
force-pushed
the
install-python-stubs
branch
from
4b1ae10 to
5891e52
Compare
|
@drake-jenkins-bot linux-focal-unprovisioned-gcc-wheel-experimental-snopt-mosek-release please. |
|
@drake-jenkins-bot mac-big-sur-unprovisioned-clang-wheel-experimental-snopt-mosek-release please. |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r12, all commit messages.
Reviewable status:
This reverts commit a61a184.
This reverts commit a61a184.
Add rules to generate (as part of the build) and install type stubs for pydrake. Modify wheel build to include these as well.
Note that this makes numpy and matplotlib build-time dependencies. (Previously they were only required to test/run pydrake.)
Note also that there are issues remaining with the .pyi's; in particular, they are completely unusable by mypy.
Toward #16987.
This change is