Add batch support to cirq Engine client #3114

dstrain115 · 2020-06-29T15:04:49Z

Adds a run_batch() call to engine that can handle
lists of circuits and lists of programs.
Creates BatchProgram and BatchRunContext objects.
Pipes the objects through engine program and engine job.

- Adds a run_batch() call to engine that can handle lists of circuits and lists of programs. - Creates BatchProgram and BatchRunContext objects. - Pipes the objects through engine program and engine job.

maffoo · 2020-06-29T16:17:45Z

cirq/google/engine/engine.py

+        code = qtypes.any_pb2.Any()
+        batch = v2.batch_pb2.BatchProgram()
+        for program in programs:
+            batch.programs.append(gate_set.serialize(program))


One thing you can do here is to instead serialize directly into the existing message:

for program in programs: gate_set.serialize(program, msg=batch.programs.add())

Yeah, I guess this would get rid of the need for a copy. Done.

maffoo · 2020-06-29T16:24:24Z

cirq/google/engine/engine_job.py

@@ -285,6 +285,10 @@ def results(self) -> List[study.TrialResult]:
                  result_type == 'cirq.api.google.v2.Result'):
                v2_parsed_result = v2.result_pb2.Result.FromString(result.value)
                self._results = self._get_job_results_v2(v2_parsed_result)
+            elif result_type == 'cirq.google.api.v2.BatchResult':


Could use the built-in Any mechanism for checking types here:

elif result.Is(v2.batch_pb2.BatchResult.DESCRIPTOR): ...

We could change other cases as well though we'd have to leave in code to handle old cirq.api.google types if we still need that.

Done. I will leave the other cases how they are for now.

maffoo · 2020-06-29T16:28:35Z

cirq/google/engine/engine_job.py

+            for sweep_result in sweep_results:
+                for trial_result in sweep_result:
+                    trial_results.append(trial_result)
+        return trial_results


Could use the existing _get_job_results_v2 function here:

@classmethod def _get_batch_results_v2(cls, ...): trial_results = [] for result in results.results: trial_results.extend(cls._get_job_results_v2(result)) return trial_results

maffoo · 2020-06-29T16:31:51Z

cirq/google/engine/engine_job.py

+            elif result_type == 'cirq.google.api.v2.BatchResult':
+                v2_parsed_result = v2.batch_pb2.BatchResult.FromString(
+                    result.value)
+                self._results = self._get_batch_results_v2(v2_parsed_result)


It seems rather odd to flatten this down to a single list of results across all programs in the batch. I would think we might want a new API to access batch results in a more structured manner.

Yeah, my plan is to have something like that in a future PR that extends the interface. This will fit into the current implementation, and then we can have a new API to grab batch results in a follow-up.

Sound good?

dabacon

Drive by :)

Do we want to make EngineProgram and EngineJob be able to know if they were batch or not? Right now I don't think you won't be able to tell this easily?

dabacon · 2020-06-29T23:15:26Z

cirq/google/engine/engine.py

+            job_id: Optional[str] = None,
+            params_list: List[study.Sweepable] = None,
+            repetitions: int = 1,
+            processor_ids: Sequence[str] = ('xmonsim',),


suggest not having a default and yelling if not supplied. this is different from other cases, but there is an issue out to not have this default (it is too dangerous, you could think you were doing an experiment, but actually be running on a simulator).

dabacon · 2020-06-29T23:17:43Z

cirq/google/engine/engine.py

+                to run the program. Only one of these will be scheduled for
+                execution.
+            gate_set: The gate set used to serialize the circuit. The gate set
+                must be supported by the selected processor.


not a fan of the default being xmon here, but we should at least document (yes run doesn't document this too)

Changed it to raise an error, like the other parameters.

…115/Cirq-1 into add_batch_support_engine

dstrain115 · 2020-06-30T13:43:17Z

Do we want to make EngineProgram and EngineJob be able to know if they were batch or not? Right now I don't think you won't be able to tell this easily?

Acknowledged but have not addressed yet. Will fix before submitting.

dstrain115 · 2020-07-01T15:26:36Z

Acknowledged but have not addressed yet. Will fix before submitting.

I have added batch_mode to the classes. For EngineJob, it is purely informational.

dstrain115 · 2020-07-04T22:12:27Z

FYI: I gave this a try over the weekend and it worked! Woo woo!

After this is submitted, I think we are good to try this out with a limited set of testers.

maffoo

Minor comments, then LGTM.

maffoo · 2020-07-07T16:45:22Z

cirq/google/engine/engine_program.py

+            raise ValueError('No parameter list specified')
+
+        # Pack the run contexts into batches
+        batch_context = qtypes.any_pb2.Any()


I'd prefer to import any_pb2 directly here, rather than rely on it being "reexported" by the qtypes module, but it looks like this is the pattern we're using throughout this module.

Also, consider creating this below, after populating the batch message, when it's ready to be packed.

Moved batch below the for loop. Also changed this to import any_pb2 directly.

maffoo · 2020-07-07T16:47:16Z

cirq/google/engine/engine_program.py

+            job_id: Optional[str] = None,
+            params_list: List[study.Sweepable] = None,
+            repetitions: int = 1,
+            processor_ids: Sequence[str] = ('xmonsim',),


Use empty tuple for the default value, as in engine.py above?

maffoo · 2020-07-07T16:56:57Z

cirq/google/engine/engine.py

+            params_list: List[study.Sweepable] = None,
+            repetitions: int = 1,
+            processor_ids: Sequence[str] = (),
+            gate_set: serializable_gate_set.SerializableGateSet = None,


Should be Optional[serializable_gate_set.SerializableGateSet] if None is allowed.

Done. Fixed in a bunch of places in this file.

maffoo · 2020-07-07T16:58:16Z

cirq/google/engine/engine.py

+            TrialResults, one for each parameter sweep.
+        """
+        if not gate_set:
+            raise ValueError('Gate set must be specified.')


Move this check to create_batch_program instead (or add a similar check there so that create_batch_program works the same way).

maffoo · 2020-07-07T16:58:36Z

cirq/google/engine/engine.py

+        Returns:
+            A EngineProgram for the newly created program.
+        """
+        gate_set = gate_set or gate_sets.XMON


Should this raise an error if gate_set is not given, as does run_batch.

maffoo · 2020-07-07T16:59:20Z

cirq/google/engine/engine.py

+        if not program_id:
+            program_id = _make_random_id('prog-')
+
+        code = qtypes.any_pb2.Any()


nit: move this down below after batch is populated and ready to pack.

It might be nice to create a helper function that packs a proto message into an any and can be called in a single expression. Then we could do, for example:

self.context.client.create_program(..., code=pack_any(batch), ...)

We could also consider changing client.create_program to take google.protobuf.Message for code and have it do the packing internally.

Implemented the first suggestion.

I hesitate to mess with the client too much, since it is partially generated by the gapic tools, so, if we regenerate the library, we don't want to mess with any customizations to the greatest extent possible.

…115/Cirq-1 into add_batch_support_engine

dabacon

Yeah!

dabacon · 2020-07-08T17:53:31Z

cirq/google/engine/engine.py

+            job_description: Optional[str] = None,
+            job_labels: Optional[Dict[str, str]] = None,
+    ) -> engine_job.EngineJob:
+        """Runs the supplied Circuit via Quantum Engine.Creates


nit: Circuit -> Circuits

* Add batch support to cirq Engine client - Adds a run_batch() call to engine that can handle lists of circuits and lists of programs. - Creates BatchProgram and BatchRunContext objects. - Pipes the objects through engine program and engine job.

Add batch support to cirq Engine client

c544538

- Adds a run_batch() call to engine that can handle lists of circuits and lists of programs. - Creates BatchProgram and BatchRunContext objects. - Pipes the objects through engine program and engine job.

googlebot added the cla: yes Makes googlebot stop complaining. label Jun 29, 2020

Merge branch 'master' into add_batch_support_engine

f95b666

dstrain115 requested review from wcourtney and maffoo June 29, 2020 15:14

maffoo reviewed Jun 29, 2020

View reviewed changes

dabacon reviewed Jun 29, 2020

View reviewed changes

dstrain115 added 2 commits June 30, 2020 06:40

Addressed review comments

cdf73ab

Merge branch 'add_batch_support_engine' of https://github.com/dstrain…

b281137

…115/Cirq-1 into add_batch_support_engine

Add batch_mode to engine_program

3dbe057

Add docstring for new parameter

f3cce22

dstrain115 requested review from maffoo and dabacon July 1, 2020 21:33

Merge branch 'master' into add_batch_support_engine

333d0db

maffoo approved these changes Jul 7, 2020

View reviewed changes

dstrain115 added 3 commits July 8, 2020 10:37

Address comments and clean up typing.

ab4680b

Merge branch 'master' into add_batch_support_engine

a5d4b2b

Merge branch 'add_batch_support_engine' of https://github.com/dstrain…

4dd4b05

…115/Cirq-1 into add_batch_support_engine

dabacon approved these changes Jul 8, 2020

View reviewed changes

Fix docstring

c10e0f2

dstrain115 merged commit b21294b into quantumlib:master Jul 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add batch support to cirq Engine client #3114

Add batch support to cirq Engine client #3114

dstrain115 commented Jun 29, 2020

maffoo Jun 29, 2020

dstrain115 Jun 30, 2020

maffoo Jun 29, 2020

dstrain115 Jun 30, 2020

maffoo Jun 29, 2020

dstrain115 Jun 30, 2020

maffoo Jun 29, 2020

dstrain115 Jun 30, 2020

dabacon left a comment

dabacon Jun 29, 2020

dstrain115 Jun 30, 2020

dabacon Jun 29, 2020

dstrain115 Jun 30, 2020

dstrain115 commented Jun 30, 2020

dstrain115 commented Jul 1, 2020

dstrain115 commented Jul 4, 2020

maffoo left a comment

maffoo Jul 7, 2020

dstrain115 Jul 8, 2020

maffoo Jul 7, 2020

dstrain115 Jul 8, 2020

maffoo Jul 7, 2020

dstrain115 Jul 8, 2020

maffoo Jul 7, 2020

dstrain115 Jul 8, 2020

maffoo Jul 7, 2020

dstrain115 Jul 8, 2020

maffoo Jul 7, 2020

dstrain115 Jul 8, 2020

dabacon left a comment

dabacon Jul 8, 2020

Add batch support to cirq Engine client #3114

Add batch support to cirq Engine client #3114

Conversation

dstrain115 commented Jun 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dabacon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstrain115 commented Jun 30, 2020

dstrain115 commented Jul 1, 2020

dstrain115 commented Jul 4, 2020

maffoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dabacon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment