Issue 435: run_solver.py re-structuring and hipo inclusion#454
Issue 435: run_solver.py re-structuring and hipo inclusion#454finozzifa wants to merge 65 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…nergy-transition/solver-benchmark into issue_435_run_solver_hipo
siddharth-krishna
left a comment
There was a problem hiding this comment.
Thanks Fabrizio! I notice one potential issue: reference benchmarks should be run with the reference Highs binary, not HiPO. Let me know if you'd like to discuss this or have any questions.
I would also like to gently encourage future PRs to be smaller / focused on a single logical change when possible. In particular, mixing refactoring with logic changes makes the PR larger and harder to review. If you agree, let's do it that way in the future please.
| reference_benchmark : bool | ||
| If True, appends the ``--highs_solver_variant hipo`` flag to run | ||
| the HiGHS HiPO variant on a reference instance. |
There was a problem hiding this comment.
Could you explain this? So far when we run the reference benchmark we don't run HiPO, we run a static binary of HiGHS with default options.
There was a problem hiding this comment.
Hi Sid. I thought the static binary was the one for HiPO, given the fact that HiPO was not available in highspy. Should we keep using the static binary?
| reported_runtime = runtime if isinstance(runtime, (int, float)) else None | ||
| return { | ||
| "status": status, | ||
| "condition": condition, | ||
| "objective": None, | ||
| "runtime": runtime, | ||
| "reported_runtime": reported_runtime, | ||
| "duality_gap": None, | ||
| "max_integrality_violation": None, | ||
| } |
There was a problem hiding this comment.
I love well documented code as much as anyone, and this PR hugely improves the documentation of this script, but is it always worth breaking things into functions? In this case, I'm not sure it makes things more readable, and we've increased the size of this code by 2-3x with the new function and docstring. What do you think?
|
|
||
|
|
||
| def parse_memory(output): | ||
| line = output.splitlines()[-1] |
There was a problem hiding this comment.
This line got removed -- are we sure the new code has the same behavior?
| if not log_file.exists(): | ||
| print(f"Creating missing log file {log_file}") | ||
| log_file.touch() | ||
| with open(log_file, "a") as f: |
There was a problem hiding this comment.
The log file is created by run_solver btw. This change is fine too.
| Run a reference benchmark using the pre-installed HiGHS binary | ||
| """ | ||
| reference_model = "/benchmark-test-model.lp" | ||
| highs_binary = "/opt/highs/bin/highs" |
There was a problem hiding this comment.
This function shouldn't be removed. We use this to estimate the runtime variance of each VM.
|
|
||
| solver_class = getattr(solvers, solver_enum.name) | ||
|
|
||
| def set_seed_options(solver_name: str) -> dict[str, int | float]: |
| return None | ||
|
|
||
|
|
||
| def run_highs_hipo_solver(input_file, solver_version, highs_variant: HighsVariant): |
There was a problem hiding this comment.
We need this function for estimating runtime variation using the reference binary solver and benchmark
|
Ok it seems I misunderstood what we were supposed to do. I think it is worth re-starting from scratch. Closing this PR. |
Issue solved
The pull request solves issue #435 (which tackles parts of issue #386).
Changes
The changes that are performed in this pull request are (on a high level).
Changes to
run_solver.pyThe changes are:
Enumclass calledHighsSolverVariantwhich contains the possible solver choices thathighspysupportsdef parse_args()that implements a command-line argument parser.hipocan be called by passing the necessary option to argumentsolver. In particular, the HiPO variants previously contained in theEnumclass calledHighsVarianthave been removed. This is because it is now possible to pass theHiPO block sizeas the CLI argument--hipo_block_sizedef parse_args()def get_solver. The function sets the solver options and seed options respectively withdef set_solver_optionsanddef set_seed_optionsFurthermore I have updated the
README.mdwith the new proposed changes torun_solver.py.Changes to
run_benchmarks.pyThe changes are:
def download_benchmark_fileand placed the logic for downloading a file over HTTP(S) requests to the functiondef _download_via_requests, the logic for downloading a file from google cloud storage (viagsutil) todef _download_via_gsutiland the logic to decompress agzipfile and remove the original compressed file todef _unzip_gz. The three functions are invoked indownload_benchmark_file.highspy==1.13.2.dev1, I have removed the functionbenchmark_highs_binary. The reference benchmark is now run by means ofdef benchmark_solverusinghighs-hipoHighsVariantEnum has been removed and its content parsed differently inrun_solver.py(please see the rs_change 3), I have removedget_highs_binary_versiondef benchmark_solverto run also the reference benchmark withhighs-hipo. Moreover, I have placed the logic to interpret a subprocessCompletedProcessfrom a solver run and produce a metrics dictionary to the functionparse_solver_result, the logic to build ametricsdictionary for solver failure cases to the functiondef return_failure_metricsand the logic to build the shell command to run a solver with resource limits to the functiondef build_solver_command. The three functions are invoked inbenchmark_solver. In particular,def build_solver_commandaccounts for the changes in the wayrun_solver.pyis invoked.Addition of
benchmark-2026.yamlI have created a new environment file
runner/envs/benchmark-2026.yamlwhich includes the channelconda-forge/label/devwherehighspy==1.13.2.dev1is available. The environment file will need to be modified once a new version ofhighspycontainingHiPOis published toconda-forge.Tests
Unit tests
When studying and understanding a new code-base, I find it helpful to write unit tests as I go. Tests serve as a kind of executable documentation. I followed this approach while working on
run_benchmarks.py. I placed the unit tests undertests/test_run_benchmarks.py. Rather than removing them afterwards, I have decided to keep them in the repository for everyone's benefit. If not needed, they can be of course removed 😉Tests for
run_solver.pyI perfomed the tests with
knitroandxpresson a "quick" benchmark, because I just wanted to verify that the new version ofrun_solver.pywas still working with these two commercial solvers.As for
highsI instead performed the tests on the benchmark that was proposed in issue #386 .Test with Knitro
Benchmark instance: pypsa-eur-elec-trex-3-12h.lp
Machine for the test: local test on a laptop
Results: the objective function is the same one found in pull request #410 (run time is different as the tests performed in #410 were done on the
benchmark_genGCP virtual machine)Test with XPRESS
Benchmark instance: pypsa-eur-elec-trex-3-12h.lp
Machine for the test: local test on a laptop
Results: the objective function is the same one found in pull request #362 (run time is different as the tests performed in #362 were done on the
benchmark_genGCP virtual machine)Test with Highs IPX
Benchmark instance: pypsa-de-elec_6_1h.mps.gz
Machine for the test: test on
benchmark_genGCP VMResults: the command line output is available at ipx_run.txt
Test with Highs HiPO
Benchmark instance: pypsa-de-elec_6_1h.mps.gz
Machine for the test: test on
benchmark_genGCP VMResults: the command line output is available at hipo_run.txt
Test with CBC
Benchmark instance:
tests/sample_benchmarks/sample_lp.lpandtests/sample_benchmarks/sample_mip.lpMachine for the test: local test on a laptop
Results:
Tests for
run_benchmarks.pyI cannot test
run_benchmarks.pyon my laptop because the script relies uponsystemd-runwhich is not available onosx-arm64. I therefore tested it through the CI.The runs on
tests/sample_benchmarks/sample_lp.lpandtests/sample_benchmarks/sample_mip.lpfailed for thecbcsolver, even though I can successfully run these instances locally withcbc.For the moment I have therefore removed
cbcfrom the default solvers that are invoked bybenchmark_all.sh.