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 callback interface #195

Closed
wants to merge 44 commits into from

Conversation

pedromxavier
Copy link

@pedromxavier pedromxavier commented Mar 17, 2023

This PR aims to reorganize the callback interface for Xpress in order to:

  • Make it easier to maintain
  • Better align with MOI's callback definitions

@pedromxavier
Copy link
Author

@joaquimg, could you take a look?

src/MOI/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI/MOI_wrapper.jl Show resolved Hide resolved
src/callbacks/MOI_callbacks.jl Outdated Show resolved Hide resolved
src/callbacks/generic.jl Outdated Show resolved Hide resolved
test/MathOptInterface/MOI_callbacks.jl Outdated Show resolved Hide resolved
src/Xpress.jl Outdated Show resolved Hide resolved
test/xprs_callbacks/XPRS_callbacks.jl Outdated Show resolved Hide resolved
src/MOI/callbacks/MOI_user_cut.jl Outdated Show resolved Hide resolved
src/MOI/callbacks/generic.jl Outdated Show resolved Hide resolved
src/MOI/callbacks/interface.jl Show resolved Hide resolved
test/MathOptInterface/MOI_callbacks.jl Outdated Show resolved Hide resolved
Copy link
Member

@joaquimg joaquimg left a comment

Choose a reason for hiding this comment

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

I marked a few examples of some issues, but most of them appear in many other places.

src/MOI/callbacks/MOI_heuristic.jl Outdated Show resolved Hide resolved
src/MOI/callbacks/MOI_heuristic.jl Outdated Show resolved Hide resolved
src/MOI/callbacks/MOI_lazy_constraint.jl Outdated Show resolved Hide resolved
src/MOI/callbacks/MOI_lazy_constraint.jl Outdated Show resolved Hide resolved
src/MOI/callbacks/MOI_lazy_constraint.jl Outdated Show resolved Hide resolved
src/MOI/callbacks/MOI_message.jl Outdated Show resolved Hide resolved
src/MOI/callbacks/MOI_user_cut.jl Outdated Show resolved Hide resolved
src/MOI/callbacks/MOI_user_cut.jl Outdated Show resolved Hide resolved
@joaquimg
Copy link
Member

Also, fix conflicts :)

src/MOI/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI/MOI_wrapper.jl Outdated Show resolved Hide resolved
src/MOI/callbacks/MOI_heuristic.jl Outdated Show resolved Hide resolved
src/MOI/callbacks/MOI_heuristic.jl Outdated Show resolved Hide resolved
info = model.callback_table.moi_heuristic::Union{Tuple{CallbackInfo{CD}}, Nothing}

if !isnothing(info)
push_callback_state!(model, CS_MOI_HEURISTIC)
Copy link
Member

Choose a reason for hiding this comment

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

This is not thread-safe, right?
We should probably comment it

Copy link
Author

Choose a reason for hiding this comment

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

Certainly not.

After reading the docs, I think that we shouldn't worry about that much since it only breaks if MUTEXCALLBACKS is set to 0. We could make this mechanism thread-safe when the mutex is off but it would take some time thinking to get things right.

Copy link
Author

Choose a reason for hiding this comment

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

Note
My first attempt would be to rely on the MIPTHREADID index to sort out each thread's context.

Copy link
Member

Choose a reason for hiding this comment

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

Lets add this info somewhere?
Maybe in the optimizer struct right by the associated fields

Copy link
Author

Choose a reason for hiding this comment

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

How about triggering a warning if the number of threads is greater than one and there are registered callbacks? @joaquimg

Copy link
Member

Choose a reason for hiding this comment

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

shouldn't it warn if there are registered callback and MUTEXCALLBACK = 0?

dmatval,
)

Xpress.Lib.XPRSloadcuts(
Copy link
Member

Choose a reason for hiding this comment

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

User cut should not be local only?

Copy link
Author

@pedromxavier pedromxavier Apr 14, 2023

Choose a reason for hiding this comment

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

That lies on the second checkbox of the PR description. We are still refactoring without breaking the old API in some sense.
Implementing heuristic/lazyconstraint/usercut properly requires some redesign and even other XPRS methods might be useful then.

Copy link
Member

Choose a reason for hiding this comment

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

So you agree, but you want to fix it in a second step?

Copy link
Member

Choose a reason for hiding this comment

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

if that is the case, lets add a comment about it so we do not forget.

Copy link
Author

Choose a reason for hiding this comment

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

The plan is to integrate our new callback coordination mechanism and then to make it follow MOI's semantics.
I'm adding the comments.

src/MOI/callbacks/XPRS_message.jl Outdated Show resolved Hide resolved
src/MOI/callbacks/XPRS_optnode.jl Outdated Show resolved Hide resolved
src/MOI/callbacks/XPRS_preintsol.jl Outdated Show resolved Hide resolved
src/MOI/callbacks/interface.jl Outdated Show resolved Hide resolved
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.

3 participants