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

The @provide/provided mechanism for optional interface functions. #1

Closed
zsunberg opened this issue Jun 15, 2020 · 19 comments · Fixed by #23
Closed

The @provide/provided mechanism for optional interface functions. #1

zsunberg opened this issue Jun 15, 2020 · 19 comments · Fixed by #23
Milestone

Comments

@zsunberg
Copy link
Member

What do we think of the @provide mechanism? That is the experimental/risky part of this package. Does anyone foresee big problems. Feel free to play around with it and see if it is intuitive.

@rejuvyesh
Copy link

Would this be enough? I think ChainRules.jl also does something like that.

@zsunberg
Copy link
Member Author

@zsunberg
Copy link
Member Author

zsunberg commented Jun 15, 2020

Hmm... yeah @rejuvyesh , we could just tell people to use static_hasmethod. It makes me a bit nervous to rely on that since they didn't put it in julia even though it apparently works. I couldn't find what you were talking about in ChainRules. If you remember where it is, let me know.

Is there value in having @provide as a more explicit signal that the environment is opting in to that optional part of the interface?


Edit: I don't think static_hasmethod will be adequate because it will make generic wrapping very hard: #1 (comment)

@rejuvyesh
Copy link

I can't find it either. I think I saw some discussions in Julia slack's #autodiff channel, but can't find it anymore.

@rejuvyesh
Copy link

I would let others pitch in. Depends on how the errors look like.

@zsunberg
Copy link
Member Author

One difference is that someone could make provided return differently based on the values of the arguments instead of just the types (though I'm not sure whether we want that).

@darsnack
Copy link

According to this blog post by the author of Tricks.jl, throwing that method error might be an anti-pattern?

When an algorithm or user tries to call a method, if the environment doesn't provide it, then a method error will be thrown by Julia. So, I see the advantages of having some kind of @provide logic as:

  • pretty printing the error message
  • allowing someone to check easily all the methods that are provided (as opposed to hitting the default error message, fixing, then iterating over and over again)

The second bullet is the more compelling reason to me to include all the logic for @provide. Though I wonder if making provided as something to override in the interface would achieve the same functionality?

@zsunberg
Copy link
Member Author

Very good thoughts. @provide/provided avoids the antipattern mentioned in that blog because it does not override the MethodError that gets thrown if you try to call a missing method.

allowing someone to check easily all the methods that are provided (as opposed to hitting the default error message, fixing, then iterating over and over again)

We have this in POMDPs: https://juliapomdp.github.io/POMDPs.jl/stable/requirements/. The main difficulty is that an algorithm-writer has to manually declare all of the requirements up front, and this may be somewhat error-prone, especially if they have to declare the types of all the arguments. (The other option is to write a very heavy-duty analyzer that looks for the interface throughout the code, and this seems difficult)

Though I wonder if making provided as something to override in the interface would achieve the same functionality?

Yes, the user can absolutely do that. @provide just syntactic sugar to do it easily in one line with the function definition.

all the logic for @provide

There is actually not too much logic (and it should be straightforward to support kwargs and where as well)

IMO the most important reason for this is actually to enable efficient behavior for different ways that problems might be implemented (IIUC this is the reason for Base.IteratorSize).

For example, in the future, we might have both

CommonRLInterface.setstate!(env, s)

and

CommonRLInterface.clone(env)

in the interface. For some envs, it might be very difficult to clone, and for others, it might be very difficult to set the state, so they might only be able to implement one.

If someone implementing MCTS wants to handle both cases elegantly and with high performance, you need either provided or static_hasmethod.

@zsunberg
Copy link
Member Author

zsunberg commented Jun 15, 2020

Actually, it seems very plausible that someone might want to make provided depend on the values of the arguments, e.g.

provided(CommonRLInterface.actionmask, env::PythonGymWrapper) = hasattr(env.pyenv, "actionmask")

I think that is a good enough argument to prefer provided to static_hasmethod.

@zsunberg
Copy link
Member Author

Regarding the antipattern from the blog, though, it will be very tempting for someone to write:

if !provided(clone, env)
    error("You need to provide clone!")
end

and this will be terrible because it will silence the method error. 😿 😿 . The same person might write something worse without provided though. One possible mitigation would be to add something like require(clone, env) that shows a message and throws the MethodError if the method is not provided.

@jbrea
Copy link

jbrea commented Jun 16, 2020

That's a neat idea with @require. I like that it's really up to the author of a solver to define what should be provided by an environment and that one can print informative error messages to the authors of environments.

An alternative strategy could be to provide testing utilities that can be used by authors of solvers and environments, but are not in the way when actually running a solver on an environment. I am thinking of something similar to an annotated methodswith function that throws warnings and errors (or prints a table), e.g.

function check_interface_methods(env_type, action_type = Int)
    # mandatory interface
    hasmethod(step!, (env_type, action_type)) || @error "step! error message"
    hasmethod(reset!, (env_type,)) || @error "reset! error message"
    ....
    # optional interface
    hasmethod(clone, (env_type,)) || @warn "clone warning message"
    ....
end

Solver authors could also implement a method like

required_interface(::typeof(solver)) = (clone,)

and we change check_interface_methods such that one can call it with an optional solver argument that changes the type of messages of the optional interface functions from warning to error for the required methods of that solver.

The solver itself would never do any checks and simply throw MethodErrors, if it encounters an environment that doesn't implement the required optional interface methods.

What I like about this idea is that the check_interface_method would make clear which optional interface functions we consider, there is nothing that would hinder performance at runtime, but the developers can use such utilities for testing their implementations and debugging.

What do you think about this?

@darsnack
Copy link

darsnack commented Jun 16, 2020

Yeah something closer to what @jbrea described is what I was thinking. Except instead of required_interface(::typeof(solver)) = (clone,), the environment write would do provided_interface(::typeof(MyEnv)) = (clone,). But based on @zsunberg 's comments, I see that someone could write

if myfunc  provided_interface(::typeof(env))
    error("Cannot find $myfunc!")
end

I thinking shifting the dynamic from what the environment provides to what the solver requires would circumvent the temptation for anyone to use these functions for any other purpose than to check if a solver and environment are compatible.


Though I would also add that if clone is the only optional interface we can think of, then it is easy enough to provide clone(env::CommonEnv) = deepcopy(env), and allow authors to override this method with more efficient versions. Do we have a compelling list of functions that would make up the optional interface?

@zsunberg zsunberg added the decision A decision that needs to be made label Jun 16, 2020
@zsunberg zsunberg added this to the v0.1 milestone Jun 16, 2020
@zsunberg
Copy link
Member Author

zsunberg commented Jun 16, 2020

Ok, seems like there are two side questions being raised:

  1. requirements specification - breaking out into Requirements specification #7
  2. default implementations - breaking out into Candidate Principle: CommonRLInterface does NOT provide any default implementations! #6

The question we need to answer in this thread could be stated as "Do we want a traits-like mechanism for telling what parts of the interface have been implemented"? The main purpose of this would be for dealing with multiple options for similar behavior (e.g. the setstate!and/or clone example above. My vote is: yes we do want this trait-like mechanism, and provided is the best way to implement it.

@zsunberg zsunberg changed the title The @provide mechanism for optional interface functions. The @provide/provided mechanism for optional interface functions. Jun 16, 2020
@zsunberg zsunberg modified the milestones: v0.1, v0.2 Jun 16, 2020
@zsunberg
Copy link
Member Author

zsunberg commented Jun 16, 2020

Do we have a compelling list of functions that would make up the optional interface?

I added a few more with this tag I think there could be a ton.

@jbrea
Copy link

jbrea commented Jun 17, 2020

"Do we want a traits-like mechanism for telling what parts of the interface have been implemented"?

Thanks for the clarification. If we want it, I agree that provided is probably the best way to implement this.

Now, specifically for the example of MCTS and clone vs. setstate!: as soon as an environment implements setstate! one could have clone with a wrapper

struct CloneEnv{S,E}
    init::S
    state::S
    env::E
end
CloneEnv(state, env) = CloneEnv(copy(state), state, e)
reset!(e::CloneEnv) = e.state .= e.init # to be discussed, if we want this
function step!(e::CloneEnv, a)
    setstate!(e.env, e.state)
    result = step!(e.env, a)
    e.state = getstate(e.env)
    result
end

# to be implemented by the author of Env
clone(e::Env) = CloneEnv(getstate(e), e)

such that MCTS can rely on clone for all environments and does not need to rely on a trait-like mechanims to distinguish between environments that have either clone or setstate! implemented.
Wouldn't this make it easier for an implementation of MCTS?

Do you envision other examples where a trait-like mechanism may be desirable?

@darsnack
Copy link

If someone implementing MCTS wants to handle both cases elegantly and with high performance, you need either provided or static_hasmethod.

I totally missed this comment. I'm also satisfied that we need something like provided and @provide is the right way to do it.

@zsunberg
Copy link
Member Author

zsunberg commented Jun 18, 2020

Wouldn't this make it easier for an implementation of MCTS?

Possibly. I think this is another option that has tradeoffs. Can you clarify this: who implements CloneEnv? Also, I'm not sure I understood # to be implemented by the author of Env; does this mean the Env author needs to know about CloneEnv?

Do you envision other examples where a trait-like mechanism may be desirable?

  1. In general, any time someone wants to programmatically explore or react to which optional parts of the interface have been implemented.

  2. A common task is to create a wrapper that inherits most functionality (as above with CloneEnv). For example, it's often useful to take a continuous environment and discretize the action space. If we have provided, we can provide a very handy tool for this:

    abstract type AbstractEnvWrapper{E<:AbstractEnv} <: AbstractEnv end
    
    actions(w::AbstractEnvWrapper) = actions(env(w))
    step!(w::AbstractEnvWrapper, a) = step!(env(w), a)
    reset!(w::AbstractEnvWrapper) = reset!(env(w))
    
    # for each optional function, use valid_actions as an example (note: there are a few methods, e.g. clone, where this will not work)
    provided(::typeof(valid_actions), Tuple{AbstractEnvWrapper{E}}) where E <: AbstractEnv = provided(valid_actions, Tuple{E})
    provided(::typeof(valid_actions), w::AbstractEnvWrapper) = provided(valid_actions, env(w))
    valid_actions(w::AbstractEnvWrapper) = valid_actions(env(w))

    Then, someone can easily implement a wrapper that forwards all of the functions by simply implementing env:

    struct MyWrapper{E} <: AbstractEnvWrapper{E}
        env::E
    end
    env(w::MyWrapper) = w.env

    and the wrapper will have correct "providedness". Then they can manipulate the action space with, e.g.

    env = MyWrapper(continuous_action_env)
    actions(w::MyWrapper) = (-1.0, 0.0, 1.0)

    hasmethod or static_hasmethod won't work in this case because it will return true because the forwarding methods for AbstractEnvWrapper exist.

  3. I think there will be a lot more cases like clone/setstate! where there are multiple ways to do things. If we adopt the principle of no default implementations (Candidate Principle: CommonRLInterface does NOT provide any default implementations! #6), provided could be a way for algorithm writers to be "friendlier" and accept a wider variety of environments, e.g.

    if provided(valid_actions, env)
        acts = valid_actions(env)
    elseif provided(action_mask, env)
        acts = actions(env)[action_mask(env)]
    else
        acts = actions(env)
    end

    we could even provide some syntactic sugar for this. If we don't adopt Candidate Principle: CommonRLInterface does NOT provide any default implementations! #6, it can come in handy for defaults, e.g.

    function valid_actions(env)
        if provided(action_mask, env)
            return actions(env)[action_mask(env)]
        else
            return actions(env)
        end
    end

    (but then there is also the question of whether provided(valid_actions, env) should return true)

@jbrea
Copy link

jbrea commented Jun 18, 2020

Yep. This looks convincing. It gives a lot of fexibility. The author of MCTS could for example decide to write either CloneEnv and clone(env) = provided(setstate!, env) ? CloneEnv(state(env), env) : error() or do something completely different with provided.


who implements CloneEnv?

If we adopt #6 (which I like) we could consider having a package CommonRLDefaults.jl with useful defaults etc., where we could put CloneEnv.

does this mean the Env author needs to know about CloneEnv?

Yes.

@zsunberg
Copy link
Member Author

zsunberg commented Jun 19, 2020

Ok, I'm going to mark this as a made-decision, let's let it rest for a few days while we wait for version 0.1 to get registered, and then we'll put it in version 0.2. If anyone still has any comments or concerns though, feel free to add them!

@zsunberg zsunberg added decision-made and removed decision A decision that needs to be made labels Jun 19, 2020
@zsunberg zsunberg reopened this Jun 22, 2020
@zsunberg zsunberg mentioned this issue Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants