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

Remove unneeded __init__ function #222

Merged
merged 3 commits into from
Aug 21, 2024
Merged

Remove unneeded __init__ function #222

merged 3 commits into from
Aug 21, 2024

Conversation

visr
Copy link
Contributor

@visr visr commented Aug 17, 2024

I'm playing around with static compilation. This seemed easy enough to fix, to avoid:

Dynamic call to Base.convert(Type, String)
In C:\Users\visser_mn\.julia\packages\HiGHS\MQAyL\src\HiGHS.jl:13

@odow
Copy link
Member

odow commented Aug 17, 2024

Last time I checked, this was needed for people to be able to provide custom binaries: https://jump.dev/JuMP.jl/stable/developers/custom_solver_binaries/

Otherwise, the path is hard-coded into the package during precompilation.

I should take another look.

@odow
Copy link
Member

odow commented Aug 17, 2024

Could we fix this by something like global libhighs::String = HiGHS_jll.libhighs?

@odow
Copy link
Member

odow commented Aug 17, 2024

See the discussion in jump-dev/ECOS.jl#134

@odow
Copy link
Member

odow commented Aug 18, 2024

I'll also add that you're unlikely to be able to statically compile any of the MOI solvers because they almost all have type instabilities in various places.

What are you trying to do (and how)?

@visr
Copy link
Contributor Author

visr commented Aug 18, 2024

Oh I see, I assumed it was just an outdated pattern, I hadn't seen it before. I'd be surprised if it was still needed though, using PackageCompiler with JLLs works well, and I think it's mainly JLLWrappers that takes care of these cases working. The BinaryBuilder docs also don't mention it. Though I haven't verified.

global libhighs::String = HiGHS_jll.libhighs in __init__ gives syntax: type declaration for global "(thismodule).libhighs" not at top level.

I'm kicking the tires on JuliaLang/julia#55047. I went from the trivial hello.jl directly to the water resources model I'm working on: https://github.com/Deltares/Ribasim, to get a sense of what is needed. Perhaps this step is a bit too large though, good to have a heads up about the type instabilities.

@odow
Copy link
Member

odow commented Aug 18, 2024

I'd be surprised if it was still needed though, using PackageCompiler with JLLs works well

It's for the following cases:

PackageCompiler

  • I install HiGHS.jl on computer A, using the default HiGHS_jll
  • I compile a sysimage using PackageCompiler.jl
  • The string libhighs is baked into the sys image, and points to something like ~/.julia/artifacts/abcdef13/lib/libhighs.dll
  • I send that sysimage to computer B (but which is the same architecture)
  • Computer B uses a custom version of HiGHS_jll
  • The constant path libhighs does not exist, and so things break.

The solution is to have __init__ look up the current version of HiGHS_jll.libhighs during initialization, so that things work even if HiGHS_jll is updated.

Custom binaries

  • I install HiGHS.jl on computer A, using the default HiGHS_jll
  • Precompilation runs, and the string libhighs is baked into the precompiled .ji file
  • I use LocalPreferences to over-ride the location of libhighs
  • using HiGHS does not pick up the change, because precompilation has already happened.
  • To fix, I need to explicitly call ] precompile HiGHS again to update the constant.

The solution again is to have __init__ look up the current version of HiGHS_jll.libhighs during initialization, so that things work even if HiGHS_jll is updated.

Perhaps this step is a bit too large though, good to have a heads up about the type instabilities.

I would be very surprised if you could statically compile your water model. JuMP is not type stable.

Copy link

codecov bot commented Aug 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (1e8a36c) to head (3f2bf04).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #222   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines         1317      1313    -4     
=========================================
- Hits          1317      1313    -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@odow odow changed the title Remove unneeded __init__ function DNM: Remove unneeded __init__ function Aug 18, 2024
@topolarity
Copy link

topolarity commented Aug 19, 2024

The string libhighs is baked into the sys image, and points to something like ~/.julia/artifacts/abcdef13/lib/libhighs.dll

I don't think that's quite right - using HiGHS_jll: libhighs does not copy the value from the JLL. Instead it means that libhighs refers to HiGHS_jll.libhighs in your code.

That's different than const libhighs = HiGHS_jll.libhighs, which takes a copy of the value in HiGHS_jll.libhighs at pre-compilation time. That does have relocatability problems, which is what jump-dev/ECOS.jl#134 was trying to solve

libhighs shouldn't end up baked into the sysimage, since it's not const and it's not a copied value, so I think this should behave correctly.

@odow
Copy link
Member

odow commented Aug 20, 2024

All I know is that this was needed for jump-dev/ECOS.jl#134.

I'm quite reticent to make any changes that are needed for an as-yet unreleased feature of Julia. Especially when there are many other parts of JuMP, MOI, and the solvers that are not type stable.

@topolarity
Copy link

Right, this was needed for jump-dev/ECOS.jl#134 because that was doing const ecos = ECOS_jll.libecos which is a copy of the value at precompile time.

I'm quite reticent to make any changes that are needed for an as-yet unreleased feature of Julia. Especially when there are many other parts of JuMP, MOI, and the solvers that are not type stable.

Totally fair - no comments from me there.

Only wanted to clarify what's going on with the relocatability weirdness here.

@odow
Copy link
Member

odow commented Aug 20, 2024

Ahhhhhh. I missed that we were originally doing const. Now I understand what you mean.

I guess we can try this and see if anyone complains.

@odow odow changed the title DNM: Remove unneeded __init__ function Remove unneeded __init__ function Aug 20, 2024
@visr
Copy link
Contributor Author

visr commented Aug 20, 2024

I should add that I'm totally fine if you prefer not to merge this. At this point I'm mainly interested since I also maintain several JLL wrapper packages and care about relocatability.

@odow
Copy link
Member

odow commented Aug 21, 2024

I think I'm okay merging this, now that @topolarity has helpfully pointed out the const bit I was overlooking!

@odow odow merged commit 537add9 into jump-dev:master Aug 21, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants