-
Notifications
You must be signed in to change notification settings - Fork 33
[WIP] Add multi output callback support. #91
Conversation
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.
Even without prettier it's still a big diff because I moved a good part of updateProps
in a for loop. I commented the part where the changes are.
.map(e => e.split('.')[0]); | ||
} else { | ||
outputIds = [outputIdAndProp.split('.')[0]]; | ||
} |
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.
Create an array of output ids instead of a single output.
A few issues I uncovered while trying to fix the tests failures:
|
I think the Hot-reload breakage is caused by the changes in #108 |
@@ -316,9 +324,12 @@ export function notifyObservers(payload) { | |||
* of a controller change. | |||
* for example, perhaps the user has hidden one of the observers | |||
*/ | |||
|
|||
const {paths} = getState(); | |||
|
|||
if ( | |||
controllersInFutureQueue.length === 0 && |
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 don't think controllersInFutureQueue
will be correct here, since the controllers
variable set above:
const controllers = InputGraph.hasNode(outputIdAndProp)
? InputGraph.dependantsOf(outputIdAndProp)
: [];
Would be []
whenever we are doing multiple outputs.
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.
The controllers variable has the required 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.
Oh I didn't know those comments were posting 😄 Makes sense after reading below
if ( | ||
controllersInFutureQueue.length === 0 && | ||
has(outputComponentId, getState().paths) && | ||
any(e => has(e, paths))(outputIds) && | ||
!controllerIsInExistingQueue |
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.
Same comment here.
src/actions/index.js
Outdated
} | ||
}; | ||
if (multi) { | ||
Object.entries(data.response).forEach(handleResponse) |
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.
could use ramda forEach
src/actions/index.js
Outdated
@@ -390,18 +397,28 @@ function updateOutput( | |||
* } | |||
* | |||
*/ | |||
|
|||
const [outputComponentId, outputProp] = outputIdAndProp.split('.'); |
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 thought this was a mistake at first, but it looks like in the multi-output case it does not matter that these variables are incorrect. Perhaps only create these locals when config.multi_output
for readability?
@@ -242,7 +242,15 @@ export function notifyObservers(payload) { | |||
); | |||
const queuedObservers = []; | |||
outputObservers.forEach(function filterObservers(outputIdAndProp) { |
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 found the variable name outputIdAndProp
confusing when reading this, since it can now refer to stuff like [output1.children:output2.n_clicks]
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.
Overall looks great, I'm surprised this could be done without a complete overhaul.
src/actions/index.js
Outdated
dependency.output.id === outputComponentId && | ||
dependency.output.property === outputProp | ||
dependency => { | ||
if (config.multi_output) { |
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 this just so we can upgrade dash_renderer
without upgrading dash
?
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 renderer can be updated without dash.
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.
Okay, then if we lock dash-renderer
version in dash
install requires then we should be good.
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
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 to keep config.multi_output
now that version locking is done?
This is really awesome stuff. I'll leave it to others to dig into the code and do a full review, but from a high level I'd really like to see an extensive suite of high-level tests for different arrangements of the dependencies (DAG), timing issues, hide/showing component behaviour, and callback dependency "chains". The set of tests in https://github.com/plotly/dash-renderer/blob/master/tests/test_render.py were the only way that I've maintained sanity while working through refactors and bug fixes. I've outlined some pseudocode of the types of tests that I would be writing to make sure that all of this tricky behaviour is locked down. There are probably many other scenarios to consider, but hopefully this gets everyone's wheels turning :)
A super simple 1-Many case. Also pushing up the number of outputs past 10 as sometimes there can be weird bugs between 9 and 10 (from one digit to two digits). N_OUTPUTS = 50
app.layout = html.Div(
[dcc.Input(id='input', value='dash')] +
[html.Div(id='output-{}'.format(i)) for i in range(N_OUTPUTS)]
])
@app.callback([Output('output-{}'.format(i), 'children') for i in range(N_OUTPUTS)], [Input('input', 'value')])
def update_output(value):
call_count.value += 1
return ['{} - {}'.format(i, value) for i in range(N_OUTPUTS)]
# pseudo-testing code here, but you get the idea
for i in range(N_OUTPUTS):
self.assertTextEqual(
'output-{}'.format(i),
'{} - dash'.format(i)
)
self.assertEqual(call_count.value, 1)
sendkeys('input', ' hello')
for i in range(N_OUTPUTS):
self.assertTextEqual(
'output-{}'.format(i),
'{} - dash hello'.format(i)
)
self.assertEqual(call_count.value, 2)
This one is a sanity test that multiple properties on the same component will get updated. app.layout = html.Div([
dcc.Input(id='input', value='dash'),
html.Div(id='output'),
])
@app.callback(
[Output('output', 'children'),
Output('output', 'style'),
Output('output', 'className')],
[Input('input', 'value')])
def update_output(value):
call_count.value += 1
return [
value,
{'fontFamily': value},
value
]
# pseudo-testing code here, but you get the idea
self.assertHTMLEqual(
'output',
'<div class="dash" style="font-family: dash">dash<div>'
)
self.assertEqual(call_count.value, 1)
sendkeys('input', ' hello')
self.assertHTMLEqual(
'output',
'<div class="dash hello" style="font-family: dash hello">dash hello<div>'
)
self.assertEqual(call_count.value, 2)
This one tests layouts that generate other components and, when generated, trigger other callbacks. This one also swaps out entire trees, to make sure that all of that state management is clean. app.layout = html.Div([
dcc.RadioItems(
id='input',
options=[{'label': i, 'value': i} for i in ['tree', 'string']]
value='tree'
),
html.Div(id='output-1'),
html.Div(id='middle', children='text in the middle'),
html.Div(id='output-2'),
])
@app.callback(
[Output('output-1', 'children'),
Output('output-2', 'children')],
[Input('input', 'value')])
def update_output(value):
call_count.value += 1
if value == 'tree':
return [
html.Div(id='output-1-child', children=[
html.Div(
id='output-1-grandchild-1',
children='output 1 grandchild 1'
),
html.Div(
id='output-1-grandchild-2',
children='output 1 grandchild 2'
),
dcc.Input(id='output-1-input', value='initial value 1'),
html.Div(id='output-1-grandchild-3')
]),
html.Div(id='output-2-child', children=[
html.Div(
id='output-2-grandchild-1',
children='output 2 grandchild 1'
),
html.Div(
id='output-2-grandchild-2',
children='output 2 grandchild 2'
),
dcc.Input(id='output-2-input', value='initial value 2'),
html.Div(id='output-2-grandchild-3'),
html.Div(id='output-2-grandchild-4'),
html.Div(id='output-2-grandchild-5'),
]),
]
else:
return ['output 1 text', 'output 2 text']
@app.callback(
[Output('output-1-grandchild-3', 'children'),
Output('output-2-grandchild-3', 'children')],
[Input('output-1-input', 'value'), Input('output-2-input', 'value')])
def callback1(value_1, value_2):
call_counts['callback1'].value += 1
return [
'output 1.3 - {} - {}'.format(value_1, value_2),
'output 2.3 - {} - {}'.format(value_1, value_2),
]
@app.callback(
[Output('output-2-grandchild-4', 'children'),
Output('output-2-grandchild-5', 'children')],
[Input('output-2-input', 'value')])
def callback2(value):
call_counts['callback2'].value += 1
return [
html.Div('ggc0 - output 2.4 - {}'.format(value), id='great-grandchild-0')
html.Div([
dcc.Input(id='great-grandchild-1', value='ggc1'),
[html.Div(id='great-grandchild-2'),
html.Div(id='great-grandchild-3')]
])
]
@app.callback(
[Output('great-grandchild-2', 'children'),
Output('great-grandchild-3', 'children')],
[Input('great-grandchild-1', 'value')])
def callback3(value):
call_counts['callback3'].value += 1
return [
'ggc2 - {}'.format(value),
'ggc3 - {}'.format(value),
]
def tree_assertions():
self.assertTextEqual('middle', 'text in the middle')
self.assertTextEqual('output-1-grandchild-1', 'output 1 grandchild 1')
self.assertTextEqual('output-1-grandchild-2', 'output 1 grandchild 2')
self.assertTextEqual('output-2-grandchild-1', 'output 2 grandchild 1')
self.assertTextEqual('output-2-grandchild-2', 'output 2 grandchild 2')
self.assertTextEqual('output-1-grandchild-3', 'output 1.3 - initial value 1 - initial value 2')
self.assertTextEqual('output-2-grandchild-3', 'output 2.3 - initial value 1 - initial value 2')
self.assertTextEqual('output-2-grandchild-4', 'output 2.4 - initial value 2')
self.assertTextEqual('great-grandchild-0', 'ggc0 - output 2.4 - initial value 2')
self.assertTextEqual('great-grandchild-1', 'ggc1')
self.assertTextEqual('great-grandchild-2', 'ggc2 - ggc1')
self.assertTextEqual('great-grandchild-3', 'ggc3 - ggc1')
self.assertEqual(call_counts['callback1'], 1)
self.assertEqual(call_counts['callback2'], 1)
self.assertEqual(call_counts['callback3'], 1)
sendkeys('output-1-input', '!')
self.assertTextEqual('output-1-grandchild-3', 'output 1.3 - initial value 1! - initial value 2')
self.assertTextEqual('output-2-grandchild-4', 'output 2.3 - initial value 1! - initial value 2')
self.assertEqual(call_counts['callback1'], 1)
self.assertEqual(call_counts['callback2'], 2)
self.assertEqual(call_counts['callback3'], 1)
sendkeys('output-2-input', '?')
self.assertTextEqual('output-1-grandchild-3', 'output 1.3 - initial value 1! - initial value 2?')
self.assertTextEqual('output-2-grandchild-4', 'output 2.3 - initial value 1! - initial value 2?')
self.assertTextEqual('output-2-grandchild-4', 'output 2.4 - initial value 2')
self.assertTextEqual('great-grandchild-0', 'ggc0 - output 2.4 - initial value 2?')
self.assertTextEqual('great-grandchild-1', 'ggc1')
self.assertTextEqual('great-grandchild-2', 'ggc2 - ggc1')
self.assertTextEqual('great-grandchild-3', 'ggc3 - ggc1')
self.assertEqual(call_counts['callback1'], 1)
self.assertEqual(call_counts['callback2'], 3)
self.assertEqual(call_counts['callback3'], 2)
sendkeys('great-grandchild-1', '$')
self.assertTextEqual('great-grandchild-2', 'ggc2 - ggc1$') self.assertTextEqual('great-grandchild-3', 'ggc3 - ggc1$')
self.assertEqual(call_counts['callback1'], 1)
self.assertEqual(call_counts['callback2'], 3)
self.assertEqual(call_counts['callback3'], 3)
# reset for the next call
call_counts['callback1'] = 0
call_counts['callback2'] = 0
call_counts['callback3'] = 0
def string_assertions(callcounts):
self.assertTextEqual('output-1', 'output 1 text')
self.assertTextEqual('output-2', 'output 2 text')
tree_assertions(1)
click_on_radio_items('input', 'string')
string_assertions()
click_on_radio_items('input', 'tree')
tree_assertions()
click_on_radio_items('input', 'string')
string_assertions()
This set of tests test that multiple outputs can trigger other multiple outputs. There are lots of different DAG shapes that we should consider here (diamonds vs straight lines, etc). For these, I recommend just drawing out a bunch of different DAG shapes on paper. Here are some examples of different shapes (each newline represents a different callback) shape 1 - diamond
shape 2 - grandparent hooking in
(D shouldn't update until B and C are finished updating) shape 3 - multi-gen
(G shouldn't update until [C, E] and [D, F] have updated) shape 4 multi-gen with granparent hook
shape 5 - multi-gen fan out
+ there's probably many more unique shapes to consider! Here's an example of how the test for shape 1 might be written. call_counts = {
'bc': Value('i', 0),
'd': Value('i', 0),
}
app.layout = html.Div([
dcc.Input(id='a', value='a'),
dcc.Input(id='b'),
dcc.Input(id='c'),
dcc.Input(id='d'),
])
@app.callback([Output('b', 'value'), Output('c', 'value')],
[Input('a', 'value')])
def update_b_c(value):
call_counts['bc'].value += 1
return ['b - ({})'.format(value), 'c - ({})'.format(value)]
@app.callback(Output('d', 'value'),
[Input('b', 'value'), Input('c', 'value')])
def update_d(valueb, valuec):
call_counts['d'].value += 1
return 'd - ({}) - ({})'.format(valueb, valuec)
wait_for_text_to_equal('#d', 'd - (b - (a)) - (c - (a))')
wait_for_text_to_equal('#b', 'b - (a)')
wait_for_text_to_equal('#c', 'c - (a)')
self.assert_equal(call_counts['bc'].value, 1)
self.assert_equal(call_counts['d'].value, 1)
sendkeys('b', '!')
wait_for_text_to_equal('#d', 'd - (b - (a)!) - (c - (a))')
wait_for_text_to_equal('#b', 'b - (a)!')
wait_for_text_to_equal('#c', 'c - (a)')
self.assert_equal(call_counts['bc'].value, 1)
self.assert_equal(call_counts['d'].value, 2)
sendkeys('a', '?')
wait_for_text_to_equal('#d', 'd - (b - (a?)!) - (c - (a?))')
wait_for_text_to_equal('#b', 'b - (a?)!')
wait_for_text_to_equal('#c', 'c - (a?)')
self.assert_equal(call_counts['bc'].value, 2)
self.assert_equal(call_counts['d'].value, 3)
|
@T4rk1n it looks like we're not detecting circular dependencies when multi-output callbacks are in the loop. Consider: app.layout = html.Div([
dcc.Input(id='a'),
dcc.Input(id='b'),
html.P(id='c')
])
@app.callback(Output('a', 'value'), [Input('b', 'value')])
def set_a(b):
return ((b or '') + 'X')[:100]
# if I use set_b, we detect the loop
@app.callback(Output('b', 'value'), [Input('a', 'value')])
def set_b(a):
return a
# if I use set_bc and comment out set_b, we don't detect the loop,
# the app runs and eventually fills up with 100 X's
@app.callback([Output('b', 'value'), Output('c', 'children')],
[Input('a', 'value')])
def set_bc(a):
return [a, a] I'm thinking what we need (and will need for #475 as well, though in a somewhat different form) is a second dep graph that expands out the multiple outputs - in the first we (already) make a link: |
ed9d861
to
b366bd3
Compare
@alexcjohnson The circular dependency test is now passing.
That was the winner solution. 🎉 |
There's an error in the style tag of the |
Ooh interesting - how did you figure this out? I guess it's the same as in #120? cc @Marc-Andre-Rivet |
And the next question, what would it take to fix that? Is it our error, or is it in |
I downloaded percy sources, open the raw html in a browser, there's no css applied to dropdown unlike when running the test. In the editor I then see red: </style><style type="text/css">.Select,.Select-control{position:relative}.Select-control,.Select-input>input |
It is our error, before the style was in a style sheet, now it's included in the head, there must something that escape the css |
The |
Thanks @T4rk1n - let’s accept these broken images for now and follow up when we learn more from Percy in https://github.com/plotly/dash-core/issues/43 |
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.
💃 great looking tests. I thought I had a case that could happen with multi-output but not single-output, but on further reflection the case I had in mind isn't possible. So, I think you've got the important ones!
Needs plotly/dash#436
Tests are gonna fail until a rc version of dash has been released.
pip install dash-renderer==0.19.0rc1
try withdash==0.38.0rc1