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

[pydrake] Fix incompatibility with Mypy #18674

Merged
merged 1 commit into from
Jan 30, 2023

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Jan 27, 2023

When parsing the docstrings of a function to find its set of overloads, C++ code samples that mention the function name were at risk of matching the C++ scoping operator :: as a type annotation, leading to malformed output. For example MyFunc(foo : int) -> bool is a valid signature, but the sample code MyFunc(Eigen::Lower) was being mis-parsed. Fix this by patching stubgen to reset in case it sees a double colon.

Enable the stubgen regression for all of pydrake; no more caveats.

Revert da58ae0 and 2cf04fd header file changes, which were work-arounds for this parsing bug and are no longer needed.

Closes #18580.


This change is Reviewable

When parsing the docstrings of a function to find its set of overloads,
C++ code samples that mention the function name were at risk of matching
the C++ scoping operator "::" as a type annotation, leading to malformed
output. For example "MyFunc(foo: int) -> bool" is a valid signature, but
the sample code "MyFunc(Eigen::Lower)" was being mis-parsed. Fix this by
patching stubgen to reset in case it sees a double colon.

Enable the stubgen regression for all of pydrake; no more caveats.

Revert da58ae0 and 2cf04fd header file changes, which were work-arounds
for this parsing bug and are no longer needed.
@jwnimmer-tri jwnimmer-tri added priority: high release notes: fix This pull request contains fixes (no new features) labels Jan 27, 2023
@jwnimmer-tri
Copy link
Collaborator Author

+@SeanCurtis-TRI for feature review, please.

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), needs at least two assigned reviewers (waiting on @jwnimmer-tri and @SeanCurtis-TRI)

a discussion (no related file):
Working

This patch changed a bit more of the pyi output than I expected:
https://gist.github.com/jwnimmer-tri/b391107de9ae4484a3950abc97d1ae39

I'm going to double-check to see if the changes are all improvements.


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), needs at least two assigned reviewers (waiting on @SeanCurtis-TRI)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

This patch changed a bit more of the pyi output than I expected:
https://gist.github.com/jwnimmer-tri/b391107de9ae4484a3950abc97d1ae39

I'm going to double-check to see if the changes are all improvements.

OK I am happy with these changes. All of them are either:

(1) Removing untyped argument names from signatures where the type is a C++ type -- because our bindings are not correctly topologically ordered and the C++ name is leaking into the docs (~90%).

(2) Removing totally buggy and wrong signatures like an argument named std which is really a mis-understood std:: (~10%).


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:

+@ggould-tri for platform review, please.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee ggould-tri(platform) (waiting on @ggould-tri and @jwnimmer-tri)

a discussion (no related file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK I am happy with these changes. All of them are either:

(1) Removing untyped argument names from signatures where the type is a C++ type -- because our bindings are not correctly topologically ordered and the C++ name is leaking into the docs (~90%).

(2) Removing totally buggy and wrong signatures like an argument named std which is really a mis-understood std:: (~10%).

I see. So, the apparent degradation (substitution of parameter names with *args, **kwargs and a return type of Any simply points to defects in the organization of binding. So, fixing those defects will restore the .pyi files to a better state?


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 ggould-tri(platform) (waiting on @ggould-tri)

a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I see. So, the apparent degradation (substitution of parameter names with *args, **kwargs and a return type of Any simply points to defects in the organization of binding. So, fixing those defects will restore the .pyi files to a better state?

Yes, exactly. I have a hope that down the road we'll have a build-system-automated interlock that all Drake C++ types used in Drake function signatures must resolve correctly in the docs.


Copy link
Contributor

@ggould-tri ggould-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 5 of 5 files at r1, all commit messages.
Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)


tools/workspace/mypy_internal/patches/reject_double_colon.patch line 9 at r1 (raw file):

the sample code "MyFunc(Eigen::Lower)" was being mis-parsed. Fix this by
patching stubgen to reset in case it sees a double colon.

minor: Has this been communicated upstream; can this reference an upstream issue number?

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 (waiting on @ggould-tri)


tools/workspace/mypy_internal/patches/reject_double_colon.patch line 9 at r1 (raw file):

Previously, ggould-tri wrote…

minor: Has this been communicated upstream; can this reference an upstream issue number?

My hypothesis is that they would not be interested in a bug report.

The premise of stubgen is that it outputs a starting point, which a human is supposed to review and amend by hand and then commit into source control. Given our scale, we've chosen not to do that.

It's also very unusual (and wrong) to have C++ sample code in Python docstrings (#12591). I don't think most users of stubgen would hit this problem, either.

You could imagine that upstream might accept a pull request (not issue) from us with this patch, but generally my thought is to wait until all of our pydrake documentation epic is done, and then circle back and submit anything upstream. It's plausible that we might change how this all works, so this patch is in some sense "in beta" for a while before we might submit it.

@jwnimmer-tri jwnimmer-tri merged commit 08268a5 into RobotLocomotion:master Jan 30, 2023
@jwnimmer-tri jwnimmer-tri deleted the pydrake-pyi-final branch January 30, 2023 17:16
liangfok added a commit to liangfok/drake that referenced this pull request Jan 31, 2023
xuchenhan-tri pushed a commit to xuchenhan-tri/drake that referenced this pull request Feb 3, 2023
When parsing the docstrings of a function to find its set of overloads,
C++ code samples that mention the function name were at risk of matching
the C++ scoping operator "::" as a type annotation, leading to malformed
output. For example "MyFunc(foo: int) -> bool" is a valid signature, but
the sample code "MyFunc(Eigen::Lower)" was being mis-parsed. Fix this by
patching stubgen to reset in case it sees a double colon.

Enable the stubgen regression for all of pydrake; no more caveats.

Revert da58ae0 and 2cf04fd header file changes, which were work-arounds
for this parsing bug and are no longer needed.
marcoag pushed a commit to marcoag/drake that referenced this pull request Feb 6, 2023
When parsing the docstrings of a function to find its set of overloads,
C++ code samples that mention the function name were at risk of matching
the C++ scoping operator "::" as a type annotation, leading to malformed
output. For example "MyFunc(foo: int) -> bool" is a valid signature, but
the sample code "MyFunc(Eigen::Lower)" was being mis-parsed. Fix this by
patching stubgen to reset in case it sees a double colon.

Enable the stubgen regression for all of pydrake; no more caveats.

Revert da58ae0 and 2cf04fd header file changes, which were work-arounds
for this parsing bug and are no longer needed.
xuchenhan-tri pushed a commit to xuchenhan-tri/drake that referenced this pull request Feb 6, 2023
When parsing the docstrings of a function to find its set of overloads,
C++ code samples that mention the function name were at risk of matching
the C++ scoping operator "::" as a type annotation, leading to malformed
output. For example "MyFunc(foo: int) -> bool" is a valid signature, but
the sample code "MyFunc(Eigen::Lower)" was being mis-parsed. Fix this by
patching stubgen to reset in case it sees a double colon.

Enable the stubgen regression for all of pydrake; no more caveats.

Revert da58ae0 and 2cf04fd header file changes, which were work-arounds
for this parsing bug and are no longer needed.
marcoag pushed a commit to marcoag/drake that referenced this pull request Mar 8, 2023
When parsing the docstrings of a function to find its set of overloads,
C++ code samples that mention the function name were at risk of matching
the C++ scoping operator "::" as a type annotation, leading to malformed
output. For example "MyFunc(foo: int) -> bool" is a valid signature, but
the sample code "MyFunc(Eigen::Lower)" was being mis-parsed. Fix this by
patching stubgen to reset in case it sees a double colon.

Enable the stubgen regression for all of pydrake; no more caveats.

Revert da58ae0 and 2cf04fd header file changes, which were work-arounds
for this parsing bug and are no longer needed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: high release notes: fix This pull request contains fixes (no new features)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incompatibility between pydrake and Mypy
3 participants