jump-dev / HiGHS.jl

A Julia interface to the HiGHS solver
https://highs.dev
MIT License
108 stars 15 forks source link

Add GC-safe region around `Highs_run` #178

Closed kpamnany closed 1 year ago

kpamnany commented 1 year ago

Prevent a long-running Highs_run call from blocking GC (and freezing other Julia threads). Ref: https://github.com/JuliaLang/julia/issues/51574

Questions:

odow commented 1 year ago

Does Highs_run call back into Julia?

Not currently, but it may do in future.

Is this the only call that can run for minutes?

There are other calls, but we don't use them at the moment.

Let me take a deeper look at this PR. I would prefer that we put the region around https://github.com/jump-dev/HiGHS.jl/blob/7316ac9140d97f3dda4bd46c48455582f20cc790/src/MOI_wrapper.jl#L1815 and left the automatically built libhighs.jl alone

kpamnany commented 1 year ago

Moved the region to MOI_wrapper.jl.

odow commented 1 year ago

Did you find this instance because of some benchmark? I guess this might actually be a problem for most of the JuMP solvers

codecov[bot] commented 1 year ago

Codecov Report

All modified lines are covered by tests :white_check_mark:

Comparison is base (7316ac9) 83.31% compared to head (3dadf96) 83.34%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #178 +/- ## ========================================== + Coverage 83.31% 83.34% +0.03% ========================================== Files 3 3 Lines 1630 1633 +3 ========================================== + Hits 1358 1361 +3 Misses 272 272 ``` | [Files](https://app.codecov.io/gh/jump-dev/HiGHS.jl/pull/178?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jump-dev) | Coverage Δ | | |---|---|---| | [src/MOI\_wrapper.jl](https://app.codecov.io/gh/jump-dev/HiGHS.jl/pull/178?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=jump-dev#diff-c3JjL01PSV93cmFwcGVyLmps) | `94.12% <100.00%> (+0.01%)` | :arrow_up: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kpamnany commented 1 year ago

I work at RelationalAI; we're using Julia to build our product. The freeze I mention happened to us. 😢

This is likely a problem with the other solvers as well, as well as any package that wraps an external library (i.e. uses ccall) that implements possibly long-running logic.

kpamnany commented 1 year ago

Can I request a new patch release with this fix?

odow commented 1 year ago

I work at RelationalAI

Ah :smile:

What other solvers do you need me to look at?

Can I request a new patch release with this fix?

Yip

kpamnany commented 1 year ago

What other solvers do you need me to look at?

Let me get back on that one.

Thanks very much!

kpamnany commented 12 months ago

Here are other jump-dev solvers that are relevant to us, but don't need an immediate fix:

Cc: @bachdavi

odow commented 12 months ago

I think these are all safe: