GurobiSolver acquires a local license at most once per process #19713
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RussTedrake
commented
Jun 30, 2023
There was a problem hiding this comment.
@jwnimmer-tri ... i could use some advice on this one.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, missing label for release notes (waiting on @RussTedrake)
solvers/gurobi_solver.cc line 1098 at r1 (raw file):
hold on to the license beyond the lifetime of the GurobiSolver iff we can confirm that the license is associated with the local host. */ std::shared_ptr<GurobiSolver::License> local_host_gurobi_license{};
i'm not sure if this is legal. i read over https://drake.mit.edu/styleguide/cppguide.html#Static_and_Global_Variables and know that the GetScopedSingleton holds the weak_ptr with never_destroyed. is that enough?
solvers/test/gurobi_solver_grb_license_file_test.cc line 84 at r1 (raw file):
TEST_F(GrbLicenseFileTest, LocalLicenseFile) {
This was my first idea for the testing strategy. But it does not work for two reasons:
- the license file acquired by an earlier test will continue to be used here.
- if I do get gurobi to use this license file, then Solve will throw (since it's not a valid license).
Might you have any suggestions?
jwnimmer-tri
reviewed
Jul 1, 2023
There was a problem hiding this comment.
Reviewed 1 of 4 files at r1, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @RussTedrake)
-- commits line 2 at r1:
BTW There might be a more direct phrasing for users to grok. Maybe:
Suggestion:
GurobiSolver acquires a license at most once per process
-- commits line 4 at r1:
typo
Suggestion:
contains
solvers/BUILD.bazel line 1247 at r1 (raw file):
":gurobi_solver", ":mathematical_program", "//common:temp_directory",
nit Oops, I should have reverted this line as well in my patch.
solvers/gurobi_solver.h line 174 at r1 (raw file):
* license server). * @return A shared pointer to a license environment that will stay valid as * long as any shared_ptr returned by this function is alive. If Gurobi not
BTW This unearthed a lingering typo.
Suggestion:
If Gurobi is notsolvers/gurobi_solver.cc line 1098 at r1 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
i'm not sure if this is legal. i read over https://drake.mit.edu/styleguide/cppguide.html#Static_and_Global_Variables and know that the GetScopedSingleton holds the weak_ptr with never_destroyed. is that enough?
solvers/test/gurobi_solver_grb_license_file_test.cc line 84 at r1 (raw file):
Previously, RussTedrake (Russ Tedrake) wrote…
This was my first idea for the testing strategy. But it does not work for two reasons:
- the license file acquired by an earlier test will continue to be used here.
- if I do get gurobi to use this license file, then Solve will throw (since it's not a valid license).
Might you have any suggestions?
jwnimmer-tri
reviewed
Jul 1, 2023
There was a problem hiding this comment.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @RussTedrake)
solvers/test/gurobi_solver_grb_license_file_test.cc line 84 at r1 (raw file):
Ah, I'd also meant to explicitly discuss the thought process here...
When unit testing process-wide state (static variables), generally the best approach is to write a tiny C++ program that interacts with the state and then a Python driver that repeatedly launches the C++ program, so that each test is obviously guaranteed to be independent.
- the license file acquired by an earlier test will continue to be used here.
Hmm. Everything passes for me, but I wonder if the GrbLicenseFileUnset test case will fail now for users with host-local license. If so, then perhaps these two test cases should probably be ported to live in Python also.
RussTedrake
force-pushed
the
gurobi_license_never_destroyed
branch
3 times, most recently
from
fe4e54d to
59ff383
Compare
RussTedrake
commented
Jul 2, 2023
There was a problem hiding this comment.
Now that this is ready, what would you like for our review strategy? Can you and I play cross-platform review? Should we seek any more?
Reviewable status: 2 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers, missing label for release notes (waiting on @RussTedrake)
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW There might be a more direct phrasing for users to grok. Maybe:
but that is only true for local licenses. I started like that, but word-smithed it down to something that was true and fit in the github character limit.
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
typo
Done,.
solvers/BUILD.bazel line 1247 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
nit Oops, I should have reverted this line as well in my patch.
Done.
solvers/gurobi_solver.h line 174 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW This unearthed a lingering typo.
Done.
solvers/gurobi_solver.cc line 1098 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Accepted. Thanks!
solvers/test/gurobi_solver_grb_license_file_test.cc line 84 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Ah, I'd also meant to explicitly discuss the thought process here...
When unit testing process-wide state (static variables), generally the best approach is to write a tiny C++ program that interacts with the state and then a Python driver that repeatedly launches the C++ program, so that each test is obviously guaranteed to be independent.
- the license file acquired by an earlier test will continue to be used here.
Hmm. Everything passes for me, but I wonder if the
GrbLicenseFileUnsettest case will fail now for users with host-local license. If so, then perhaps these two test cases should probably be ported to live in Python also.
Makes sense, and the tests all look good. Thanks!
RussTedrake
added
the
release notes: feature
|
fwiw -- before it was |
jwnimmer-tri
reviewed
Jul 2, 2023
There was a problem hiding this comment.
Now that this is ready, what would you like for our review strategy? Can you and I play cross-platform review? Should we seek any more?
I think we share co-authorship and co-feature-review. I Think this still needs a platform reviewer, to make sure the code is clear to someone not in the trench with us.
Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: 3 unresolved discussions, LGTM missing from assignee jwnimmer-tri(platform), needs at least two assigned reviewers
Previously, RussTedrake (Russ Tedrake) wrote…
but that is only true for local licenses. I started like that, but word-smithed it down to something that was true and fit in the github character limit.
I was thinking the commit body could explain the caveat, to the extent the subject line can't be completely precise.
Anyway, adding "local" in the subject like "GurobiSolver acquires a local license at most once per process" would be a good clue too.
Here's the defect I was shooting for (should have said this originally, instead of only a suggestion): I don't think the word "destroy" will be descriptive at all to any user.
(We could say "never relinquishes" but I still think the affirmative phrasing is best.)
solvers/gurobi_solver.h line 169 at r2 (raw file):
; see has_acquired_local_license().
This part is out of date now.
solvers/gurobi_solver.cc line 1114 at r2 (raw file):
? GetScopedSingleton<GurobiSolver::License>() : nullptr; }()};
BTW Re-reading this now, I noticed we probably don't need a lambda, since the body of the lambda is just a return statement.
Suggestion:
static never_destroyed<std::shared_ptr<void>> local_host_holder{
IsGrbLicenseFileLocalHost()
? GetScopedSingleton<GurobiSolver::License>()
: nullptr};solvers/test/gurobi_solver_grb_license_file_test.cc line 84 at r1 (raw file):
I wonder if the
GrbLicenseFileUnsettest case will fail now for users with host-local license.
I guess its still passing for you? That's good news. (I can't test with a local license.)
RussTedrake
force-pushed
the
gurobi_license_never_destroyed
branch
2 times, most recently
from
4e3f47f to
f0e4898
Compare
RussTedrake
commented
Jul 2, 2023
There was a problem hiding this comment.
+@jwnimmer-tri for feature review.
+@RussTedrake for feature review
+@sherm1 for platform review
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),sherm1(platform)
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I was thinking the commit body could explain the caveat, to the extent the subject line can't be completely precise.
Anyway, adding "local" in the subject like "GurobiSolver acquires a local license at most once per process" would be a good clue too.
Here's the defect I was shooting for (should have said this originally, instead of only a suggestion): I don't think the word "destroy" will be descriptive at all to any user.
(We could say "never relinquishes" but I still think the affirmative phrasing is best.)
Done.
solvers/gurobi_solver.h line 169 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
; see has_acquired_local_license().
This part is out of date now.
Done.
solvers/gurobi_solver.cc line 1114 at r2 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW Re-reading this now, I noticed we probably don't need a lambda, since the body of the lambda is just a return statement.
Done.
solvers/test/gurobi_solver_grb_license_file_test.cc line 84 at r1 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I wonder if the
GrbLicenseFileUnsettest case will fail now for users with host-local license.I guess its still passing for you? That's good news. (I can't test with a local license.)
Correct.
jwnimmer-tri
reviewed
Jul 3, 2023
There was a problem hiding this comment.
Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignees jwnimmer-tri(platform),sherm1(platform)
solvers/gurobi_solver.h line 174 at r3 (raw file):
Otherwise the default behavior is to release the license after each Solve().
This is too-easily misinterpreted.
It's true that each call to solvers::Solve() acquires and relinquishes the license (for a non-localhost license).
However, repeated calls to gurobi_solver.Solve() on the same instance of GurobiSolver do not relinquish the license afterward. The license is a member variable.
RussTedrake
force-pushed
the
gurobi_license_never_destroyed
branch
from
f0e4898 to
385f318
Compare
RussTedrake
commented
Jul 3, 2023
There was a problem hiding this comment.
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),sherm1(platform)
solvers/gurobi_solver.h line 174 at r3 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Otherwise the default behavior is to release the license after each Solve().
This is too-easily misinterpreted.
It's true that each call to
solvers::Solve()acquires and relinquishes the license (for a non-localhost license).However, repeated calls to
gurobi_solver.Solve()on the same instance ofGurobiSolverdo not relinquish the license afterward. The license is a member variable.
Done. you're right. I've tried again.
|
@drake-jenkins-bot mac-arm-ventura-clang-bazel-experimental-release please. |
RussTedrake
commented
Jul 4, 2023
There was a problem hiding this comment.
@jwnimmer-tri -- this needs an LGTM from you when you're ready.
Reviewable status: LGTM missing from assignees jwnimmer-tri(platform),sherm1(platform)
jwnimmer-tri
reviewed
Jul 5, 2023
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: LGTM missing from assignee sherm1(platform)
jwnimmer-tri
changed the title
sherm1
reviewed
Jul 5, 2023
There was a problem hiding this comment.
Platform
Reviewed 4 of 6 files at r2, 1 of 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: 2 unresolved discussions
-- commits line 5 at r4:
typo: keywork -> keyword
solvers/gurobi_solver.h line 170 at r4 (raw file):
* method on each Solve(). * * If the license file contains the string `HOST_ID`, then we treat this as
typo: HOST_ID -> HOSTID
Iff the contents of the file identified in GRB_LICENSE_FILE contains the magic keyword HOSTID, then any license that is obtained will never be destroyed. Resolves RobotLocomotion#19657. A new implementation and unit test strategy
RussTedrake
force-pushed
the
gurobi_license_never_destroyed
branch
from
385f318 to
e5c7068
Compare
RussTedrake
commented
Jul 5, 2023
There was a problem hiding this comment.
Reviewable status:
complete! all discussions resolved, LGTM from assignees jwnimmer-tri(platform),sherm1(platform),RussTedrake(platform)
Previously, sherm1 (Michael Sherman) wrote…
typo: keywork -> keyword
Done.
solvers/gurobi_solver.h line 170 at r4 (raw file):
Previously, sherm1 (Michael Sherman) wrote…
typo: HOST_ID -> HOSTID
Done.
sherm1
reviewed
Jul 5, 2023
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status:
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Iff the contents of the file identified in GRB_LICENSE_FILE contain the magic keywork HOSTID, then any license that is obtained will never be destroyed.
Resolves #19657.
This change is