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

[solvers] Deprecate dReal and IBEX for removal #18156

Merged

Conversation

jwnimmer-tri
Copy link
Collaborator

@jwnimmer-tri jwnimmer-tri commented Oct 21, 2022

Deprecate the classes DrealSolver, IbexSolver, and their nested helper classes.
Deprecate the workspace externals cds, dreal, ibex, and picosat.


This pull request is a proposal of an idea bouncing around for a few months. I'll leave it open for comments until 2022-10-31 at which point we'll plan to merge it (unless we hear any objections). Feel free to reply in this issue, or Drake slack, or send me an email.

There are two thoughts behind this proposal.

(1) It is becoming increasingly difficult to keep up with the build and release tasks involved with distributing dReal and IBEX (#15872, #15970, #16882, #17231, #17869, etc.).

The code and its dependencies offers a few unique challenges -- LGPL licensing, x86 assembly code, and a very finicky version compatibility matrix across patch-level releases for multiple packages. As we broaden our support for ARM64-compatible binary releases, and fully-linked libraries (e.g., for PyPI), those all have been posing challenges for the build team. We can probably overcome all of those challenges by throwing more time and money the the problem. However, given the next point below, my current hypothesis is that it's not worth it.

(2) Talking to users who have tried to use Drake's IBEX and/or dReal support, the general consensus is that it's implausibly slow to run for anything larger than toy-sized problems. (Sometimes, even toy-sized problems were unreasonably slow.) We don't think that anyone has successfully used Drake's wrappers of those tools for any meaningful research problem.

I think there's probably still a great frontier for research here, but I think that work can happen external to the Drake source tree.


This change is Reviewable

Copy link
Contributor

@mpetersen94 mpetersen94 left a comment

Choose a reason for hiding this comment

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

:lgtm: I have no need for IBEX at this point.

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge"

@jwnimmer-tri
Copy link
Collaborator Author

+@rpoyner-tri for build system & deprecation annotations feature review, please.

Copy link
Contributor

@rpoyner-tri rpoyner-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 4 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee rpoyner-tri(platform), needs at least two assigned reviewers, labeled "do not merge"

@jwnimmer-tri
Copy link
Collaborator Author

-(status: do not merge)

Copy link
Contributor

@rpoyner-tri rpoyner-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 18 of 22 files at r1, 3 of 4 files at r2.
Reviewable status: needs at least two assigned reviewers (waiting on @jwnimmer-tri)

@jwnimmer-tri
Copy link
Collaborator Author

+@SeanCurtis-TRI for platform review per schedule, please.

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:

With a couple of questions...the universe apparently doesn't quite work like I think it does.

Reviewed 18 of 22 files at r1, 4 of 4 files at r2, all commit messages.
Reviewable status: 2 unresolved discussions (waiting on @jwnimmer-tri)


solvers/dreal_solver.h line 82 at r2 (raw file):

  DRAKE_NO_COPY_NO_MOVE_NO_ASSIGN(DrealSolver)

  DRAKE_DEPRECATED("2023-02-01",

BTW Given the class itself is deprecated, why flag the default constructor specifically?


solvers/solver_type_converter.cc line 89 at r2 (raw file):

  } else if (solver_id == UnrevisedLemkeSolverId::id()) {
    return SolverType::kUnrevisedLemke;
  } else if (solver_id == DrealSolver::id()) {

BTW So, the enum required a pragma push, but calling these deprecated static methods don't?

Deprecate the classes DrealSolver and IbexSolver, and their SolverType
enum values.

Deprecate the workspace externals cds, dreal, ibex, and picosat.
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 rpoyner-tri(platform),SeanCurtis-TRI(platform) (waiting on @rpoyner-tri and @SeanCurtis-TRI)


solvers/dreal_solver.h line 82 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Given the class itself is deprecated, why flag the default constructor specifically?

Consider this example:

class [[deprecated("Yeet")]] Foo {
 public:
  Foo() = default;
  int getter() const { return 4; }
};

int stuff() {
  return Foo{}.getter();
}

The function stuff() does not generate any warnings (tested with gcc 9.5).

A deprecation on a class only triggers for use of that class as a typename. It does not trigger on temporary objects.


solvers/solver_type_converter.cc line 89 at r2 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW So, the enum required a pragma push, but calling these deprecated static methods don't?

Done.

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 2 files at r3.
Reviewable status: :shipit: complete! all discussions resolved, LGTM from assignees rpoyner-tri(platform),SeanCurtis-TRI(platform) (waiting on @rpoyner-tri)


solvers/dreal_solver.h line 82 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Consider this example:

class [[deprecated("Yeet")]] Foo {
 public:
  Foo() = default;
  int getter() const { return 4; }
};

int stuff() {
  return Foo{}.getter();
}

The function stuff() does not generate any warnings (tested with gcc 9.5).

A deprecation on a class only triggers for use of that class as a typename. It does not trigger on temporary objects.

Interesting. Thanks for the info.

It makes me think we need a deprecation playbook. I don't think we've classically done this in deprecating classes.

@jwnimmer-tri jwnimmer-tri merged commit b034166 into RobotLocomotion:master Nov 1, 2022
@jwnimmer-tri jwnimmer-tri deleted the deprecate-dreal-ibex branch November 1, 2022 13:24
@jwnimmer-tri jwnimmer-tri mentioned this pull request Jan 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: low release notes: newly deprecated This pull request contains new deprecations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants