Update the engine code to use gRPC instead of REST #2675
Conversation
lindmarkm
commented
Jan 11, 2020
lindmarkm
commented
- Added a generated gRPC client for Quantum Engine.
- Update all serialization code from proto dicts to the actual protos.
- Updated the engine and engine_job to use the gRPC client.
| @@ -15,19 +15,19 @@ | |||
| from cirq.google import api | |||
|
|
|||
| from cirq.google.api.v1.params import ( | |||
| sweep_to_proto_dict, | |||
| sweep_from_proto_dict, | |||
There was a problem hiding this comment.
So, these seem relatively obscure, but they are all exposed at the cirq.google level. Have you checked to make sure nothing is using these (for instance, in pyle)?
There was a problem hiding this comment.
I don't expect there are many uses and the couple I know of are simple to adapt. Is there a way to find other possible uses? I'm honestly not certain why they are exposed at this level at all and would be happy to remove all the serialization methods from here. Thoughts?
There was a problem hiding this comment.
I think we should remove it from the init. If people still need it, they can use cirq.google.api.v1.params.sweep_to_proto, but then they know they are treading on unsafe ground.
There was a problem hiding this comment.
Sounds good to me. Removed all the to and from proto functions.
| def sweep_from_proto_dict(param_sweep: Dict) -> sweeps.Sweep: | ||
| if 'sweep' in param_sweep and 'factors' in param_sweep['sweep']: | ||
| def sweep_from_proto(param_sweep: params_pb2.ParameterSweep) -> sweeps.Sweep: | ||
| if param_sweep.HasField('sweep') and len(param_sweep.sweep.factors) > 0: |
There was a problem hiding this comment.
Does this work if the len=1? If len=1, should it still be a Product?
There was a problem hiding this comment.
The serialization functionality should be equivalent. Swapping from REST to gRPC was a big enough change that I've tried to avoid making any other changes to behavior. If you're asking if the method should optimize the factors = 1 case to return _sweep_from_param_sweep_zip_proto(param_sweep.sweep.factors[0]) then possibly? Perhaps a future PR.
There was a problem hiding this comment.
I mean, if factors=1, should it even be a zip? Shouldn't it then just be a singlesweep of points or linspace?
There was a problem hiding this comment.
I agree, but in an attempt to limit this PR I'm aiming for being functionally the same. A new small PR should probably exist to remove the unneeded sweeps.
| if 'sweeps' in param_sweep_zip: | ||
| def _sweep_from_param_sweep_zip_proto(param_sweep_zip: params_pb2.ZipSweep | ||
| ) -> sweeps.Sweep: | ||
| if len(param_sweep_zip.sweeps) > 0: |
There was a problem hiding this comment.
Same concern here about the case where len == 1.
| # See the License for the specific language governing permissions and | ||
| # limitations under the License. | ||
|
|
||
| from __future__ import absolute_import |
There was a problem hiding this comment.
I'm not sure I get what the point of this file is. Can you explain it? Is it basically just a way of pointing your client to "latest" which is v1alpha1 now?
(And also possibly add some doc to the file?)
There was a problem hiding this comment.
All of the files and structure under google/engine/client are lightly-modified from generated code so apart from tweaking it slightly for the package structure I've avoided making changes to it to reduce the size of future diffs. I believe you are correct with the idea of using it as a pointer to support multiple versions or future beta versions.
There was a problem hiding this comment.
Should we have a Readme.md or equivalent explaining that these are generated code so no one directly modifies them? (possibly including instructions/pointers on how to generate it?)
There was a problem hiding this comment.
The generator is only partially public so I don't think this is the best place for a readme. I think a lot will depend on the scale of changes that need to be made. New versions or a number of signature changes and new methods likely warrant a re-generation. For smaller changes I expect it to be easier to hand modify these in place. I suppose that makes it more palatable to further massage these into a format we are happier with. Thoughts?
| @@ -1,6 +1,8 @@ | |||
| # Runtime requirements for the python 3 version of cirq. | |||
|
|
|||
| dataclasses; python_version < '3.7' | |||
| enum34; python_version < "3.4" | |||
| google-api-core[grpc] >= 1.14.0, < 2.0.0dev | |||
There was a problem hiding this comment.
How much of an overhead is this? I am (slightly) concerned about pulling in a bunch of google stuff for people who are using cirq for non-engine related tasks.
There was a problem hiding this comment.
Thanks, I forgot to actually remove the google-api-python-client dependency.
Fair. I'm not certain how to qualify the overhead. It definitely adds "unneeded" dependencies for people who are not using the engine. We talked about not requiring this on install and only complaining at runtime if a user attempted to create an engine or having the client as a new optional module but settled on keeping it in cirq for now.
requirements.txt
Outdated
| @@ -1,7 +1,8 @@ | |||
| # Runtime requirements for the python 3 version of cirq. | |||
|
|
|||
| dataclasses; python_version < '3.7' | |||
| google-api-python-client~=1.6 | |||
| enum34; python_version < "3.4" | |||
There was a problem hiding this comment.
I don't think we (or anyone) supports python < 3.4, so the enum backport is probably unnecessary
There was a problem hiding this comment.
Thanks. Removed.
