fix: handle None return from error handler in no-output background callbacks - #4012
dajiaohuang wants to merge 7 commits into
Conversation
…llbacks When a background callback with no outputs raises an exception, the error handler is called. If the error handler returns None, the code was incorrectly setting output_value = NoUpdate(), which then triggered an InvalidCallbackReturnValue exception at the output validation step. This change adds the same output_spec check that exists in the non-background callback path (line 866) to the background callback path (line 635), ensuring that NoUpdate() is only set when there are actual outputs to update. Fixes plotly#3628
|
Thanks for the PR! Could you please look into the test failures and see what's going on? |
|
T4rk1n
left a comment
T4rk1n
left a comment
There was a problem hiding this comment.
Works good, and the lcbc019 test covers it. One gap: the job-cancel branch a few lines above has the same NoUpdate() for no-output callbacks, so cancelling a no-output background callback still raises InvalidCallbackReturnValue. Can you fix that here too, since it's the same bug? Please also add a CHANGELOG entry under Fixed for #3628.
| error_handler, | ||
| callback_ctx, | ||
| multi, | ||
| output_spec, |
There was a problem hiding this comment.
The cancel branch just above still sets output_value = NoUpdate() whether or not there are outputs. Say a no-output background callback gets cancelled through cancel=, or its job is gone when the poll arrives. _prepare_response then goes to the else branch and raises No-output callback received return value: <NoUpdate>. Can you give that branch the same output_spec guard, and add a cancel case to the test?
| assert created_epoch.isdigit() | ||
|
|
||
|
|
||
| def test_mcpbg012_tasks_result_passes_output_spec(monkeypatch): |
There was a problem hiding this comment.
This test monkeypatches six things just to check that one kwarg gets passed through, and the fake copies the whole signature, so it breaks on any refactor. lcbc019 already covers the behavior. Can you drop this one, or change it to drive a real no-output MCP background tool?
3 participants
Summary
When a background callback with no outputs raises an exception, the error handler is called. If the error handler returns
None, the code was incorrectly settingoutput_value = NoUpdate(), which then triggered anInvalidCallbackReturnValueexception at the output validation step.Fix
This change adds the same
output_speccheck that exists in the non-background callback path to the background callback path at_callback.py:635, ensuring thatNoUpdate()is only set when there are actual outputs to update.Testing
The issue includes a minimal reproducible example that can be used to verify the fix.
Related Issue
Fixes #3628