introduce abstract base class for backends #84
Conversation
With this change, backend modules should inherent from BaseBackend and will need to implement a small set of methods for them to be registered as working backends. Also, the runnable backend modules are put into the namespace so e.g. they get included in the sphinx docs.
There was a problem hiding this comment.
I'm really glad to see this PR, @ewinston! I feel it provides a much more consistent and robust way of defining local backends, making it more friendly for developers.
As hinted during the documentation PR, I feel this would be a really good time to tackle the way the backends are discovered. I understand the motivation on that comment, but going case by case and making hopefully reasonable assumptions:
- if a user uses
qiskitas a library and wants to implement their own simulator, currently the autodiscover mechanism will only pick up their simulator if it is placed on thePYTHON_LIBRARY_PATH/qiskit/backend/, which is unlikely to be there if installed via pip. - if a QISKit developer is implementing a custom simulator that might not be part of the SDK, it seems safe to assume he would be comfortable declaring it elsewhere or modifying another file on the library to make it work.
- if we implement a simulator in the future for being available in the SDK, it will end up being discovered every time - making the autodiscovery a bit redundant.
I might be missing some use cases and would welcome some input, but for these three cases we are mostly trading quite a bit of "niceties" (clarity, the ability for the users to discover the backends using their development tools, documentation, etc) compared to using the standard import mechanism. Would it be worth discussing a bit the available options and try to address it on this PR?
qiskit/backends/_basebackend.py
Outdated
| @configuration.setter | ||
| @abstractmethod | ||
| def configuration(self, configuration): | ||
| """Set backend configuration""" |
There was a problem hiding this comment.
Is it likely that any local backend will do something different than return self._configuration and self._configuration = configuration on these two functions?
All of the existing simulators seem to be have exactly the same implementation for these two methods, and it might be worth just moving the implementation for these two functions to BaseBackend directly (and not making them @abstractmethods) to avoid duplication - if in the end a future simulator needs to do something different it can just override them anyway.
There was a problem hiding this comment.
I did this in case a user wanted to do some kind of module specific validation of configuration but since I don't currently have a clear picture of this perhaps it would be ok to implement it in BaseBackend.
qiskit/backends/_basebackend.py
Outdated
| self._configuration = None # IMPLEMENT for your backend | ||
|
|
||
| @abstractmethod | ||
| def run(self): |
There was a problem hiding this comment.
With the recent changes, would it be a good idea to define run_circuit(...) as an @abstractmethod, this enforcing the simulators to provide and implement it? Currently, it seems it is never called directly outside each module (ie. it is always called from FooSimulator.run()), but I'm not sure if the goal is to eventually use run_circuit() directly elsewhere.
There was a problem hiding this comment.
Since we have changed to using qobj for communicating tasks the simulator, and perhaps the online backends, don't need to run circuits individually. For instance a c simulator can take in the whole qobj. The run_circuit() method is sort of a non-essential fix to get the python simulators to work with qobj without making too many changes. It seems like a good method to have for python simulators but some simulators might want to do optimizations on batches of circuits which don't need running individual ones. At least this way we make it optional.
There was a problem hiding this comment.
It sounds sensible, thanks for the clarification - I was mostly not sure how "universal" the run_circuit() was meant to be, and keeping it just on the simulator that needs it sounds fine to me!
qiskit/backends/_basebackend.py
Outdated
| FileNotFoundError if backend executable is not available. | ||
| """ | ||
| self._qobj = qobj | ||
| self._configuration = None # IMPLEMENT for your backend |
There was a problem hiding this comment.
Can we also at least document the format of the configuration somewhere on the docstrings of this file, to make as clear as possible for users interested in subclassing it? Going a bit further, we might want to make some configuration values class methods, for example name (so they are available as MySimulator.name) to further enforce them (I'm picking name as an example as it is the identifier that is later used to refer to the simulator by the user).
There was a problem hiding this comment.
I think the nice thing about the discovery is that the backend developer does not have to put import statements in any qiskit file for the backend to be used. They only need to know the name of the backend they put into the configuration of their own backend module. Even for the number of backends we have this keeps the core code cleaner.
I think you could be right about the user putting the module in the backends folder if they've installed with pip. Perhaps if they are developing backends they are more likely to use a git repo and would be comfortable with putting modules there. Still, maybe it's strange to have "local" modules mixed with qiskit ones. Perhaps we can have a ".qiskit/backends" folder in their home directory, or other locations, which are also checked.
Another nice thing about the discovery mechanics is that if, for instance wants to make several versions of a compiled simulator (e.g. different optimizations) it would be easier to implement.
We could also use the discovery function to "discover" online backends and make them subclass BaseBackend. This would make us less dependent on changes to the set of available online backends.
I'm a little unclear about what niceties we are giving up. In the previous discovery I intentionally hid backends and just made LocalSimulator the only one that was visible. With this PR I import all the backend modules into the namespace as you suggested so you should see no difference from doing a normal import (that was the goal anyway). For instance documentation and tab completion seems to be available now.
Add "qiskit.backends._backendutils.register_backend()" as the main function for registering a local backend, including in turn: * revise the autodiscovery function "discover_sdk_backends()", making use of "register_backend()" and dropping compatibility with old-style definition of backends (ie. using a "__configuration" module variable". * merge the module-level variables "_backend_classes" and "_backend_configurations" into a single "_REGISTERED_BACKENDS" dict that uses namedtuples, for consistency. * update "local_backends()" and "remote_backends()" to make use of the new merged variable. * reorganize the "qiskit.backend.__init__" imports to expose only the methods needed by the user. * update the tests and add tests for "register_backend()".
Change "BaseBackend.configuration" to a non-abstract property, dropping the setter, as all the current backends share the same implementation, in order to avoid code duplication. Custom backends can still override the method directly if needed.
There was a problem hiding this comment.
Thanks for the latest changes! As a summary of the conversations, the backends are now registered via a new public register_backend() function, which is used during the autodiscovery and can be used by users to register backends outside the SDK.
There is still some work to do once we tackle the bigger issue of better integrating the online backends, but looks ready to merge to me - maybe except the _qelocal.py file?
introduce abstract base class for backends
Description
With this change, backend modules should inherent from BaseBackend
and will need to implement a small set of methods for them to be
registered as working backends. Also, the runnable backend modules are put
into the namespace so e.g. they get included in the sphinx docs.
Motivation and Context
This change encourages a common structure for backend modules.
How Has This Been Tested?
make test
make doc
Screenshots (if appropriate):
Types of changes
Checklist: