Parallelize the Rounding Stage of GCS #22222

cohnt · 2024-11-20T21:21:48Z

This ended up being pretty simple -- solve all the restrictions in a parallel for loop, and override options.parallelism to set the number of threads to one if any not-thread-safe costs or constraints are included in the GCS.

Gonna test this with the various debug builds before I send it up for review.

This change is

…e done.

… are included in the GCS.

cohnt · 2024-11-21T03:33:08Z

@drake-jenkins-bot linux-jammy-clang-bazel-experimental-thread-sanitizer please
@drake-jenkins-bot linux-jammy-clang-bazel-experimental-valgrind-memcheck please
@drake-jenkins-bot linux-jammy-clang-bazel-experimental-undefined-behavior-sanitizer please
@drake-jenkins-bot linux-jammy-clang-bazel-experimental-address-sanitizer please

AlexandreAmice

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @cohnt)

geometry/optimization/graph_of_convex_sets.cc line 1618 at r1 (raw file):

    // If any costs or constraints aren't thread-safe, we can't parallelize.
    bool is_thread_safe = true;

Don't forget to set options.kMaxThreads = 1 and make sure to warn if is_thread_safe == false && parallelism.num_threads() > 1, otherwise users might be surprised if the extra threads aren't used.

…ts or constraints. Also make sure only one thread is used per solver.

cohnt

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @cohnt)

geometry/optimization/graph_of_convex_sets.cc line 1618 at r1 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Don't forget to set options.kMaxThreads = 1 and make sure to warn if is_thread_safe == false && parallelism.num_threads() > 1, otherwise users might be surprised if the extra threads aren't used.

Done.

cohnt · 2024-11-30T22:31:05Z

+@RussTedrake for feature review, if you have the time?

RussTedrake

we'll need some test coverage.

Reviewed 1 of 2 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @cohnt)

geometry/optimization/graph_of_convex_sets.cc line 1662 at r2 (raw file):

    if (n_threads_to_use < options.parallelism.num_threads()) {
      log()->warn(
          "GCS restriction has costs or constraints which are not thread-safe, "

perhaps "not declared thread-safe"? i suspect most are thread safe, but might need to be marked appropriately since we have to assume the worst.

geometry/optimization/graph_of_convex_sets.cc line 1690 at r2 (raw file):

    using common_robotics_utilities::parallelism::DegreeOfParallelism;
    using common_robotics_utilities::parallelism::DynamicParallelForIndexLoop;
    using common_robotics_utilities::parallelism::ParallelForBackend;

btw -- i guess i thought that drake/common/parallelism.h was supposed to shield us from using CRU parallelism directly. I guess you need more?

geometry/optimization/graph_of_convex_sets.cc line 1696 at r2 (raw file):

    DynamicParallelForIndexLoop(
        DegreeOfParallelism(n_threads_to_use), 0, ssize(candidate_paths),
        solve_ith_convex_restriction, ParallelForBackend::BEST_AVAILABLE);

this whole glob of code is conceptually separable from the rest of SolveShortestPath, and it's adding a LOT of lines to an already enormous method. What do you think about pulling it out into it's own method? It might even be useful by itself? (e.g. SamplePaths got pulled out for this reason).

cohnt

What would test coverage look like? Since the default is to use parallelism, we already have a ton of test cases that verify that parallelism works. I guess I could pick one of the test cases to run with and without parallelism, and verify that the solution is the same?

Reviewable status: 4 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @cohnt)

geometry/optimization/graph_of_convex_sets.cc line 1662 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

perhaps "not declared thread-safe"? i suspect most are thread safe, but might need to be marked appropriately since we have to assume the worst.

Done.

geometry/optimization/graph_of_convex_sets.cc line 1690 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

btw -- i guess i thought that drake/common/parallelism.h was supposed to shield us from using CRU parallelism directly. I guess you need more?

I was trying to work within the current program structure, but now that I'm refactoring, I've been able to remove the CRU usage.

geometry/optimization/graph_of_convex_sets.cc line 1696 at r2 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

this whole glob of code is conceptually separable from the rest of SolveShortestPath, and it's adding a LOT of lines to an already enormous method. What do you think about pulling it out into it's own method? It might even be useful by itself? (e.g. SamplePaths got pulled out for this reason).

I've done some refactoring to clean this up. Do you like this better?

RussTedrake

Yes, that seems reasonable. Philosophically, if you are adding a new feature (especially one that changes this much code), then we want to make sure we have coverage.

Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @cohnt)

geometry/optimization/graph_of_convex_sets.cc line 1696 at r2 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

I've done some refactoring to clean this up. Do you like this better?

yes. thank you!

geometry/optimization/graph_of_convex_sets.cc line 1662 at r3 (raw file):

    }

    std::optional<solvers::SolverId> maybe_solver_id = std::nullopt;

btw -- do you worry about calling ChooseBestSolver for every solve if solver_id is not set here? I guess it's necessary in the current workflow, since there could be one program in progs that e.g. visits a vertex whom no other program visits which has a different (e.g. nonconvex) program.

geometry/optimization/graph_of_convex_sets.h line 883 at r3 (raw file):

  // Add results for additional variables in `result` to make it comparable with
  // other transcriptions.
  void SubstituteAdditionalVariables(

The prefix "Substitute" here confused me, since you have placeholder variable substitution happening in various places. Can we find a more specific name? This is only to make the convex restriction result look more like the MIP result?

fysa, in PRs like #21833, I've actually been trying to do less of this writing of extra stuff into the solution results, and instead making GetSolutionCost and friends more robust to missing information.

cohnt

Added this for one of the test cases that does rounding.

Dismissed @RussTedrake from a discussion.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @cohnt)

geometry/optimization/graph_of_convex_sets.h line 883 at r3 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

The prefix "Substitute" here confused me, since you have placeholder variable substitution happening in various places. Can we find a more specific name? This is only to make the convex restriction result look more like the MIP result?

fysa, in PRs like #21833, I've actually been trying to do less of this writing of extra stuff into the solution results, and instead making GetSolutionCost and friends more robust to missing information.

How about this? A little verbose, but very specific.

geometry/optimization/graph_of_convex_sets.cc line 1662 at r3 (raw file):

Previously, RussTedrake (Russ Tedrake) wrote…

btw -- do you worry about calling ChooseBestSolver for every solve if solver_id is not set here? I guess it's necessary in the current workflow, since there could be one program in progs that e.g. visits a vertex whom no other program visits which has a different (e.g. nonconvex) program.

For the reason you described, I think we have to call ChooseBestSolver each time. Not sure what the overhead looks like.

Presumably, the best practice is to set this solver if it's known -- do you want me to document that in GraphOfConvexSetsOptions?

AlexandreAmice

Reviewable status: 3 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @cohnt)

geometry/optimization/graph_of_convex_sets.cc line 1662 at r3 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

For the reason you described, I think we have to call ChooseBestSolver each time. Not sure what the overhead looks like.

Presumably, the best practice is to set this solver if it's known -- do you want me to document that in GraphOfConvexSetsOptions?

FWIW, ChooseBestSolver is pretty fast since it is just a big lookup table. If you're worried about the cost of repeatedly constructing the best solver, then SolveInParallel already takes care of that by caching the constructed solvers as it solves.

geometry/optimization/graph_of_convex_sets.cc line 1653 at r4 (raw file):

          "thread-safe. Any paths being considered in the rounding process "
          "that contain these constraints will be solved sequentially.");
    }

Maybe only warn once in case the GCS is reused to avoid console spam.

Code quote:

    if (!is_thread_safe) {
      log()->warn(
          "GCS restriction has costs or constraints which are not declared "
          "thread-safe. Any paths being considered in the rounding process "
          "that contain these constraints will be solved sequentially.");
    }

cohnt

Reviewable status: 3 unresolved discussions, LGTM missing from assignee RussTedrake(platform), needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes

geometry/optimization/graph_of_convex_sets.cc line 1653 at r4 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

Maybe only warn once in case the GCS is reused to avoid console spam.

Done.

RussTedrake

+@rpoyner-tri for platform review, please.

Reviewed 2 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee rpoyner-tri(platform), labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @cohnt)

geometry/optimization/graph_of_convex_sets.h line 883 at r3 (raw file):

Previously, cohnt (Thomas Cohn) wrote…

How about this? A little verbose, but very specific.

I like it.

geometry/optimization/graph_of_convex_sets.cc line 1662 at r3 (raw file):

Previously, AlexandreAmice (Alexandre Amice) wrote…

FWIW, ChooseBestSolver is pretty fast since it is just a big lookup table. If you're worried about the cost of repeatedly constructing the best solver, then SolveInParallel already takes care of that by caching the constructed solvers as it solves.

Nice.

rpoyner-tri

+(release notes: fix)

Reviewed 1 of 2 files at r1, 1 of 3 files at r3, 2 of 3 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: 1 unresolved discussion, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

cohnt

-(release notes: fix)
+(release notes: feature)

Dismissed @RussTedrake from a discussion.
Reviewable status: labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits) (waiting on @cohnt)

cohnt added 3 commits November 20, 2024 16:06

Rewrite to have a single for loop where all the restriction solves ar…

476b926

…e done.

Run the restriction solves in parallel.

09ab209

Don't use parallelization if costs/constraints that aren't threadsafe…

6b5ffb3

… are included in the GCS.

cohnt added status: do not merge status: do not review labels Nov 20, 2024

cohnt removed the status: do not review label Nov 21, 2024

AlexandreAmice reviewed Nov 21, 2024

View reviewed changes

Warn if the user requests multithreading, but has non-thread-safe cos…

de65fa1

…ts or constraints. Also make sure only one thread is used per solver.

cohnt commented Nov 21, 2024

View reviewed changes

AlexandreAmice mentioned this pull request Nov 21, 2024

Provide ability to call SolveInParallel with program generators. #22226

Open

cohnt assigned RussTedrake Nov 30, 2024

RussTedrake reviewed Dec 1, 2024

View reviewed changes

cohnt added 4 commits December 2, 2024 14:26

Refactor to simplify SolveConvexRestriction.

7bc52a9

No longer use SolveMainProgram in SolveConvexRestriction.

b301979

Refactor to use SolveInParallel.

f7a2ce2

Remove parallelism import since it's no longer needed.

2155db1

cohnt commented Dec 2, 2024

View reviewed changes

RussTedrake reviewed Dec 3, 2024

View reviewed changes

cohnt added 2 commits December 3, 2024 10:48

New name for method.

8a90022

Rudimentary test coverage for parallelized rounding.

6942ff6

cohnt commented Dec 3, 2024

View reviewed changes

AlexandreAmice reviewed Dec 3, 2024

View reviewed changes

Only warn once for non-thread-safe costs/constraints.

4eb5b36

cohnt commented Dec 3, 2024

View reviewed changes

RussTedrake assigned rpoyner-tri Dec 3, 2024

RussTedrake reviewed Dec 3, 2024

View reviewed changes

rpoyner-tri added the release notes: fix This pull request contains fixes (no new features) label Dec 3, 2024

rpoyner-tri reviewed Dec 3, 2024

View reviewed changes

cohnt added release notes: feature This pull request contains a new feature and removed release notes: fix This pull request contains fixes (no new features) labels Dec 3, 2024

cohnt commented Dec 3, 2024

View reviewed changes

cohnt added status: squashing now https://drake.mit.edu/reviewable.html#curated-commits and removed status: do not merge labels Dec 3, 2024

cohnt merged commit e03af33 into RobotLocomotion:master Dec 3, 2024
10 checks passed

cohnt deleted the gcs-rounding-parallel branch December 3, 2024 21:10

RussTedrake pushed a commit to RussTedrake/drake that referenced this pull request Dec 15, 2024

Parallelize the Rounding Stage of GCS (RobotLocomotion#22222)

455795d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parallelize the Rounding Stage of GCS #22222

Parallelize the Rounding Stage of GCS #22222

cohnt commented Nov 20, 2024 •

edited by sherm1

Loading

cohnt commented Nov 21, 2024

AlexandreAmice left a comment

cohnt left a comment

cohnt commented Nov 30, 2024

RussTedrake left a comment

cohnt left a comment

RussTedrake left a comment

cohnt left a comment

AlexandreAmice left a comment

cohnt left a comment

RussTedrake left a comment

rpoyner-tri left a comment

cohnt left a comment

Parallelize the Rounding Stage of GCS #22222

Parallelize the Rounding Stage of GCS #22222

Conversation

cohnt commented Nov 20, 2024 • edited by sherm1 Loading

cohnt commented Nov 21, 2024

AlexandreAmice left a comment

Choose a reason for hiding this comment

cohnt left a comment

Choose a reason for hiding this comment

cohnt commented Nov 30, 2024

RussTedrake left a comment

Choose a reason for hiding this comment

cohnt left a comment

Choose a reason for hiding this comment

RussTedrake left a comment

Choose a reason for hiding this comment

cohnt left a comment

Choose a reason for hiding this comment

AlexandreAmice left a comment

Choose a reason for hiding this comment

cohnt left a comment

Choose a reason for hiding this comment

RussTedrake left a comment

Choose a reason for hiding this comment

rpoyner-tri left a comment

Choose a reason for hiding this comment

cohnt left a comment

Choose a reason for hiding this comment

cohnt commented Nov 20, 2024 •

edited by sherm1

Loading