-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Backend rework, adding compile and execute #376
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.
@jaygambetta This is starting to look good for the user! A few comments:
-
I was thinking example level 0 would be the simplest and level 2 the one that goes into the details of compile/pass manager/etc. Currently it is the reverse.
-
I think the
compile_config
is a bit counter-intuitive.
First, I think the backend should come out and be an explicit input to bothcompile
andexecute
. It is as important to compile/execute as the circuits themselves.
Second, I wouldn't mind unpacking the entire config dict. I know Jay likes the config, but the advantage of unpacking it is that the default values become explicit, rather than buried in aCOMPILE_CONFIG_DEFAULT
in the source code. It is not uncommon for Python functions to have long signatures with a bunch of optional args---you only set them when you need them. e.g:
https://docs.scipy.org/doc/scipy/reference/generated/scipy.optimize.curve_fit.html
@ajavadia i dont mind either way. I will let you and the team decide what is best and more standard for these. My preference for going lower in numbers for more control is it is like lower level in the stack. But i have to admit that i did it they way you are proposing first. As for unpacking the config i dont mind -- my concern is that as we extend and add openpulse and extensions i dont want to force configs to have options that are not used. A simple example is why do we give the seed to a compile for a real device. I know i am stilling doing this but with a dictionary we can envision making this nice in the future. |
# I think we need to make a qobj into a class | ||
|
||
# Runing the job | ||
sim_result = my_backend.run(qiskit.QuantumJob(qobj, preformatted=True)) |
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.
Do we really want to expose QuantumJob
? AFAIK QuantumJob
is basically a qobj with a timeout and probably another connection related attribute, right? So I was thinking about using something like:
sim_result = my_backend.run( qobj, timeout='600')
Passing the result of a "compilation" (qobj actually) to a the run()
method directly, and adding connection related attributes as parameters.
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 agree we should remove it. I agree there is no need for QuantumJob. but i dont want to remove it until we have the qobJ standardized.
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.
Just to close this, I've been talking with @ajavadia and he showed me that conceptually it's good to have a Job and a Result separately, as someone can ask for specific things of a Job, like where in the queue is it, or what's it state... something that doesn't fit very well with the concept of a 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.
cool. i also think that the run and the job allows all the cool async stuff that you @diego-plan9 and @ewinston did to become the default (not in this pr, but one after it :-))
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.
Ok, don't want to jump to conclusions for now. Let's postpone this discussion :)
qiskit/wrapper/__init__.py
Outdated
return qiskit._compiler.compile(list_of_circuits, backend, compile_config) | ||
|
||
|
||
def execute(list_of_circuits, backend_id, compile_config=None, |
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.
-
In this function and the one above, can we change
backend_id
->backend
? Then it will be very similar to what users were using before inquantumprogram
, they just now switch towrapper
. Anyone usingwrapper
will probably just use strings anyway. If you're really worried about confusion with backend objects, then I would saybackend_name
is better thanbackend_id
. -
I really think the
compile_config
is hard to use. It is unclear what configurations are inside it, and it is less clear what the default config dictionary is. I think we should expand it to individual function arguments, and give explicit defaults for each. Here we have made the function signature compact, and shifted the burden to the user.
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, clarity was the goal - I feel it is better to be explicit, and reserve
backend
for actual instances.backend_name
sounds good (I preferid
as it places more emphasis on it being a "unique identifier", but either is fine). -
I'm on the same page - it is hard to declare and document (it touches the whole issue of using dicts). Since a small revision of
qiskit._compile
should be done after this PR, I'm hoping we can tackle thecompile_config
dict as a whole during that one and update the wrapper function accordingly if needed.
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.
ok i agree
|
||
try: | ||
import Qconfig | ||
qiskit.wrapper.register(Qconfig.APItoken, Qconfig.my_config['url']) |
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.
my_config['url']
-> config['url']
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.
Thanks, good catch!
print("\n(Remote Backends)") | ||
print(qiskit.wrapper.remote_backends()) | ||
|
||
# Compile and run the Quantum Program on a real device 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.
remove reference to Quantum Program in the comment
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 - I was not aiming for having the using_qiskit_core_level_x
files totally polished during this PR round, "only" to be runnable, and there might be more leftovers there.
qiskit/tools/qcvv/tomography.py
Outdated
@@ -694,7 +694,7 @@ def count_keys(n): | |||
############################################################################### | |||
|
|||
|
|||
def fit_tomography_data(tomo_data, method='wizard', options=None): | |||
def fit_tomography_data(tomo_data, method='wrapper', options=None): |
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.
Seems like the find-and-replace of wizard
--> wrapper
picked up some unintended cases here!
This file already had a wizard in it. But it was the only file thankfully. So we should just revert every wrapper
here to wizard
.
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.
🤦♂️ thanks for double-checking the rest of replacement as well!
qiskit/wrapper/__init__.py
Outdated
""" | ||
backend = _DEFAULT_PROVIDER.get_backend(backend_id) | ||
return qiskit._compiler.execute(list_of_circuits, backend, compile_config, | ||
wait, timeout) |
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 it common for this much code to go inside __init__
, or should these functions be in a separate file inside the wrapper
directory? I don't care that much, just curious if it is good style.
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 common practice, but bordering the "acceptable number of lines limit" from my personal preference point of view! I'll just move it to a file, to pave the way for future additions.
qiskit/wrapper/__init__.py
Outdated
Returns: | ||
list[str]: the names of the available backends. | ||
""" | ||
return _DEFAULT_PROVIDER.available_backends(filters) |
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.
This should go and query every authenticated provider for its available backends, and return an aggregated set of backends. Right now it only returns the local ones.
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.
Hmmm, the idea is that the DefaultQiskitProvider
is the one that aggregates the backends and acts as that layer - this way the wrapper is still thin, and future expansions at that level are implemented in that provider. I'll double check that it is working as intended, in any case.
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.
Just double checked:
In [1]: import qiskit
In [2]: qiskit.wrapper.available_backends()
Out[2]:
['local_sympy_qasm_simulator',
'local_qasm_simulator',
'local_unitary_simulator',
'local_sympy_unitary_simulator']
In [3]: import Qconfig
In [4]: qiskit.wrapper.register(Qconfig.APItoken, Qconfig.config['url'])
In [5]: qiskit.wrapper.available_backends()
Out[5]:
['local_sympy_qasm_simulator',
'local_qasm_simulator',
'local_unitary_simulator',
'local_sympy_unitary_simulator',
'ibmqx_qasm_simulator',
'ibmqx_hpc_qasm_simulator',
'ibmqx5',
'ibmqx4',
'ibmqx2']
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 sorry i read this wrong. i confused default provider with local provider.
Looks good!
qiskit/wrapper/__init__.py
Outdated
Returns: | ||
BaseBackend: a Backend instance. | ||
""" | ||
return _DEFAULT_PROVIDER.get_backend(name) |
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.
Similarly, this should query every provider for the named backend. It might have to return a list in some cases.
So Diego can see Fix for the quantum program with new backends Fixing the tests Small changes Linting Adding the qp to the compile section for completeness Doc changes naming Small changes Linting Small changes Moving the example WIP Backend example (#372) * Fix for the quantum program with new backends * Fixing the tests * Small changes * Linting * Update tests using coupling_map as list, add test Update the test in order to specify the coupling_maps as lists, due to recent changes. Add a `test_compile_coupling_map_as_dict` that still uses the dict format, for backwards compatibility during the deprecation. * Update coupling_map as list related docstrings Along with other fixes to docstrings and tests. * Disable travis osx build temporarily (#374) Temporarily disable the osx build on Travis, as they are having a backlog that might take some time to recover: https://www.traviscistatus.com/incidents/qkqy13yk55q9 * adding a missing coupling in test_quantumprogram
Does not work yet but im tied qiskit.api in __init__. update api use in hello_world. removing api.less_busy Small edits Adding backend name
doctstring improvements in qiskit/api backends registered with their API. remove backend name. replace backend with name in status/cal/par
Ignore the inputs to the compiler for now i think compile works Adding execute Small changes Fixing to pass test Linting fixes Rename variable Cleaning up Deciding to break all tests for the greater good :-) Adding a default name=None Making It pass again with the name gone from register Lint fix Updating the readme
Small changes to the backends Adding execute
…inition in method args. passing tests.
Renaming api backendmanager Adding using level1
expand _backend_manager methods. - reimplement some of the functionality in _backendutils.py in _backend_manager - generalize backend discovery. Now there is one method which can find both local and remote backends. - adds credentials to backend configuration - defaults to merging configuration parameter during initialization - register takes new keyword, "package", which gets searched for backends which derive from BaseBackend using the credentials in configuration if supplied.
separating result from qobj schema, and putting them in qiskit/schema fixes in jay's office Adding setup.py ignore to put the fire out Fixing some bugs disabling qasm header for extensions/qiskit_simulator Adding the examples working -- test need to fix the test. modifying quantumprogram to use backendmanager and its tests to pass Passing the testquantumprogram Passing backend now Style fixes Fixing some lint errors Renaming test_backend test_backend_manager Adding compile test Small changes Fix basebackend tests Also ignore ds_store files Linting don't traverse extensions when registering api add back in job_processor tests
used in tests for now.
Introduce `BaseProvider`, for supporting logical grouping of several providers into a single component. Add `LocalProvider` as the provider that handles the local simulators, replacing the autodiscovery functions (for local backends) of `_backendutils`. Add IBMQProvider, QeRemote api parameter Introduce `IBMQProvider` as the provider handling the backends provided through IBMQuantumExperience, moving the logic from `_backendutils` (in particular, the adaptation of the config). Adjust `QeRemote` instantiation by adding an `api` parameter which is set per instance, instead of the explicit `set_api()` class method. Remove the remote backends related functionality from `_backendutils`.
Add a thin `DefaultQISKitProvider` that makes use of `LocalProvider` and `IBMQProvider` for providing access to all the default backends defined in QISKit during a session. Add two module-level `register()` and `available_backends()` functions that make use of the `DefaultQISKitProvider`. An instance of this provider is created when importing `qiskit.wizard`.
Add basic filtering capabilities for `available_backends()`, by accepting a `filter` dict that is propagated to the calls to the underlying Providers.
Replace the usage of the functions in `_backend_manager` with: * for `QuantumProgram`, the equivalent functions from `qiskit.wizard`, with the assumption that users via the to-be-deprecated `QuantumProgram` would accept that only one connection to the API (or none) is allowed. The same assumption is used in the tests. * for the rest of the SDK, use the `Backend` objects directly. The same principle is applied to the tests. For decoupling, adjust the signature of several functions and methods in order to accept `Backend` objects directly, instead of receiving the name. Note that this is also applied to `QJob.qobj['config']['backend']`, and should be clarified when revising the QJob class. Lint, style and bugs fixes
* Removed BaseBackend tests
Revise the anonymous creation of `Registers`, simplifying it by using `itertools.count` in the same spirit as the current implementation in QuantumProgram. Fix a number of places where the arguments to the constructor were passed in the wrong order, avoiding a ton of deprecation warnings during the tests. Reintroduce the ability of passing an empty circuit list to QuantumProgram.compile(), which was lost during the transition. Add new (and not complete) tests to `test_identifiers` that mimic the tests for QuantumProgram anonymous features. Reorganize qiskit.backends: * move the local simulators to `qiskit.backends.local` * move the remote backend to `qiskit.backends.ibmq` Remove the underscore from the backend files, as they are meant to be used publicly. Rename `IbmQ` to `IBMQBackend` in order to clarify its usage and avoid using a too generic name. Update references in tests and files accordingly. Unify handling of backend configurations Revise `BaseBackend`, in order to unify the handling of the `configuration` attribute. The default configuration for each local backend is declared as a class attribute, and used by the base initialization if it is not overridden during instantiation of the subclasses. For the `IBMQBackend`, `configuration` is mandatory. Drop the usage of ABC for simplicity, as the main benefits provided by ABC are currently not being used. Fix appveyor dependency on jsonschema Move compile, execute to wizard, tune examples Move `compile()` and `execute()` to `qiskit.wizard`, as thin wrappers over `qiskit._compiler` that accept the backend as string instead of the backend object. Update the using_qiskit_level examples for using the wizard.
This change was applied to BaseBackend and BaseProvider.
Rename the helper module to `qiskit.wrapper`, revising its docstrings in the process.
Revise the examples/python/using_qiskit examples to ensure they are runnable, and rename them. Revert wrong renaming in tomography Move the code from `qiskit.wrapper.__init__` to `qiskit.wrapper._wrapper`. Rename `backend_id` parameter to `backend_name` in the wrapper functions. Fix typos and small issues.
Update the providers so that `available_backends()` returns instances directly instead of backend names, simplifying working with the components directly. Add a convenience `__str__` method for being able to print the name of a backend from an instance, and adjust tests accordingly. The wrapper's `available_backends()` function (and as a result, the `QuantumProgram` methods) still do return strings instead of instances, with the idea of keeping it compatible and user-friendly. Minor adjustments in other files. Fixing hello_quantum
Revise after the modifications to the C++ simulator, updating the references for the location of the file (`qiskit/backends/local` instead of `qiskit/backends`). Reintroduce the c++ simulator gates, that were commented out in the process.
Accept instances instead of backend names in wrapper.compile(). Use wrapper in `QuantumProgram.compile()`.
Thanks everyone for the work on this rather huge PR - finally merging! 🎉 |
Backend rework, adding compile and execute
Removing more of the sdk from the quantum program and fleshing out the backends more
Description
Examples
Motivation and Context
How Has This Been Tested?
Types of changes
Checklist: