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

Improved support for Gurobi environments #2370

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jmarecek
Copy link
Contributor

@jmarecek jmarecek commented Mar 5, 2024

Description

Gurobi is a commercial solver, with an academic license, trial license, etc. Three years ago, @sjschneider added support for instatiating the model with a custom environment, which would have the license details pre-loaded. It is non-obvious from the documentation how to set the environment up, though.

This pull request fixes gurobi_conif.py and gurobi_qpif.py such that even without specifying the environment, the license is loaded from the correct environment variables.

Type of change

  • New feature (backwards compatible)
  • New feature (breaking API changes)
  • Bug fix
  • Other (Documentation, CI, ...)

Contribution checklist

  • Add our license to new files.
  • Check that your code adheres to our coding style.
  • Write unittests.
  • Run the unittests and check that they’re passing.
  • Run the benchmarks to make sure your change doesn’t introduce a regression.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ SteveDiamond
❌ jmarecek-cez
You have signed the CLA already but the status is still pending? Let us recheck it.

11: s.SOLVER_ERROR,
12: s.SOLVER_ERROR,
13: s.SOLVER_ERROR}
7: s.USER_LIMIT, # ITERATION_LIMIT
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused about why this is a delta. This change should be in master. Maybe you need to sync with master?

or license_project is None
or license_expiration_date is None
or license_code is None):
print("Note: License parameters can be provided in enviroment "
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not print if verbose=False. Also I don't understand the meaning of the message.

# Create Gurobi model using default (unspecified) environment
model = grb.Model()
# Alternatively, reads the license off the environment variables
log_file = os.getenv("GUROBI_LOG_FILE", "gurobi.log")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you factor out/clean up this code somehow?

@SteveDiamond
Copy link
Collaborator

Can you explain with a more concrete example what this PR is enabling?

@sjschneider
Copy link
Contributor

sjschneider commented Mar 5, 2024

I think it's bad form to have cvxpy read in environment variables like this. To the extent the mechanism for providing the Gurobi Environment was unclear, I think this code belongs as just an example.

I should also note that the Gurobi Env API has not been static and so this puts an additional burden on cvxpy for a specific license type that will not be easy to test.

@phschiele
Copy link
Collaborator

I think it's bad form to have cvxpy read in environment variables like this. To the extent the mechanism for providing the Gurobi Environment was unclear, I think this code belongs as just an example.

I should also note that the Gurobi Env API has not been static and so this puts an additional burden on cvxpy for a specific license type that will not be easy to test.

Considering the concerns about testing, do we want to proceed with this PR?
I'd prefer simply adding a line to the docs that shows the syntax for using the (existing) environment feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants