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

Support loading extension in precompiled package #206

Closed
wants to merge 1 commit into from

Conversation

yuyichao
Copy link

The global needs to exist in the base package to avoid the error

Creating a new global in closed module `NLopt` (`Optimizer`) breaks incremental compilation because the side effects will not be permanent.

This only happens if NLopt and MathOptInterface are both loaded during precompilation. It's still unfortunate that the global is not const anymore but this would at least fix the error.

The global needs to exist in the base package to avoid the error

    Creating a new global in closed module `NLopt` (`Optimizer`) breaks incremental compilation because the side effects will not be permanent.
@blegat blegat mentioned this pull request Oct 20, 2023
11 tasks
@codecov-commenter
Copy link

codecov-commenter commented Oct 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Files Coverage Δ
src/NLopt.jl 52.43% <ø> (ø)

📢 Thoughts on this report? Let us know!.

@stevengj
Copy link
Collaborator

It's still unfortunate that the global is not const anymore but this would at least fix the error.

Is it const if MathOptInterface is loaded? The code still does const Optimizer = NLoptMathOptInterfaceExt.Optimizer in that case, no?

@yuyichao
Copy link
Author

No, which branch got executed only depends on Julia version and not on what’s being loaded. At precompile time on a new enough julia version, MOI is never loaded and the constant branch never runs.

@yuyichao
Copy link
Author

Alternatively, we could make Optimizer an abstract or parametrized type so that the binding can be constant. I believe the standard use of this is actually only using this as a factory type and there isn't even any requirement that the returned value is an instance of the type passed in (and the input doesn't even need to be a type).

This would only make a difference if NLopt.Optimizer is used outside of JuMP in a way that relies on this being a concrete type and I did find one of such use in the wield with a simple github search https://github.com/OpenMendel/OrdinalMultinomialModels.jl/blob/60bbc53665879b9fb8f2950c602f8444b10e9ad8/src/ordmntest.jl#L138 . It should be trivially fixable but the change could still technically count as breaking.

@odow
Copy link
Member

odow commented Jan 15, 2024

Closing as duplicate of #211

@odow odow closed this Jan 15, 2024
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.

4 participants