Skip to content

Missing inputs #1212

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

Merged
merged 5 commits into from
Apr 28, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
- [#1078](https://github.com/plotly/dash/pull/1078) Permit usage of arbitrary file extensions for assets within component libraries

### Fixed
- [#1212](https://github.com/plotly/dash/pull/1212) Fixes [#1200](https://github.com/plotly/dash/issues/1200) - prior to Dash 1.11, if none of the inputs to a callback were on the page, it was not an error. This was, and is now again, treated as though the callback raised PreventUpdate. The one exception to this is with pattern-matching callbacks, when every Input uses a multi-value wildcard (ALL or ALLSMALLER), and every Output is on the page. In that case the callback fires as usual.
- [#1201](https://github.com/plotly/dash/pull/1201) Fixes [#1193](https://github.com/plotly/dash/issues/1193) - prior to Dash 1.11, you could use `flask.has_request_context() == False` inside an `app.layout` function to provide a special layout containing all IDs for validation purposes in a multi-page app. Dash 1.11 broke this when we moved most of this validation into the renderer. This change makes it work again.

## [1.11.0] - 2020-04-10
Expand Down
2 changes: 2 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ $ cd dash-renderer
$ npm run build # or `renderer build`
# install dash-renderer for development
$ pip install -e .
# build and install components used in tests
$ npm run setup-tests
# you should see both dash and dash-renderer are pointed to local source repos
$ pip list | grep dash
```
Expand Down
104 changes: 86 additions & 18 deletions dash-renderer/src/actions/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,6 @@ function unwrapIfNotMulti(paths, idProps, spec, anyVals, depType) {
']'
);
}
// TODO: unwrapped list of wildcard ids?
// eslint-disable-next-line no-console
console.log(paths.objs);
throw new ReferenceError(
'A nonexistent object was used in an `' +
depType +
Expand Down Expand Up @@ -247,20 +244,47 @@ async function fireReadyCallbacks(dispatch, getState, callbacks) {

let payload;
try {
const outputs = allOutputs.map((out, i) =>
unwrapIfNotMulti(
paths,
map(pick(['id', 'property']), out),
cb.callback.outputs[i],
cb.anyVals,
'Output'
)
);
const inVals = fillVals(paths, layout, cb, inputs, 'Input', true);

const preventCallback = () => {
removeCallbackFromPending();
// no server call here; for performance purposes pretend this is
// a clientside callback and defer fireNext for the end
// of the currently-ready callbacks.
hasClientSide = true;
return null;
};

if (inVals === null) {
return preventCallback();
}

let outputs;
try {
outputs = allOutputs.map((out, i) =>
unwrapIfNotMulti(
paths,
map(pick(['id', 'property']), out),
cb.callback.outputs[i],
cb.anyVals,
'Output'
)
);
} catch (e) {
if (e instanceof ReferenceError && !flatten(inVals).length) {
// This case is all-empty multivalued wildcard inputs,
// which we would normally fire the callback for, except
// some outputs are missing. So instead we treat it like
// regular missing inputs and just silently prevent it.
return preventCallback();
}
throw e;
}

payload = {
output,
outputs: isMultiOutputProp(output) ? outputs : outputs[0],
inputs: fillVals(paths, layout, cb, inputs, 'Input'),
inputs: inVals,
changedPropIds: keys(cb.changedPropIds),
};
if (cb.callback.state.length) {
Expand Down Expand Up @@ -360,14 +384,18 @@ async function fireReadyCallbacks(dispatch, getState, callbacks) {
updatePending(pendingCallbacks, without(updated, allPropIds));
}

function handleError(err) {
function removeCallbackFromPending() {
const {pendingCallbacks} = getState();
if (requestIsActive(pendingCallbacks, resolvedId, requestId)) {
// Skip all prop updates from this callback, and remove
// it from the pending list so callbacks it was blocking
// that have other changed inputs will still fire.
updatePending(pendingCallbacks, allPropIds);
}
}

function handleError(err) {
removeCallbackFromPending();
const outputs = payload
? map(combineIdAndProp, flatten([payload.outputs])).join(', ')
: output;
Expand Down Expand Up @@ -398,9 +426,12 @@ async function fireReadyCallbacks(dispatch, getState, callbacks) {
return hasClientSide ? fireNext().then(done) : done;
}

function fillVals(paths, layout, cb, specs, depType) {
function fillVals(paths, layout, cb, specs, depType, allowAllMissing) {
const getter = depType === 'Input' ? cb.getInputs : cb.getState;
return getter(paths).map((inputList, i) =>
const errors = [];
let emptyMultiValues = 0;

const fillInputs = (inputList, i) =>
unwrapIfNotMulti(
paths,
inputList.map(({id, property, path: path_}) => ({
Expand All @@ -411,8 +442,45 @@ function fillVals(paths, layout, cb, specs, depType) {
specs[i],
cb.anyVals,
depType
)
);
);

const tryFill = (inputList, i) => {
try {
const inputs = fillInputs(inputList, i);
if (isMultiValued(specs[i]) && !inputs.length) {
emptyMultiValues++;
}
return inputs;
} catch (e) {
if (e instanceof ReferenceError) {
errors.push(e);
return null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering what would be required to be able to write this (and https://github.com/plotly/dash/pull/1212/files#diff-15692ba768eeab56e95aa409ab7c19b7R279) so that we don't use exceptions as flow control.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

don't use exceptions as flow control

yeah I suppose that's cleaner now that they aren't always errors -> b44bd54

// any other error we still want to see!
throw e;
}
};

const inputVals = getter(paths).map(allowAllMissing ? tryFill : fillInputs);

if (errors.length) {
if (errors.length + emptyMultiValues === inputVals.length) {
// We have at least one non-multivalued input, but all simple and
// multi-valued inputs are missing.
// (if all inputs are multivalued and all missing we still return
// them as normal, and fire the callback.)
return null;
}
// If we get here we have some missing and some present inputs.
// That's a real error, so rethrow the first missing error.
// Wildcard reference errors mention a list of wildcard specs logged
// TODO: unwrapped list of wildcard ids?
// eslint-disable-next-line no-console
console.log(paths.objs);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this meant to be console.error?

throw errors[0];
}

return inputVals;
}

function handleServerside(config, payload, hooks) {
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
"format": "run-s private::format.*",
"initialize": "run-s private::initialize.*",
"lint": "run-s private::lint.*",
"test.integration": "run-s private::test.setup-* private::test.integration-*",
"setup-tests": "run-s private::test.setup-*",
"test.integration": "run-s setup-tests private::test.integration-*",
"test.unit": "run-s private::test.unit-**"
},
"devDependencies": {
Expand Down
Loading