-
Notifications
You must be signed in to change notification settings - Fork 267
Callbacks as CSharp Events #2433
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
base: latest
Are you sure you want to change the base?
Callbacks as CSharp Events #2433
Conversation
Several HiGHS callbacks are each exposed as its own event in the C# HighsLpSolver
class. The event names are as follows:
kCallbackLogging => LogMessageReceived
kCallbackMipLogging => MipStatusReported
kCallbackMipImprovingSolution => MipImprovingSolutionFound
kCallbackMipInterrupt => MipInterruptCheck
kCallbackSimplexInterrupt => SimplexInterruptCheck
kCallbackIpmInterrupt => IpmInterruptCheck
Clients subscribe to and unsubscribe from these events as they would for any other event,
using syntax such as `lp.LogMessageReceived += MyLoggingFunc`. Subscribers don't need
to call Highs_startCallback or Highs_stopCallback. The events are written so that they
automatically call startCallback when the first client subscribes to the event, and
automatically call stopCallback when the last client unsubscribes from the event.
The interrupt check events include a `user_interrupt` flag in their event args. If an event
handler sets this flag, it is passed back to HiGHS via the cbDataIn callback function argument,
which causes the event's algorithm to stop.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## latest #2433 +/- ##
==========================================
+ Coverage 79.30% 79.44% +0.14%
==========================================
Files 346 346
Lines 84987 85060 +73
==========================================
+ Hits 67400 67579 +179
+ Misses 17587 17481 -106 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'm submitting this as a draft PR to get feedback from the HiGHS team, and to give me more time to run it through its paces. In particular I'd like to discuss testing of this particular feature. I'd love for it to be included in CI so we can detect if future callback changes break the integration. Locally, I tested by adding a function to the C interface that lets you trigger callbacks with supplied data to a specified callback function. This function is only useful for testing purposes and wouldn't belong in the released C interface. I also added a C# test library that calls the callback trigger function. Without testing for specific callbacks with specific data, we can't be confident data is getting across the C/C# boundary correctly. My testing approach is a lot more intrusive than just adding some code to existing test files, so I'd be interested in hearing how the team would like to approach regression testing for this feature. |
|
Thank you, @darrylmelander! Yes, we would like to add such tests to our CI too. We would possibly try to follow or extend what we already have in terms of testing.
So far, we have a csharp executable, which calls the csharp API: https://github.com/ERGO-Code/HiGHS/blob/master/examples/call_highs_from_csharp.cs Perhaps we could have another example, which uses some callbacks from C#, to illustrate how to do it. This is built by cmake.
Additionally, we also test the nuget package: https://github.com/ERGO-Code/HiGHS/blob/master/.github/workflows/test-nuget-win.yml It is worth adding callback examples tests to the nuget as well.
We are developing some code in external repositories with additional tests to be run before we tag a new release. I could add more C# tests there too, containing specific callbacks with specific data. Perhaps it is worth having a short call about this. We are busy this week organising our workshop, but we could arrange a call next week. Please send me an email at [email protected] with your availability if you have the time and would like to discuss some of the details. |
|
@darrylmelander Are you still working on this? Otherwise I would be interested in expanding further and testing to get it merged |
I think so. @darrylmelander emailed us yesterday saying "I will send you my code as soon as I get a chance, hopefully soon." |
I have tested it locally, but to do so I had to modify the core HiGHS library to expose some additional functions. Otherwise I didn't have a way to trigger specific events with specific data my tests could check against. Those changes, plus the tests that use those changes, is what I will be sending to jajhall when I get some time in the next few days. I would welcome additional testing and follow-on development. |
|
Any updates on this? If not, I could write some tests. I had a look already, I haven't seen any existing tests for C#, so I'd appreciate a pointer where to get started (any preferences on libraries, should this be hooked into existing tests [though py is not, so I assume no], ...) |
|
Unfortunately no-one in the HiGHS team is a C# developer, so we've relied on external contributors to do the groundwork, with us just adding easy things by pattern-matching. For libraries in general, please ask @galabovaa |
1 similar comment
|
Unfortunately no-one in the HiGHS team is a C# developer, so we've relied on external contributors to do the groundwork, with us just adding easy things by pattern-matching. For libraries in general, please ask @galabovaa |
| { | ||
| this.highs = HighsLpSolver.Highs_create(); | ||
| _cbDelegate = this.callbackFunction; | ||
| Highs_setCallback(this.highs, Marshal.GetFunctionPointerForDelegate(_cbDelegate), IntPtr.Zero); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You must manually keep the delegate from being collected by the garbage collector from managed code. The garbage collector does not track references to unmanaged code.
You should store the delegate pointer, not _cbDelegate.
(Removed reference to UnmanagedCallersOnlyAttribute since it's not available in the target version)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The _cbDelegate member variable stores a reference to the delegate, which prevents the delegate from being garbage collected for the lifetime of the HighsModel. The whole reason _cbDelegate exists is to satisfy the guidance you quoted. By my understanding, whether you store the return value of Marshal.GetFunctionPointerForDelegate has zero impact on garbage collection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, I think you may be right, thanks
Give access to some callbacks in C#, as events on the C# HighsLpSolver class. Only a handful of callbacks are made available by this commit. The callbacks, and the new events they map to, are as follows:
Clients subscribe to and unsubscribe from these events as they would for any other event, using syntax such as
solver.LogMessageReceived += MyLoggingFunc. Event data only includes what's relevant to the type of event. The*InterruptCheckevents allow you to set the interrupt flag, the others don't.The events in this PR address #2042.