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

Prepare for MOI v0.9 #126

Merged
merged 16 commits into from
Aug 20, 2019
Merged

Prepare for MOI v0.9 #126

merged 16 commits into from
Aug 20, 2019

Conversation

rschwarz
Copy link
Collaborator

@rschwarz
Copy link
Collaborator Author

This should be ready (passes the tests locally).

In fact, most of the changes in this commit are about implementing features that existed in MOI prior to v0.9, but that I learned about via the newly added tests and docs 👨‍🎓

@matbesancon matbesancon reopened this Aug 13, 2019
@rschwarz
Copy link
Collaborator Author

If we want to pass the tests in CI here, we should also change the version requirements in the project.toml, I think. I did not do so before, because the MOI version was not released yet.

@matbesancon
Copy link
Member

From the failure, it looks like IpOpt needs to be upgraded first?

@rschwarz
Copy link
Collaborator Author

I don't know where you see something specific to IPOPT. The first failing log that I looked at in Travis only shows MOI unit test stuff.

In any case, SCIP.jl does not use IPOPT.jl, so it should not matter?

@matbesancon
Copy link
Member

I don't know where you see something specific to IPOPT.

Log on Travis:
https://travis-ci.org/SCIP-Interfaces/SCIP.jl/jobs/571451684#L318

Anyway the error seems linked to something else

@rschwarz
Copy link
Collaborator Author

Log on Travis:

Yeah, but message from Ipopt always appears (once), even when everything passes. So it does not mean anything.

@rschwarz
Copy link
Collaborator Author

OK, the failing tests seem to point to real problems in SCIP, not an out-of-date Project.toml. Probably related to changes (tests) that were added to MOI after this PR was created. I will need to take a closer look later.

@rschwarz
Copy link
Collaborator Author

Unfortunately, the MINLPtests are failing with a Pkg dependency problem that I can't quite figure out from the error message.

 - they are failing often (which we allow)
 - they take quite a while, especially on OSX
 - the MINLPTests will only start after these have finished, taking even longer
@matbesancon
Copy link
Member

It seems to require JuMP, which has not been upgraded yet to MOI 0.9

@matbesancon
Copy link
Member

@rschwarz there was a problem with the way the compats were specified, on #121 CEnum v0.2 is not compatible with the way we use it

@codecov
Copy link

codecov bot commented Aug 16, 2019

Codecov Report

Merging #126 into master will increase coverage by 1.69%.
The diff coverage is 91.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
+ Coverage    14.8%   16.49%   +1.69%     
==========================================
  Files         135      135              
  Lines        4262     4310      +48     
==========================================
+ Hits          631      711      +80     
+ Misses       3631     3599      -32
Impacted Files Coverage Δ
src/MOI_wrapper/results.jl 100% <100%> (ø) ⬆️
src/wrapper/scip_param.jl 27.65% <100%> (+12.76%) ⬆️
src/MOI_wrapper/variable.jl 85.84% <100%> (+9.31%) ⬆️
src/managed_scip.jl 95.65% <84.84%> (-1.55%) ⬇️
src/MOI_wrapper.jl 90.47% <92.85%> (+8.84%) ⬆️
src/wrapper/scip_sol.jl 6.94% <0%> (+1.38%) ⬆️
src/wrapper/cons_linear.jl 50% <0%> (+4.54%) ⬆️
src/wrapper/cons_quadratic.jl 26.19% <0%> (+4.76%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 121a1ea...80859f3. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 16, 2019

Codecov Report

Merging #126 into master will increase coverage by 1.69%.
The diff coverage is 91.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
+ Coverage    14.8%   16.49%   +1.69%     
==========================================
  Files         135      135              
  Lines        4262     4310      +48     
==========================================
+ Hits          631      711      +80     
+ Misses       3631     3599      -32
Impacted Files Coverage Δ
src/MOI_wrapper/results.jl 100% <100%> (ø) ⬆️
src/wrapper/scip_param.jl 27.65% <100%> (+12.76%) ⬆️
src/MOI_wrapper/variable.jl 85.84% <100%> (+9.31%) ⬆️
src/managed_scip.jl 95.65% <84.84%> (-1.55%) ⬇️
src/MOI_wrapper.jl 90.47% <92.85%> (+8.84%) ⬆️
src/wrapper/scip_sol.jl 6.94% <0%> (+1.38%) ⬆️
src/wrapper/cons_linear.jl 50% <0%> (+4.54%) ⬆️
src/wrapper/cons_quadratic.jl 26.19% <0%> (+4.76%) ⬆️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebc5285...3f8e5e1. Read the comment docs.

@rschwarz
Copy link
Collaborator Author

So, we could merge this to master despite the failing MINLPTests (waiting for JuMP release, waiting for Julia bugfix), so that we can take care of the other open PRs.

But I don't know whether we should tag a new version for SCIP.jl before a compatible JuMP exists?

@matbesancon
Copy link
Member

So, we could merge this to master despite the failing MINLPTests

Maybe we can add an allowed failure on the travis script?

But I don't know whether we should tag a new version for SCIP.jl before a compatible JuMP exists?

I think we should, officially, JuMP is just one of the interfaces on top of MOI, this should not be blocking for solvers

 - these require JuMP
 - SCIP itself should only depend on MOI
@rschwarz
Copy link
Collaborator Author

Good points. I'm not so clear about the Travis syntax on allowing that stage to fail. First attempt incoming...

@rschwarz rschwarz merged commit ed32eef into master Aug 20, 2019
@rschwarz rschwarz deleted the rs/moi0.9 branch August 20, 2019 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants