Simplify backend switch #370
Conversation
Codecov Report
@@ Coverage Diff @@
## master #370 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 78 78
Lines 12733 12741 +8
=========================================
+ Hits 12733 12741 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
src/qibo/backends/__init__.py
Outdated
| "numpy": NumpyDefaultEinsumBackend, | ||
| "numpy_defaulteinsum": NumpyDefaultEinsumBackend, | ||
| "numpy_matmuleinsum": NumpyMatmulEinsumBackend | ||
| } |
There was a problem hiding this comment.
I am checking this PR, and I think we could be a little bit more specific for this dict.
What do you think if we include some docs so when you raise an error we can show some message like:
ValueError: Unknown backend *. Please select one of the available backends:
- custom: uses precompiled primitives, fastest simulation engine...
- tensorflow: same/alias as custom...
- defaulteinsum: uses tf primitives, and einsum, faster on GPU...
- matmuleinsum: ...
There was a problem hiding this comment.
Another motivation to do that is the follow: suppose I set qibo.set_backend("numpy"), then when calling qibo.get_backend() it prints numpy_defaulteinsum, so this may confuse the user, and in the worst case break some programmatic check.
There was a problem hiding this comment.
I updated the error when an invalid backend is given so that a list of the available backends is printed. You can check the error message by passing a wrong value on purpose (for example do qibo.set_backend("test")).
Regarding the second point, I agree that it may be a bit confusing that numpy and numpy_defaulteinsum are essentially the same backend (same for custom and tensorflow). The reason I did it this way is because I wanted to enable the user to do qibo.set_backend("numpy") as a shortcut for qibo.set_backend("numpy_defaulteinsum"). If you think it is confusing we can remove numpy and keep only the numpy_defaulteinsum option.
There was a problem hiding this comment.
@stavros11 just few comments concerning the layout, otherwise looks good to merge.
src/qibo/backends/__init__.py
Outdated
| calc_backend, gate_backend = gate_backend | ||
| if gate_backend == "custom": | ||
| calc_backend = "tensorflow" | ||
| bk = _construct_backend(calc_backend) | ||
| K.assign(bk) |
There was a problem hiding this comment.
If we replace this line with K = bk the bug in #374 seems to be fixed.
This sounds like a problem with the init global allocation and the _CONSTRUCTED_BACKENDS.
One possibility to avoid this issue consists in always reuse the backends from _CONSTRUCTED_BACKENDS.
There was a problem hiding this comment.
I just pushed some updates in the backend switching mechanism that fixes the device issue and avoids the .assign function. I think the idea is close to one of the solutions we were discussing earlier today. I still need to polish several things, fix the tests and your above comments but let me know if you have any comments on this implementation.
There was a problem hiding this comment.
Thanks, looks pretty good, and indeed it is quite close to my draft in #374. I think we can keep your approach (without the new method), given that you already expose all required features!
|
Thanks for checking. I made some additional updates regarding your previous comments above. If CI passes I think this PR is okay now. |
Simplifies the backend switching mechanism by creating a seperate backend class for each of the einsum backends. For example currently we have a single
NumpyBackendclass that handles bothnumpy_defaulteinsumandnumpy_matmuleinsum. In this PR I seperate the backends by create different classesNumpyDefaultEinsumBackendandNumpyMatmulEinsumBackend. I believe this allows a cleaner switching function and may be easier to maintain if we add more backends either for simulation or hardware.There is no change from the user perspective, that is the user can still switch the backends using
qibo.set_backend("custom"),qibo.set_backend("defaulteinsum"),qibo.set_backend("numpy_defaulteinsum"), etc..