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

Partial updates #680

Merged
merged 9 commits into from
Apr 8, 2019
Merged

Partial updates #680

merged 9 commits into from
Apr 8, 2019

Conversation

alexcjohnson
Copy link
Collaborator

Fixes #668

Provides a new dash.no_update - a singleton that you can use in a callback return to prevent a single output from updating. Two reasons you might want to do this:

  1. Most importantly, for updating only some of a multi-output callback, leaving the rest unchanged
  2. Even for a single-output callback, it might not be ideal to raise an exception. Maybe there's cleanup you want to do after determining the return value, or further logic that might reverse the decision to not update.

I'm not super excited about the name, any suggestions? PreventUpdate makes sense for an exception as it's based on a verb, this seems like it should be a noun but related to PreventUpdate, so I went with no_update.

Also its location: it's not an exception so doesn't seem to belong as dash.exceptions.no_update. For now I just put it at the top level as dash.no_update. OK?

Copy link
Contributor

@valentijnnieman valentijnnieman left a comment

Choose a reason for hiding this comment

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

💃 Looks like a good solution to me. I'm good with the name – another suggestion could be something like disabled_update, or prevented_update, but I'm not crazy about those either! Don't let the naming block you too much I guess. :-)

component_ids[o_id][o_prop] = val

if not has_update:
raise exceptions.PreventUpdate
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use flask.abort instead of going the slow exception way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know what flask.abort does, but I wanted to trigger the PreventDefault handler, in case the renderer does any optimizations with that specific response to maximize performance.

dash/dash/dash.py

Lines 180 to 183 in bb5ed70

@self.server.errorhandler(exceptions.PreventUpdate)
def _handle_error(_):
"""Handle a halted callback and return an empty 204 response"""
return '', 204

Copy link
Contributor

Choose a reason for hiding this comment

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

It's an empty 204 which would be the same as flask.abort(204). Raising the exception takes longer because it has to be caught and then the handler is called whereas abort is immediate.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually seems like flask.abort is doing more than that... if I try to do flask.abort(204) it fails with LookupError: no exception for 204. The docs say:

If a status code is given it’s looked up in the list of exceptions and will raise that exception

Which anyway sounds at least as complicated as what we're doing with raise PreventUpdate.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, 204 is not an error. But then you are at the dispatch level, think you can just do return '', 204 and avoid any error handling.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're in the weeds now - the return from this function feeds into response.set_data() in the actual dispatch, which I don't believe accepts a status code.

dash/dash/dash.py

Line 1094 in b2a97b4

response.set_data(self.callback_map[output]['callback'](*args))

There may be a way to improve on this but I'd like to defer it for this PR, if that's alright.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, that's new. Anyway, this works well for partial updates.

@notatallshaw
Copy link

With this partial update logic if a callback outputs to 2 props "foo" and "bar" where "foo" receives an update but "bar" doesn't then I assume if "bar" is the input to another callback then that callback doesn't get triggered?

@alexcjohnson
Copy link
Collaborator Author

@notatallshaw I believe that's how it works - I should probably add another test to 🔒 this down though. (we do have tests confirming that this is what happens with PreventUpdate)

if val is not no_update:
has_update = True
o_id, o_prop = o.component_id, o.component_property
component_ids[o_id][o_prop] = val
Copy link
Member

Choose a reason for hiding this comment

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

Glad that we're returning a dict instead of a list here 😸 I was sure that we would've had to change the renderer to do this.

@chriddyp
Copy link
Member

chriddyp commented Apr 8, 2019

  • return [3, dash.no_update, 10]
  • return [3, dash.without_update, 10]
  • return [3, dash.existing_value, 10]

At first glance, I sort of like existing_value the best.

But I also like the symmetry of PreventUpdate / no_update is great & PreventUpdate might be more common than no_update.

And now that I think about it, with existing_value, folks might try to actually access the existing value (instead of using State): "I need the current value of a property. I tried to access dash.existing_value with no avail."

So, 👍 no_update.

💃 💃 💃

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