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

[Dev] Extensions #12

Closed
ocots opened this issue Sep 3, 2024 · 13 comments · Fixed by #21
Closed

[Dev] Extensions #12

ocots opened this issue Sep 3, 2024 · 13 comments · Fixed by #21
Assignees
Labels
enhancement New feature or request internal dev Modification to the code is requested and should not affect user experiment

Comments

@ocots
Copy link
Member

ocots commented Sep 3, 2024

We could make two extensions containing respectively JuMPModels and OptimalControlModels.

The module JuMPModels would be triggered by using JuMP and OptimalControlModels by OptimalControl.

This would reduce what is loaded for instance if a user only wants to load OptimalControl models.

using OptimalControl
using OptimalControlProblems

nlp = OptimalControlModels.cart_pendulum()

Note

CTBasePlots.jl is an extension of CTBase.jl. To trigger it, see the Project.toml.

@ocots ocots added enhancement New feature or request internal dev Modification to the code is requested and should not affect user experiment labels Sep 3, 2024
@ocots ocots self-assigned this Sep 3, 2024
@ocots
Copy link
Member Author

ocots commented Sep 3, 2024

Another possibility is to write:

using OptimalControl
using OptimalControlProblems

nlp = cart_pendulum(; model=:optimalcontrol)

or

using JuMP
using OptimalControlProblems

nlp = cart_pendulum(; model=:jump)

with an error of the form:

julia> using JuMP
julia> using OptimalControlProblems

julia> nlp = cart_pendulum(; model=:optimalcontrol)
ERROR: ExtensionError. Please make: julia> using OptimalControl

Note

In CTDirect.jl you have an example of such a case. See here to see how is defined the solve_docp function which returns the error and here the extension which makes the job.

@frapac
Copy link
Collaborator

frapac commented Sep 3, 2024

I would be in favor of not complexifying too much OptimalControlProblems.jl, and I don't think we should use package extensions at the moment.

I think it's reasonable to assume both JuMP and OptimalControl as direct dependencies there, as they are both complementary to formulate OCP.

Package extensions would make sense if we want to add more backends to the benchmark library.

@frapac
Copy link
Collaborator

frapac commented Sep 3, 2024

Regarding your second point, I think a more Julian way would be to use multiple dispatch. Something like:

# template 
function cart_pendulum end 

abstract type AbstractOCPBackend end 

struct JuMPBackend <: AbstractOCPBackend end

function cart_pendulum(::JuMPBackend) 
    # some code
end

struct OptimalControlBackend <: AbstractOCPBackend end

function cart_pendulum(::OptimalControlBackend) 
    # some code
end

The problem can be instantiated e.g. as

nlp = cart_pendulum(JuMPBackend())

That way an error is automatically thrown if the backend is not defined.

@ocots
Copy link
Member Author

ocots commented Sep 3, 2024

I would be in favor of not complexifying too much OptimalControlProblems.jl, and I don't think we should use package extensions at the moment.

I think it's reasonable to assume both JuMP and OptimalControl as direct dependencies there, as they are both complementary to formulate OCP.

Package extensions would make sense if we want to add more backends to the benchmark library.

I agree with the fact that we should not complexify too much for the user and for someone who want to add models. I do not think that this is so complex to make an extension and for the user, it changes just the fact that he has to make an additional using and if you want to add problems, it does not change anything.

I would be in favor to make extensions because it is very nice to reduce as much as possible time for installation, precompilation, etc.

One use case is the following: we will make some tests inside CTDirect.jl using some problems from OptimalControlProblems.jl. In this use case there is no need of Jump models. Besides, to define an OptimalControlModel, we do not need OptimalControl.jl but simply CTBase.jl. So to trigger OptimalControlModels, we should add in the weak dependencies not OptimalControl but CTBase. Like that both CTBase.jl and OptimalControl.jl will trigger it since OptimalControl load CTBase.

Besides, handling extensions errors permit to give a message more detailed. In Julia, the error messages are not easy to understand.

@PierreMartinon Any thoughts?

@ocots
Copy link
Member Author

ocots commented Sep 3, 2024

For the second point, I agree with you but I have hidden multiple dispatch. I like the idea that the user should not care about types. The function with the model keyword would call a function with multiple dispatch.

@frapac
Copy link
Collaborator

frapac commented Sep 3, 2024

One use case is the following: we will make some tests inside CTDirect.jl using some problems from OptimalControlProblems.jl.

I see your point. In that case let's follow your idea.

I like the idea that the user should not care about types. The function with the model keyword would call a function with multiple dispatch.

I see. But is that compatible with package extensions? As far as I understand, you will have to define the structs JuMPBackend and OptimalControlBackend in the main repo to define your switch statement as

function cart_pendulum(; model=:optimalcontrol)
     if model == :optimalcontrol
         cart_pendulum(OptimalControlBackend())
     elseif model == :jump
         cart_pendulum(JuMPBackend())
     else
          error("...")
     end
end

Is that the idea? If so I think this will lead to a lot of boilerplate code, and hinder the package's extensibility if we want to plug additional backends.

@ocots
Copy link
Member Author

ocots commented Sep 3, 2024

For the second point you will have the following.

In the main module:

abstract type AbstractModelBackend end
struct OptimalControlBackend <: AbstractModelBackend end
struct JuMPBackend <: AbstractModelBackend end

function cart_pendulum(; model=:optimalcontrol)
     if model == :optimalcontrol
         cart_pendulum(OptimalControlBackend())
     elseif model == :jump
         cart_pendulum(JuMPBackend())
     else
          error("unknown model: ", model)
     end
end

# function to be extended
function cart_pendulum(model_backend::AbstractModelBackend, args...; kwargs...)
    if typeof(model_backend) == OptimalControlBackend
        throw(ExtensionError(:OptimalControl)) # this provides an error which tells to make julia> using OptimalControl
    elseif typeof(model_backend) == JuMPBackend
        throw(ExtensionError(:JuMP))
    end
end

In the extension for OptimalControl models we would have:

function cart_pendulum(model_backend::OptimalControlBackend; nh::Int=100)
...
end

@ocots
Copy link
Member Author

ocots commented Sep 3, 2024

To make things shorter we can define in the main module:

abstract type AbstractModelBackend end
struct OptimalControlBackend <: AbstractModelBackend end
struct JuMPBackend <: AbstractModelBackend end

backends = Dict(:optimalcontrol => OptimalControlBackend, :jump => JuMPBackend)
weakdeps = Dict(OptimalControlBackend => :OptimalControl, JuMPBackend => :JuMP)

function cart_pendulum(; model=:optimalcontrol)
    model  keys(backends) ? cart_pendulum(backends[model]()) : error("Unknown model: ", model)
end

function cart_pendulum(model_backend::T, args...; kwargs...) where {T <: AbstractModelBackend}
    throw(ExtensionError(weakdeps[T])) # equivalent to error("Please make: julia> using ", weakdeps[T])
end

and in the extension, it is as usual:

function cart_pendulum(model_backend::OptimalControlBackend; nh::Int=100)
...
end

Note

The part in the main module may be automated from the files I guess. We only need the name of the problems.

Note

I am tired but we can also mutualize code for backends and weakdeps. However, the real weak dependency for OptimalControlBackend is not OptimalControl but CTBase. It would be better to trigger with CTBase. In that case it would work also with a using OptimalControl since CTBase is in the dependencies of OptimalControl.

@ocots
Copy link
Member Author

ocots commented Sep 4, 2024

A version more generic :-)

# ------- Backend Definitions -------
abstract type AbstractModelBackend end
struct OptimalControlBackend <: AbstractModelBackend end
struct JuMPBackend <: AbstractModelBackend end

# backend dictionary
backends = Dict(:optimalcontrol => OptimalControlBackend, :jump => JuMPBackend)

# weak dependencies
weakdeps = Dict(OptimalControlBackend => :CTBase, JuMPBackend => :JuMP)

# default backend
__model_backend_key() = :optimalcontrol

# path to problems
path = joinpath(dirname(@__FILE__), "OptimalControlModels")

# ------- Problem Definitions -------
files = filter(x -> x[(end - 2):end] == ".jl", readdir(path))
for file in files
    if file  "OptimalControlModels.jl"
        problem = Symbol(file[1:end-3])
        code = quote
            function $problem(; model=__model_backend_key())
                model_ = Symbol(lowercase(string(model)))
                model_  keys(backends) ? $problem(backends[model_]()) : error("Unknown model: ", model)
            end

            function $problem(model_backend::T, args...; kwargs...) where {T <: AbstractModelBackend}
                throw(ExtensionError(weakdeps[T]))
            end
        end
        eval(code)
    end
end

@frapac
Copy link
Collaborator

frapac commented Sep 4, 2024

Well, to be honest, I think this new version is way too complicated :)
I think it's a bit overkill for a package that just aims at implementing a collections of optimal control instances.

I am still convinced that we should stick to multiple dispatch. JuMPBackend and OptimalControlBackend should be defined in the their respective extensions. That way, the users can add new backends if they want too.

@ocots
Copy link
Member Author

ocots commented Sep 5, 2024

It was just for fun 😆

Extensions is the most important for me.

We can do multiple dispatch or let like it is, both are fine for me. Between multiple dispatch or qualify the functions I have no preference.

@0Yassine0
Copy link
Collaborator

Honestly, I'm having a hard time understanding how does the extension work. For this example :

In CTDirect.jl you have an example of such a case. See here to see how is defined the solve_docp function which returns the error and here the extension which makes the job.

I tried to do the same with the cart_pendulum problem:

  • Project.toml
[weakdeps]
JuMP = "4076af6c-e467-56ae-b986-b466b2749572"
OptimalControl = "5f98b655-cc9a-415a-b60e-744165666948"

[extensions]
JuMPModels = ["JuMP"]
OptimalControlModels = ["OptimalControl"]
  • OptimalControlProblems
module OptimalControlProblems

abstract type AbstractModelBackend end
struct OptimalControlBackend <: AbstractModelBackend end
struct JuMPBackend <: AbstractModelBackend end

backends = Dict(:optimalcontrol => OptimalControlBackend, :jump => JuMPBackend)
weakdeps = Dict(OptimalControlBackend => :OptimalControl, JuMPBackend => :JuMP)

function cart_pendulum(; model=:optimalcontrol)
    if model == :optimalcontrol
        cart_pendulum(OptimalControlBackend())
    elseif model == :jump
        cart_pendulum(JuMPBackend())
    else
         error("unknown model: ", model)
    end
end

function cart_pendulum(model_backend::T, args...; kwargs...) where {T <: AbstractModelBackend}
end

end
  • OptimalControlModels
module OptimalControlModels

using OptimalControlProblems

function OptimalControlProblems.cart_pendulum(model_backend::OptimalControlBackend;nh::Int=100)
    println("OK")
end

end

and to test it I run :

import Pkg
Pkg.activate(".")
using OptimalControlProblems
nlp = OptimalControlProblems.cart_pendulum(; model=:optimalcontrol)

And what I didn't understand is how the module of OptimalControlProblems calls cart_pendulum function of OptimalControlModels without any include or call between them ?

@ocots
Copy link
Member Author

ocots commented Sep 5, 2024

@0Yassine0 Point demain 9h.

@0Yassine0 0Yassine0 linked a pull request Sep 6, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request internal dev Modification to the code is requested and should not affect user experiment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants