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

Add DRAKE_ASSERT statements to ensure programs being solved in parallel are actually threadsafe. #22228

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cohnt
Copy link
Contributor

@cohnt cohnt commented Nov 21, 2024

This change is Reviewable

@cohnt cohnt changed the title Add DRAKE_ASSERT statements to ensure programs being solved in parall… Add DRAKE_ASSERT statements to ensure programs being solved in parallel are actually threadsafe. Nov 21, 2024
@cohnt
Copy link
Contributor Author

cohnt commented Nov 21, 2024

We recently added a couple pieces of code where we solve mathematical programs in parallel, without actually asserting that they're thread-safe. This isn't a problem, since they are thread-safe, but if someone changes these programs down the line and inadvertently makes them not-thread-safe, it could lead to very confusing errors.

+@AlexandreAmice for feature review on this one?

@cohnt cohnt added the release notes: none This pull request should not be mentioned in the release notes label Nov 21, 2024
Copy link
Contributor

@AlexandreAmice AlexandreAmice left a comment

Choose a reason for hiding this comment

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

You want to put the asserts right before solve.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee AlexandreAmice, needs platform reviewer assigned, needs at least two assigned reviewers


geometry/optimization/convex_set.cc line 123 at r1 (raw file):

  for (int i = 0; i < ssize(progs); ++i) {
    ConstructEmptyBoundednessProgram(&(progs[i]), s);
    DRAKE_ASSERT(progs[i].IsThreadSafe());

No need for this one here. Only need to check before calling solve


geometry/optimization/convex_set.cc line 161 at r1 (raw file):

    progs[thread_num].linear_costs()[0].evaluator()->update_coefficient_entry(
        dimension, maximize ? -1 : 1);
    solver_interfaces[thread_num]->Solve(progs[thread_num], std::nullopt,

Put the asserts before Solve

Copy link
Contributor Author

@cohnt cohnt 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: 2 unresolved discussions, LGTM missing from assignee AlexandreAmice, needs platform reviewer assigned, needs at least two assigned reviewers


geometry/optimization/convex_set.cc line 123 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

No need for this one here. Only need to check before calling solve

Done.


geometry/optimization/convex_set.cc line 161 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Put the asserts before Solve

Done.

Copy link
Contributor

@AlexandreAmice AlexandreAmice 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 1 of 1 files at r2.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

2 participants