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

Rewrite the C wrapper #268

Merged
merged 15 commits into from
Nov 24, 2023
Merged

Rewrite the C wrapper #268

merged 15 commits into from
Nov 24, 2023

Conversation

odow
Copy link
Member

@odow odow commented Nov 16, 2023

Part of #267 Currently failing because things are still only half-fixed, but it's enough to get a sense for discussion.

I've learnt a lot about the code. The main issues:

  • No C return codes are checked or thrown
  • Multiple ways to do the same thing by (ab)using multiple dispatch
  • Lots of macros to "simplify" the code

It all looks very familiar, because it is exactly how a lot of the early Cbc/Clp/GLPK/Gurobi/CPLEX wrappers were written. In the long run though, I think it was a bad choice. The MILP wrappers are a lot more maintainable now that we switched to libxxx.jl + MOI. And you can still call any C function, it just takes a little more overhead, but it's also much clearer exactly what you are doing and why.

Copy link

codecov bot commented Nov 19, 2023

Codecov Report

Attention: 22 lines in your changes are missing coverage. Please review.

Comparison is base (4db649c) 74.57% compared to head (1be5603) 88.61%.

Files Patch % Lines
src/MOI_wrapper.jl 87.59% 17 Missing ⚠️
src/C_wrapper.jl 97.90% 5 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #268       +/-   ##
===========================================
+ Coverage   74.57%   88.61%   +14.04%     
===========================================
  Files           5        3        -2     
  Lines        1805     1019      -786     
===========================================
- Hits         1346      903      -443     
+ Misses        459      116      -343     

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

@odow odow mentioned this pull request Nov 21, 2023
@odow odow changed the title WIP: remove unecessary parts of C wrapper Rewrite the C wrapper Nov 21, 2023
src/C_wrapper.jl Outdated Show resolved Hide resolved
@odow odow requested a review from frapac November 21, 2023 22:37
@odow
Copy link
Member Author

odow commented Nov 21, 2023

I've asked the Artelys folks if they have people using the current low-level API. That would be one blocker moving forward with this. (We could keep the old functions if needed. But only advertise the "proper" way of using the API going forward.)

@mgabay
Copy link

mgabay commented Nov 24, 2023

Hi Oscar,

We can't think of any client who could be using it currently.

Anyway, I've been looking into the discussion in #267 and #268 and the proposal makes a lot of sense! I think it is a very good idea to go on with these changes, especially as 1.0 has not been released yet.

Also, I expect it would make it easier to reflect possible additions to Knitro API in the future. Don't worry, no breaking changes coming in foreseeable future on our side ;-)

Thanks a lot for your work on this!

@odow
Copy link
Member Author

odow commented Nov 24, 2023

Also, I expect it would make it easier to reflect possible additions to Knitro API in the future

Yes, precisely. This change would make it so there is no friction between changes/updates/new features in the C API and Julia. They would be exactly equivalent.

This is the approach we have taken with Gurobi.jl etc, and it works very nicely.

@odow
Copy link
Member Author

odow commented Nov 24, 2023

Since this is getting rather big, I'm going to merge this for now. I'll make a separate PR that fixes all of the return code checking.

@odow odow merged commit 14b5497 into master Nov 24, 2023
5 checks passed
@odow odow deleted the od/rm-c-wrapper branch November 24, 2023 21:17
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.

2 participants