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

Single input #1180

Merged
merged 32 commits into from
Aug 6, 2020
Merged

Single input #1180

merged 32 commits into from
Aug 6, 2020

Conversation

mbegel
Copy link

@mbegel mbegel commented Apr 5, 2020

Single Input in callbacks don't need to be in a list.

  • I have broken down my PR scope into the following TODO tasks
    • modify dash.clientside_callback to support single input not in a list
    • modify dash.callback to support single input not in a list
  • I have run the tests locally and they passed.
  • I have extended existing tests, to cover the new feature in this PR

@alexcjohnson
Copy link
Collaborator

This is an interesting idea. A single State doesn't need to be in a list either, you could in fact argue that neither would multiple Input or State items if we dealt with them all as *args and simply required all the Input items to come before all the State items - there's precedent for that part in Dash for R, where Input and State go together in a single list when defining the callback.

Note that in the case of Output there is a difference between an un-nested and a single-item list, as it impacts the form of the callback return value. So even if we went all the way to complete un-nesting we can't give the same flexibility to the outputs.

@chriddyp thoughts about this? On the one hand I like removing the extra brackets, and I've made the mistake myself many times of forgetting them. On the other hand it gets away from the one-way-to-do-everything philosophy a bit.

@chriddyp
Copy link
Member

Yeah, I like getting away from square brackets too. I've forgotten them before too. It also makes getting the indentation right difficult:

@app.callback(
    Output(...),
    [
        Input(...),
        Input(...)
    ],
    [
        State(...),
        State(...)
    ]
)

vs

@app.callback(
    Output(...),
    Input(...),
    Input(...),
    State(...),
    State(...)
)

Re output symmetry - The [Output(...)] vs Output(...) has tripped up a few people in the forum. I see that some people end up wanting to standardize with square brackets around everything and so they use [Output()] even for a single list. Then, they don't really understand why they have to wrap their output into a list of length one. It's subtle.

So, I'd vote for moving away to no lists for Output, Input, or State. If there is a single Output, then return a single item. If there are multiple Output items, then return a tuple or a list.

I think we can do this in a backwards compatible way, right? If we did this, we'd move all documentation over to this new syntax.


As an aside...

In general when we consider explicit list vs *args syntax, we should think about whether a user would want to assign a variable to that list. If so, then they might have a hard time understanding how *args unpacking works (I found *args pretty confusing in my first year of using Python). This is one of the main arguments for keeping children as a list (see plotly/Dash.jl#7 (comment) for a discussion).

With Input & State, I don't think that users are assigning their lists of Input & State as variables, so I don't see any issues with that.


One of the original motivations with [Input(...)] was pedagogical: Users would see that Input was in a list and then intuit that for "multiple inputs, you could put multiple items into the list". I see students of the workshop come to this conclusion when we teach workshops.

I don't think that this is a very strong argument though, as our pedagogy will immediately follow up with an example with two inputs. And this still looks pretty intuitive I think:

@app.callback(
    Output('my-output', 'children'),
    Input('my-dropdown-1', 'value'),
    Input('my-dropdown-2', 'value'))
def update_output(value1, value2):
    return 'You have selected {} and {}'.format(value1, value2)

So, I think I'd be in favor of moving away from square brackets all together in a backwards compatible way.

@alexcjohnson
Copy link
Collaborator

I suppose as long as we still allow the brackets, and when you DO supply a single output in brackets we still require the return value to be a length-1 list, I think you're right and we can do this in a backward-compatible way.

With Input & State, I don't think that users are assigning their lists of Input & State as variables, so I don't see any issues with that.

There is still a use case for this in programmatically-generated callbacks where you don't know how many outputs you'll have, whether it'll be one or more. In that case you'd want your return to always be wrapped in a length-1 list. Though even that use case becomes much less important with pattern-matching callbacks 🎉

@wilhelmhb
Copy link
Contributor

The latest commits implement the behavior presented by @chriddyp in this comment.

This allows to include Inputand State objects without any specific order in the callback decorator. It is backward compatible: lists of inputs and states are still handled. Also, I don't require the output of the function to be a list for callbacks having a length-one list of Output, but in this case, if the return value is a list, then the value associated to the output is considered to be the content of the list.

Side note: I don't know what is going on with Github, as this pull request shows around 90 commits whereas the comparison of the branches shows only 12, which is the real number of changes.

@alexcjohnson
Copy link
Collaborator

@mbegel @wilhelmhb apologies for the delay getting back to this PR. I've attempted to rebase off dev - seems to have fixed whatever was going on with GitHub though I've apparently broken something else... to be determined, and I'm happy to sort it out and continue working on this PR, though it may not be right away.

@alexcjohnson
Copy link
Collaborator

@wilhelmhb thanks for generalizing this. One thing I'd like to modify:

This allows to include Input and State objects without any specific order in the callback decorator.

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.

FYI the piece that made rebasing tricky (and that I appear to have messed up somewhere) is prevent_initial_call #1228, which due to this PR has become a keyword-only arg. I suppose we could also support the last positional arg being a boolean. We will likely want to also support output, inputs, and state as kwargs for full backward compatibility in case anyone has been using their names in callback definitions.

Allow to use named parameters in callback
Raise exceptions if outputs, inputs or states are not ordered
@wilhelmhb
Copy link
Contributor

Thanks for rebasing, it's much cleaner this way =)

My latest commit addresses these two concerns:

I think it's still important to have the items in order: outputs, inputs, state, rather than mixing them up.

We will likely want to also support output, inputs, and state as kwargs for full backward compatibility in case anyone has been using their names in callback definitions.

Unfortunately, I'm not able to run the lint test on my machine, and I can't access the detailed CI results anymore.

@alexcjohnson
Copy link
Collaborator

@Marc-Andre-Rivet I think this is ready for a final review.

"then all Inputs, then all States. Found this out of order:
{}
""".format(
repr(extra_args)
Copy link
Contributor

Choose a reason for hiding this comment

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

When running against

@app.callback(
    Output('table', 'data'),
    State('table', 'data_previous'),
    Input('btn', 'n_clicks'),
    State('table', 'data')
)

Got the error message:

dash.exceptions.IncorrectTypeException: In a callback definition, you must provide all Outputs first,
"then all Inputs, then all States. Found this out of order:
[<Input `btn.n_clicks`>, <State `table.data`>]

As the error causing props are from the previous State('table', 'data_previous') and Input('btn', 'n_clicks'), the message could be tweaked to be more helpful / point to the problematic parameters / relationship more precisely.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good call. In d158ef8 I changed it so this particular situation would say:

dash.exceptions.IncorrectTypeException: In a callback definition, you must provide all Outputs first,
then all Inputs, then all States. After this item:
<State `table.data_previous`>
we found this item next:
<Input `btn.n_clicks`>

@alexcjohnson alexcjohnson mentioned this pull request Jul 31, 2020
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

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

💃

@alexcjohnson
Copy link
Collaborator

Merging - to be included in Dash 1.15, should come out in the next couple of weeks. Thanks for your contributions @mbegel and @wilhelmhb !

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 this pull request may close these issues.

5 participants