-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Support Arbitrary callbacks #2822
Conversation
T4rk1n
commented
Mar 29, 2024
- Allow no output callback, resolve Allow for "null" / no output callbacks #1549
- Add global set_props, resolve [Feature Request] Global set_props in backend callbacks. #2803
- Fix side update pattern ids, fix [Long Callback] Support pattern matching for long callbacks side updates (progress/cancel/running) #2111
- Allow no output callback - Add global set_props - Fix side update pattern ids
Going to add a few more tests, then we should be almost there! |
@@ -331,9 +337,12 @@ def wrap_func(func): | |||
def add_context(*args, **kwargs): | |||
output_spec = kwargs.pop("outputs_list") | |||
app_callback_manager = kwargs.pop("long_callback_manager", None) | |||
callback_ctx = kwargs.pop("callback_context", {}) | |||
callback_ctx = kwargs.pop( | |||
"callback_context", AttributeDict({"updated_props": {}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does callback_ctx
always need to contain an updated_props
key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember there is an error with some mocked tests.
|
||
callback_context = CallbackContext() | ||
|
||
|
||
def set_props(component_id: typing.Union[str, dict], props: dict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why the function signature for set_props
isn't structured as 3 inputs: component_id
, prop_name
, value
?
That to me seems more consistent with the rest of the Dash API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the same signature as the frontend set_props
, we considered set_props("id", prop=value)
but decided to stay with same API for frontend and backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true, this pattern is a little more javascripty, but consistency is good and it could be convenient to be able to set multiple props of the same component in one call.
@@ -0,0 +1,7 @@ | |||
class ProxySetProps(dict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of ProxySetProps
? Maybe add a comment at the top of the file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a trick I found to make it work with background callbacks, when the value is set the handler is called and save the set_props data to be retrieved in the callback loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that mean background callbacks can update arbitrary props mid-execution and have that reflected in the app? If so that's super slick! Does it work to update the same prop(s) as the regular callback output(s), like progressive loading of the result?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the app is updated async like with the set_progress, it should be possible to update the same prop as the output.
Aside from my comments, LGTM. ✨ In general there are a number of places where you've added separate code paths for |
if cb.get("no_output"): | ||
outputs_list = [] | ||
elif not outputs_list: | ||
# FIXME Old renderer support? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to support old renderer? Embedded? We should rebuild & test embedded with this update anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is what the comment on split_callback_id says for old embedded and new backend, it's pretty old now, maybe we can remove entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into it when I'll update embedded next.
component_ids[id_str][prop] = vali | ||
else: | ||
if output_value is not None: | ||
raise InvalidCallbackReturnValue( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the hash part is gonna be for all no-output callback error, let's try to remove it.
dash/_callback.py
Outdated
else: | ||
if output_value is not None: | ||
raise InvalidCallbackReturnValue( | ||
f"No output callback received return value: {output_value}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should be "No-output" rather than "No output"
# A single dot within a dict id key or value is OK | ||
# but in case of multiple dots together escape each dot | ||
# with `\` so we don't mistake it for multi-outputs | ||
hashed_inputs = None | ||
|
||
def _hash_inputs(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @T4rk1n , no more comments from me. 🎉
Could you double-check with @alexcjohnson whether his questions have been resolved before merging?