-
Notifications
You must be signed in to change notification settings - Fork 41
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
Choose solver, add back V3 CI tests #565
Conversation
@@ -117,7 +117,8 @@ docs/formulation/*log | |||
/.werf_secret_key | |||
|
|||
# Customized solver setup and docker files | |||
julia_src/xpress/licenseserver | |||
julia_src/xpress/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this directory should no longer exist, correct? Is this a check just in case someone hasn't deleted it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should no longer exist, but I'd rather keep this here as people transition over, just in case.
julia_src/http.jl
Outdated
# Load Xpress only if it is installed, as indicated by ENV["XPRESS_INSTALLED"]="True" | ||
xpress_installed = get(ENV, "XPRESS_INSTALLED", "False") | ||
if xpress_installed == "True" | ||
import Xpress # Why not "using Xpress"? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to "using Xpress", and tested with V2 and V3
…hen not installed
@@ -227,7 +227,7 @@ def test_superset_input_fields(self): | |||
r = json.loads(resp.content) | |||
results = r["outputs"] | |||
|
|||
self.assertAlmostEqual(results["Financial"]["npv"], 11323.01, places=0) | |||
self.assertAlmostEqual(results["Financial"]["npv"], -258533.19, places=-3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did just changing the solver change the result by this much?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, no, it's because I had to fix all of the technology sizes to avoid a timeout with the open source solver. Fixing the sizes still tests the capabilities here, which also implies the NPV test is not needed.
@@ -27,14 +27,12 @@ jobs: | |||
NREL_DEV_API_KEY: ${{ secrets.NREL_DEV_API_KEY }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider renaming line 1 of this file and push_tests.yml
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
|
Good catch and thanks for testing. I plan to go over this in the dev meeting tomorrow, and good to know we should just delete licenseserver and re-do those instructions. |
Settings.solver_name
, and useful error messages if trying to use Xpress without installationtest_job_endpoint.py
tests to use an open source solver so we can have full V3 CI testing again