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

Improve error message #1448

Merged
merged 4 commits into from
Oct 29, 2020
Merged

Improve error message #1448

merged 4 commits into from
Oct 29, 2020

Conversation

almarklein
Copy link
Contributor

@almarklein almarklein commented Oct 29, 2020

I was struggling with an error in an app I was working on. I got an error, but it did not tell me much. This catches and re-raises that error to add a more meaningful message, which would have saved me some time :)

Contributor Checklist

  • Update code
  • I'm assuming this error is most likely to occur in the situation that I was in. If this is not the case, the message might only add to the confusion.
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added entry in the CHANGELOG.md

@alexcjohnson
Copy link
Collaborator

Ah, very interesting - thanks for bringing this up @almarklein !

It's a little confusing to understand how forgetting the @ gets us into this state, so let me detail it a little. The callback map entry is created when app.callback is called - and this happens whether you mark it as a decorator or not:

callback_id = self._insert_callback(output, inputs, state, prevent_initial_call)

But the callback function only gets attached to the callback map when the result of app.callback is evaluated with its function, and THIS part only happens when app.callback is marked as a decorator (or equivalently if you call it as a higher-order function app.callback(...)(func)):

dash/dash/dash.py

Line 1046 in e2f7884

self.callback_map[callback_id]["callback"] = add_context

Which explains how we got to a situation where a callback is dispatched without a function to dispatch to!


OK, with that out of the way, I think you're right that omitting the @ is about the only way to encounter this particular error. My first reaction was to see if we could catch this error earlier, on startup rather than when we finally try to dispatch this callback. It's a bit tricky to figure out when that should happen though - it would need to be after all the callbacks have been defined, but also it shouldn't depend how you run the server (run_server vs gunicorn etc)... So yeah, let's keep the check where you have it.

The only thing I'd add to this is to include output - the "callback ID" that describes all the component IDs and props returned by this callback - in the message, so that if you have a big app with many callbacks it'll be easy to find the offending one.

@almarklein
Copy link
Contributor Author

The only thing I'd add to this is to include output [...] in the message

Done!

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 thanks!

@almarklein
Copy link
Contributor Author

Thanks for the quick and clear review :)

@alexcjohnson alexcjohnson merged commit ab212a3 into plotly:dev Oct 29, 2020
@almarklein almarklein deleted the forgot-decorator branch October 29, 2020 16:16
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.

2 participants