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

Deprecated default solvers #249

Merged
merged 2 commits into from
Dec 11, 2018

Conversation

rofinn
Copy link
Contributor

@rofinn rofinn commented Dec 6, 2018

This seems like the first step to dropping the DEFAULT_SOLVER behaviour and getting rid of the following warning:

┌ Warning: Package Convex does not have ECOS in its dependencies:
│ - If you have Convex checked out for development and have
│   added ECOS as a dependency but haven't updated your primary
│   environment's manifest file, try `Pkg.resolve()`.
│ - Otherwise you may need to report an issue with Convex

which causes precompilation to fail on 1.0. Related to #244

@ararslan
Copy link
Contributor

ararslan commented Dec 6, 2018

There seems to be an issue with how often this gets called. See for example the number of times the warning is emitted in https://travis-ci.org/JuliaOpt/Convex.jl/jobs/464710210.

@rofinn
Copy link
Contributor Author

rofinn commented Dec 6, 2018

Yeah, I just figured it was an issue with depwarn. I know we've had issues with that in the past.

@ararslan
Copy link
Contributor

ararslan commented Dec 6, 2018

Ah, it seems to be because it's used in the default constructor for Problem. This might help:

--- a/src/problems.jl
+++ b/src/problems.jl
@@ -40,7 +40,7 @@ end
 
 # constructor if model is not specified
 function Problem(head::Symbol, objective::AbstractExpr, constraints::Array=Constraint[],
-                 solver::MathProgBase.AbstractMathProgSolver=get_default_solver())
+                 solver::MathProgBase.AbstractMathProgSolver=DEFAULT_SOLVER)
     Problem(head, objective, MathProgBase.ConicModel(solver), constraints)
 end

@rofinn
Copy link
Contributor Author

rofinn commented Dec 6, 2018

I guess if we deprecate the set_default_solver function too then that should be fine.

@rofinn rofinn force-pushed the rf/deprecate-default-solvers branch from 2441651 to 04afdf5 Compare December 6, 2018 23:43
@ararslan
Copy link
Contributor

ararslan commented Dec 6, 2018

The warning still pops up a lot but it's at least less spammy than before it seems.

@rofinn
Copy link
Contributor Author

rofinn commented Dec 6, 2018

Yeah, looks like the test code is calling get_default_solver() pretty often.

@ararslan
Copy link
Contributor

ararslan commented Dec 6, 2018

That's probably not representative of external user/package code, so maybe fine? 😅

Copy link
Contributor

@iamed2 iamed2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did the depwarn cause a problem when that function was used as a default argument? Is it anything special to do with that, or is depwarn just generally not good at warning only once?

@rofinn
Copy link
Contributor Author

rofinn commented Dec 7, 2018

I'm not sure why the depwarn was causing problems, but I have noticed that it isn't very good at only warning once. Maybe it's related to where/how the deprecated function is called? :/

@oxinabox
Copy link

oxinabox commented Dec 7, 2018

It is because maxlog is broken, no?
JuliaLang/julia#28786

It is now fixed, but that fix is only in master, I believe.

@iamed2
Copy link
Contributor

iamed2 commented Dec 7, 2018

Looks like that made it in to 1.0.2

@oxinabox
Copy link

oxinabox commented Dec 7, 2018

Ah, so it is.

@rofinn rofinn mentioned this pull request Dec 7, 2018
@ararslan ararslan merged commit c992f1b into jump-dev:master Dec 11, 2018
@ararslan ararslan deleted the rf/deprecate-default-solvers branch December 11, 2018 17:32
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.

None yet

5 participants