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.

Sign up for GitHub

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

Jump to bottom

Re-apply "add missing bindings for State<T> abstract values" #11427

Merged
merged 2 commits into from
May 9, 2019
Merged

Re-apply "add missing bindings for State<T> abstract values" #11427

merged 2 commits into from
May 9, 2019

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented May 9, 2019 •
edited
Loading

Re-applies #11416. Relates #11424.

This change is Reviewable

@jwnimmer-tri jwnimmer-tri added priority: high status: do not merge status: commits are properly curated https://drake.mit.edu/reviewable.html#curated-commits labels May 9, 2019
@jwnimmer-tri
Copy link
Collaborator Author

jwnimmer-tri commented May 9, 2019 •
edited
Loading

Ugh, this fix (in its original push) doesn't work anymore, either.

@jwnimmer-tri jwnimmer-tri force-pushed the pydrake-state-heisenbug branch 2 times, most recently from f1d76ff to add4201 Compare May 9, 2019 14:00
RussTedrake and others added 2 commits May 9, 2019 10:01
@RussTedrake @jwnimmer-tri
Add missing bindings for State<T> abstract values
78f9f4b 
(Needed for implementing unrestricted updates from python.)

This re-applies cabd2ab, undoing the
revert in 64fe7e5.
@jwnimmer-tri
pydrake: Revert some bindings of State accessors
8883716 
Under Xenial GCC 5.4 + Python2 + --compilation_mode=dbg only, the
//bindings/pydrake/systems:py/custom_test has about 75% failure rate
when pydrake.systems.framework.State has too many methods bound.

We'll need to root cause this further, but for now this repairs CI,
while stopping short of a full revert.
@jwnimmer-tri jwnimmer-tri force-pushed the pydrake-state-heisenbug branch from add4201 to 8883716 Compare May 9, 2019 14:01
@jwnimmer-tri
Copy link
Collaborator Author

I think this revision works \CC @EricCousineau-TRI @RussTedrake FYI. If CI ends up happy, I'll assign review.

@jwnimmer-tri
Copy link
Collaborator Author

The debug build has succeeded. +@RussTedrake do you want to sign off on this approach being worthwhile for master?

RussTedrake
RussTedrake approved these changes May 9, 2019
View reviewed changes
Copy link
Contributor

@RussTedrake RussTedrake left a comment

Choose a reason for hiding this comment

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

:lgtm: (that's some beautiful code!)
thanks for pushing it through. my only concern is that we still don't know why it failed before...

Reviewed 2 of 2 files at r2.
Reviewable status: needs at least two assigned reviewers, labeled "do not merge"

@jwnimmer-tri
Copy link
Collaborator Author

I guess we'll keep pounding on #11424 to root cause it. Hopefully this PR doesn't cause any ongoing trouble. If so, we'll just have to revert it again.

+@ggould-tri for platform review, please.

ggould-tri
ggould-tri approved these changes May 9, 2019
View reviewed changes
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.

... looks sane to me. :lgtm: fingers crossed

Reviewed 2 of 2 files at r2.
Reviewable status: labeled "do not merge"

@jwnimmer-tri jwnimmer-tri merged commit 703768e into RobotLocomotion:master May 9, 2019
@jwnimmer-tri jwnimmer-tri deleted the pydrake-state-heisenbug branch May 9, 2019 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ggould-tri ggould-tri ggould-tri approved these changes

@RussTedrake RussTedrake RussTedrake approved these changes

Assignees

@RussTedrake RussTedrake

@ggould-tri ggould-tri

Labels
priority: medium status: commits are properly curated https://drake.mit.edu/reviewable.html#curated-commits
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jwnimmer-tri @RussTedrake @ggould-tri