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

Assert and validate Dash component functionality #3

Closed
6 of 7 tasks
rpkyle opened this issue Apr 3, 2020 · 11 comments · Fixed by #39
Closed
6 of 7 tasks

Assert and validate Dash component functionality #3

rpkyle opened this issue Apr 3, 2020 · 11 comments · Fixed by #39
Assignees

Comments

@rpkyle
Copy link
Contributor

rpkyle commented Apr 3, 2020

Both the R and Python implementations of the Dash backend perform various tests for component functionality, in R, these occur within assert_valid_callbacks. We should implement the following tests within Dash.jl in similar fashion:

  • Verify that no output arguments are duplicated
  • Verify that parameters contain only input or state arguments
  • Verify that all input arguments precede state arguments, when latter are present
  • Verify that input arguments are a nested list of dictionaries (or whatever data structure makes the most sense in Julia)
  • Verify that state arguments are a nested list of dictionaries, if they exist
  • Verify that at least one valid input has been provided
  • Verify that no callback input arguments are identical to output arguments (in Dash, we call these "circular callbacks" and they are not allowed)
@waralex
Copy link
Collaborator

waralex commented Apr 4, 2020

I don't fully understand what that means. Most of this seems to be due to the fact that both Python and R are untyped languages.

On the items:

Verify that no output arguments are duplicated

Dash produce error exception "output "$(out)" already registered" on callback! call if output already used

Verify that parameters contain only input or state arguments
Verify that all input arguments precede state arguments, when latter are present
Assert that the component ID as passed is a character string
Verify that input arguments are a nested list of dictionaries (or whatever data structure makes the most sense in Julia)
Verify that state arguments are a nested list of dictionaries, if they exist

In Dash.jl signature of callback! is (func::Function, app::DashApp, id::CallbackId) where CallbackId is:

struct CallbackId
    state ::Vector{IdProp}
    input ::Vector{IdProp}
    output ::Vector{IdProp}    
end

IdProp is Tuple{Symbol, Symbol} - i.e. pair of Symbols, the first symbol is the ID of component, the second if the property name.
So it is physically impossible to violate these points.
In additional in callback! all IDs and props are checked for presence in layout of app and raise one of the following exceptions if the checks fails: error("The layout havn't component with id $(id[1])]") or error("The component with id $(id[1]) havn't property $(id[2])")` (This may not be correct, because layout can now be set after callbacks thanks to this issue)

Verify that at least one valid input has been provided
Verify that no callback input arguments are identical to output arguments (in Dash, we call these "circular callbacks" and they are not allowed

I will add these checks to callback! call

@waralex
Copy link
Collaborator

waralex commented Apr 4, 2020

Verify that at least one valid input has been provided
Verify that no callback input arguments are identical to output arguments (in Dash, we call these "circular callbacks" and they are not allowed

At master checks for this added

@rpkyle
Copy link
Contributor Author

rpkyle commented Apr 4, 2020

I don't fully understand what that means. Most of this seems to be due to the fact that both Python and R are untyped languages.

I actually think Python and R are dynamically typed, rather than untyped, but I can see what you mean about the character string check above. I've removed it, thanks for the reminder!

As long as the user is presented with a helpful message explaining the error when the inputs don't match what Dash expects, I think this is fine.

If there are situations where a user cannot pass arguments of a given type or structure to the constructor because the language disallows it, then it definitely makes sense to avoid a pointless check.

If the Julia exception serves this purpose, I imagine this is OK. Some situations are a little more nuanced, though. What happens when a nested structure is not used when passing multiple output or state arguments?

Verify that no callback input arguments are identical to output arguments (in Dash, we call these "circular callbacks" and they are not allowed

I will add these checks to callback! call

Perfect!

@waralex
Copy link
Collaborator

waralex commented Apr 5, 2020

Yes, I chose the wrong term. I meant that Julia is also dynamically typed, but in Python and R there is no way to use strict typing.

What happens when a nested structure is not used when passing multiple output or state arguments?

struct CallbackId
    state ::Vector{IdProp}
    input ::Vector{IdProp}
    output ::Vector{IdProp}    
end

It's always struct of arrays. For example state cannot be a non array. Is that what you mean by "nested list of dictionaries"?
There is a syntactic sugar around this structure:

callback!(app, callid"""{state1.value, state2.value}
                                   submit-button.n_clicks
                                   => output.children""" ) do state1, state2, n_clicks
.....
end

callid is the macro, that is processed at compilation time, parses the string and make the corresponding CallbackId structure. This is for convenience and compactness of the code only. From the point of view of the language after compilation, callback! only possible with a typed CallbackId structure

@rpkyle
Copy link
Contributor Author

rpkyle commented May 24, 2020

@waralex In order to close this issue, there's just one item left to resolve, I believe:

  • Verify that all input arguments precede state arguments, when latter are present

Have we already made this change? I see

CallbackId(;input,
output,
state = Vector{IdProp}()
) = CallbackId(state, input, output)

... but on the other hand, I also see:

callback!(app, CallbackId(
state = [(:graphTitle, :type)],
input = [(:graphTitle, :value)],
output = [(:outputID, :children), (:outputID2, :children)]
)
) do stateType, inputValue
return (stateType * "..." * inputValue, inputValue)
end

@waralex
Copy link
Collaborator

waralex commented May 24, 2020

@rpkyle The order of passing keywords arguments is not fixed. If this is so important and the user can never specify state after input, then the only way to do this is to reject keywords argruments and use positional. Are you sure we need to do this?

According to this issue this python code should produce a validation error:

app.layout = html.Div([
    dcc.Input(id='my-id', value='initial value', type='text'),
    dcc.Input(id='my-state', value='initial state', type='text'),
    html.Div(id='my-div')
])


@app.callback(
    state = [State("my-state", "value")],
    inputs = [Input(component_id='my-id', component_property='value')],
    output = Output(component_id='my-div', component_property='children')
)
def update_output_div(input_value, state):
    return 'You\'ve entered "{}, {}"'.format(input_value, state)

However it works perfectly

@rpkyle
Copy link
Contributor Author

rpkyle commented May 24, 2020

@rpkyle The order of passing keywords arguments is not fixed. If this is so important and the user can never specify state after input, then the only way to do this is to reject keywords argruments and use positional. Are you sure we need to do this?

According to this issue this python code should produce a validation error:

app.layout = html.Div([
    dcc.Input(id='my-id', value='initial value', type='text'),
    dcc.Input(id='my-state', value='initial state', type='text'),
    html.Div(id='my-div')
])


@app.callback(
    state = [State("my-state", "value")],
    inputs = [Input(component_id='my-id', component_property='value')],
    output = Output(component_id='my-div', component_property='children')
)
def update_output_div(input_value, state):
    return 'You\'ve entered "{}, {}"'.format(input_value, state)

However it works perfectly

I asked the same question during initial development of the R version of the backend: plotly/dashR#51 (comment)

While it's not explicitly required that Dash handles the callback parameters in this manner, ordering of inputs and state is the same in R and Python by convention. The examples in our current documentation reflect this. The parity between backends makes it easier for individuals switching between implementations, and easier for those writing documentation and preparing instructional materials since there is a continuity here.

If Dash.jl is able to "reshuffle" the elements as @alexcjohnson mentions in his comment above, it's fine to avoid imposing a requirement that inputs precede state, but I would revise the examples so that they declare inputs first anyway -- there's no reason not to do this, and we might as well be consistent across all flavours of Dash.

@waralex
Copy link
Collaborator

waralex commented May 24, 2020

While it's not explicitly required that Dash handles the callback parameters in this manner, ordering of inputs and state is the same in R and Python by convention.

Well. Then the question remains what to do with the callid Now it looks like callid"{state1, state2,...} input 1, input 2,... = > output 1, output 2,...." which obviously violates the convention. The decision is yours, the options are as follows:

  • Use a different syntax (suggest one that is clear to the user and meets the agreement)
  • Remove it completely.

@alexcjohnson
Copy link
Contributor

Let me back up for a moment to the motivation for this restriction on the Python side: the callback function receives arguments (input1, input2, ..., state1, state2, ...) and for clarity it's important that the order in which arguments are specified in the callback definition matches the order they arrive in the callback function. There has been some discussion about this constraint just recently and in particular I said:

I think it's still important to have the items in order: outputs, inputs, state, rather than mixing them up. It would be super confusing to have outputs mixed in with the others, so definitely we need those to be first. Less confusing to have inputs and state mixed up, and I can see the rationale of grouping related items, but the distinction between inputs and state is important enough to the logic of the callback that I think it's worth keeping them separate.

That's a little more strict than the comment I made when this came up in dashR - we could revisit if someone feels strongly but lately I've been leaning more in the direction of extra uniformity and "one right way to use the package." That discussion was happening in the context of a PR to remove the requirement (in Python) to wrap inputs in a list and states in another list. Of course if you use named args output, inputs, and state you can put those in whatever order you want but normally folks don't bother naming those because (again, specifically in our Python implementation) each type of param has its own class Output, Input, or State.

Additionally, unless there's a strong reason to prefer a different order in Julia, I'd like to keep this consistent between languages: in the callback definition, outputs first, then inputs, then state; in the function arguments, inputs first and then state. That will help users of Dash in one language to read Dash apps written in another language, whether to adapt an example back to their primary language or for some other purpose.

The callid macro is nice for simple cases, but I'm worried about how that would work for more complex cases, and in particular I'm worried about pattern-matching callbacks. There we have component IDs as dicts (key/value objects), which Python (and JSON) serialize with {} and have no restrictions on the characters included in the keys or values. Even regular string IDs, the only characters they cannot contain are { and ., but in principle users should be able to include in or start an ID with => or whitespace or emojis... Properties are generally more constrained, I'm not sure it's a hard constraint but with the exception of the- in data-* and aria-* for html elements they're normally all valid javascript identifiers.

So all that said I might prefer to remove callid and focus on seeing what we can do to simplify the interface to CallbackId (or whatever we call that 😅)

@waralex
Copy link
Collaborator

waralex commented May 26, 2020

I understand the motivation and I agree with it. I suggest that I do the following as part of this task:

  1. remove the callid.
  2. Think about simplifying the interface with CallbackId (the reason why callid appeared is that it is very cumbersome and difficult to read to call with CallbackId in callbacks with a large number of parameters)
  3. Change the order of arguments in the callback function itself (now it is the same as in callid - in Dashboards, I assumed callid was the main way to set parameters)
  4. Change the documentation accordingly

Regarding point 2, I see such requirements for determining the parameters of the callback

  • it should be as easy and unambiguous to read as possible .
  • as long as it is possible without reducing readability, it should be compact.

Correct me if I'm wrong. I think that in any case, we will need to discuss several options when I come up with them, because my understanding of readability and convenience may not coincide with yours

@alexcjohnson
Copy link
Contributor

That all sounds right, feel free to either propose a specific API here, or to open a PR and we can discuss there, depending on your confidence in what you're suggesting.

@waralex waralex linked a pull request May 29, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants