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

Disable jsonschema validation by default #688

Merged
merged 2 commits into from
Apr 14, 2020

Conversation

mtreinish
Copy link
Member

Summary

Currently the aer backends are running jsonschema validation on the Qobj
objects passed in via terra by default. This is unnecessary overhead as
terra is programatically creating these objects and they're extremely
unlikely to be structurally invalid (which is all jsonschema will
catch). Even, if it was this will be caught later when something goes to
use it and the error message from that is more likely to be useful since
the way the json schemas are constructed precludes useful error message.
This commit changes the default value of this to be opt-in on the run()
method to avoid this unnecessary overhead.

Details and comments

Currently the aer backends are running jsonschema validation on the Qobj
objects passed in via terra by default. This is unecessary overhead as
terra is programatically creating these objects and they're extremely
unlikely to be structurally invalid (which is all jsonschema will
catch). Even, if it was this will be caught later when something goes to
use it and the error message from that is more likely to be useful since
the way the json schemas are constructed precludes useful error message.
This commit changes the default value of this to be opt-in on the run()
method to avoid this unecessary overhead.
@mtreinish mtreinish added the Changelog: API Change Include in the Changed section of the changelog label Apr 10, 2020
@mtreinish
Copy link
Member Author

The test failures here are unrelated. It looks like they're caused by Qiskit/qiskit#4016 which merged this morning

@atilag
Copy link
Member

atilag commented Apr 11, 2020

I agree to disable json schema validation by default temporary, while we come up with a better validation strategy (I'm currently working on it). Anyway, after analyzing several profiling reports, the time spent validating in large simulations can be considered negligible. I can't think of any real use-case where this is a real problem, apart from competitive benchmarkings of course :)

@atilag
Copy link
Member

atilag commented Apr 11, 2020

Just a reminder: If we decide to disable schema validation by default, we have to coordinate with our cloud simulator team so they explicitly activate validation

@mtreinish
Copy link
Member Author

This is independent of the cloud simulator. As I said in the commit message this is just for the aer provider. The cloud simulator runs in standalone mode so this won't affect it. Additionally, the qobj is validated prior to the standalone aer simulator receiving it.

@chriseclectic
Copy link
Member

@atilag this disabling is fine as we basically validate twice anyway, once in python and again when we are parsing the JSON into C++. If we were disabling the checking of the C++ validation (which is not done via schema, but by explicitly checking instructions for things like right number of qubits and no duplicate qubits etc) we would need to coordinate with cloud team.

@atilag
Copy link
Member

atilag commented Apr 13, 2020

No, let's only disable the Python schema validation for now.
Ok, I want to merge this once tests passes (I know the problem it's not related to this PR, but let's fix it and re-run all the tests).

@chriseclectic chriseclectic added the stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable label Apr 14, 2020
@chriseclectic chriseclectic added this to the Aer 0.5.1 milestone Apr 14, 2020
@chriseclectic chriseclectic merged commit 916cf69 into Qiskit:master Apr 14, 2020
hhorii pushed a commit to hhorii/qiskit-aer that referenced this pull request Apr 16, 2020
Currently the aer backends are running jsonschema validation on the Qobj
objects passed in via terra by default. This is unecessary overhead as
terra is programatically creating these objects and they're extremely
unlikely to be structurally invalid (which is all jsonschema will
catch). Even, if it was this will be caught later when something goes to
use it and the error message from that is more likely to be useful since
the way the json schemas are constructed precludes useful error message.
This commit changes the default value of this to be opt-in on the run()
method to avoid this unecessary overhead.

Co-authored-by: Christopher J. Wood <cjwood@us.ibm.com>
chriseclectic added a commit to chriseclectic/qiskit-aer that referenced this pull request Apr 20, 2020
Currently the aer backends are running jsonschema validation on the Qobj
objects passed in via terra by default. This is unecessary overhead as
terra is programatically creating these objects and they're extremely
unlikely to be structurally invalid (which is all jsonschema will
catch). Even, if it was this will be caught later when something goes to
use it and the error message from that is more likely to be useful since
the way the json schemas are constructed precludes useful error message.
This commit changes the default value of this to be opt-in on the run()
method to avoid this unecessary overhead.

Co-authored-by: Christopher J. Wood <cjwood@us.ibm.com>
@mtreinish mtreinish deleted the remove-jsonschema-validation branch April 22, 2020 19:35
mtreinish added a commit to mtreinish/qiskit-aer that referenced this pull request Apr 24, 2020
A lone release note file accidently got commited to qiskit/providers/aer
in Qiskit#688. This commit corrects this oversight and moves the file to the
proper location under releasenotes/notes.
atilag pushed a commit that referenced this pull request Apr 27, 2020
A lone release note file accidently got commited to qiskit/providers/aer
in #688. This commit corrects this oversight and moves the file to the
proper location under releasenotes/notes.
chriseclectic pushed a commit to chriseclectic/qiskit-aer that referenced this pull request May 30, 2020
A lone release note file accidently got commited to qiskit/providers/aer
in Qiskit#688. This commit corrects this oversight and moves the file to the
proper location under releasenotes/notes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: API Change Include in the Changed section of the changelog stable-backport-potential The issue or PR might be minimal and/or import enough to backport to stable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants