Simulated Engine Implementation #4638

dstrain115 · 2021-11-08T21:26:47Z

Add AbstractEngine Interface

This PR adds three groups of classes to cirq_google.

The first four classes define abstract base classes (ABC)
for the four main types of the Engine:

AbstractJob, AbstractProgram, AbstractProcessor, AbstractEngine

These classes closely model the currect EngineJob, EngineProgram,
EngineProcessor, and Engine classes with the methods turned into
abstract models.

The second group is a partially implemented sub-classes that define
in-memory objects for functionality unlikely to be implemented
differently, such as reservations, labels, and descriptions:

AbstractLocalJob, AbstractLocalProgram,
AbstractLocalProcessor, AbstractLocalEngine

Lastly, the final group of classes defines implementation for
an Engine emulator backed by a sampler, such as the cirq simulator
or other local simulator.

SimulatedLocalJob, SimulatedLocalProgram,
SimulatedLocalProcessor, SimulatedLocalEngine

- Wrapper around sampler to do device related validation. - This will be used in AbstractEngine in order to centralize and simplify validation.

This PR adds three groups of classes to cirq_google. The first four classes define abstract base classes (ABC) for the four main types of the Engine: AbstractJob, AbstractProgram, AbstractProcessor, AbstractEngine These classes closely model the currect EngineJob, EngineProgram, EngineProcessor, and Engine classes with the methods turned into abstract models. The second group is a partially implemented sub-classes that define in-memory objects for functionality unlikely to be implemented differently, such as reservations, labels, and descriptions: AbstractLocalJob, AbstractLocalProgram, AbstractLocalProcessor, AbstractLocalEngine Lastly, the final group of classes defines implementation for an Engine emulator backed by a sampler, such as the cirq simulator or other local simulator. SimulatedLocalJob, SimulatedLocalProgram, SimulatedLocalProcessor, SimulatedLocalEngine

MichaelBroughton

Can we break this up into smaller pieces ?

dstrain115 · 2021-11-09T13:44:43Z

I merged with master, which brought file count down from 25 to 20.

I have split this into 3 PRs:

Abstract interface only, no functionality (4 files) Add Abstract Engine Interface #4644
Partial local implementation (8 additional files) Add Local implementation of abstract engine classes #4645
Full implementation using simulated sampler (8 additional files) Simulated Engine Implementation #4638 (This PR)

Once the first two PRs are merged, this will be down to 8 files.

… into engine_interface

dstrain115 · 2021-11-20T14:28:39Z

Ok, the first two PRs are in and merged. This PR is ready to review with only 8 files now!

95-martin-orion

First pass - I still need to do a deeper dive on simulated_local_processor[_test].py.

95-martin-orion · 2021-11-23T21:50:06Z

cirq-google/cirq_google/engine/simulated_local_engine_test.py

+from cirq_google.engine.abstract_program import AbstractProgram
+from cirq_google.engine.simulated_local_engine import SimulatedLocalEngine
+from cirq_google.engine.simulated_local_processor import SimulatedLocalProcessor
+


I'm surprised there's so much in this test. Can we limit if to behavior above and beyond what's covered in the parent class tests?

I am loathe to remove these tests. Even if they are redundant, it is nice to test that all the cross-references work with the final child class.

95-martin-orion · 2021-11-23T21:57:19Z

cirq-google/cirq_google/engine/simulated_local_job.py

+
+        This function will fail if any other type of results were returned.
+        """
+        raise NotImplemented  # coverage: ignore


Recommend adding a simple test to assert that this is not implemented.

95-martin-orion · 2021-11-23T22:07:31Z

cirq-google/cirq_google/engine/simulated_local_processor.py

+    API or for testing or mocking.
+
+    Attributes:
+        sammpler: A `cirq.Sampler` that can execute the quantum jobs.


sammpler -> sampler

95-martin-orion · 2021-11-23T22:07:49Z

cirq-google/cirq_google/engine/simulated_local_processor.py

+    """A processor backed by a sampler and device.
+
+    Intended for local simulation testing, this processor will
+    create a VAlidationSampler that will validate requests based on


VAlidation -> Validation

95-martin-orion · 2021-11-23T22:23:02Z

cirq-google/cirq_google/engine/simulated_local_processor.py

+        latest = None
+        current_calibration = None
+        for calibration_seconds in self._calibrations:
+            if calibration_seconds <= timestamp and (
+                latest is None or latest < calibration_seconds
+            ):
+                latest = calibration_seconds
+                current_calibration = self._calibrations[latest]
+        return current_calibration


[Optional] There's probably a more concise/pythonic way of doing this with max(..., key=...). Leaving it up to you whether to switch to that form.

Good idea. I didn't realize you could use a key with max. Good trick.

95-martin-orion · 2021-11-23T22:25:31Z

cirq-google/cirq_google/engine/simulated_local_processor.py

+        return self._device
+
+    def get_device_specification(self) -> Optional[v2.device_pb2.DeviceSpecification]:
+        raise NotImplemented  # coverage: ignore


Add negative test for this method.

95-martin-orion · 2021-11-23T22:29:03Z

cirq-google/cirq_google/engine/simulated_local_processor.py

+        return job
+
+    def run_calibration(self, *args, **kwargs):
+        raise NotImplemented  # coverage: ignore


Add negative test

95-martin-orion · 2021-11-23T22:32:17Z

cirq-google/cirq_google/engine/simulated_local_program.py

+    from cirq_google.engine.simulated_local_processor import SimulatedLocalProcessor
+
+
+class SimulatedLocalProgram(AbstractLocalProgram):


Tests for this class? Kind of surprised coverage check isn't firing for this...

Added some basic tests. Coverage was previously provided by tests in processor and engine.

Where are these tests? There's no simulated_local_program_test.py in this PR - did it get missed in the commit?

95-martin-orion · 2021-11-23T22:37:27Z

cirq-google/cirq_google/engine/simulated_local_program.py

+    This class functions as a parent class for a `SimulatedLocalJob`
+    object.
+
+    This is an abstract class that inheritors should implement.


Remove this line? This doesn't look like an abstract class, at least.

MichaelBroughton

Just a few comments from me and then I think we should be good to merge.

MichaelBroughton · 2021-11-24T17:23:29Z

cirq-google/cirq_google/engine/simulated_local_job.py

+    def __init__(
+        self,
+        *args,
+        sampler: cirq.Sampler = cirq.Simulator(),


Nit: unsafe default.

MichaelBroughton · 2021-11-24T17:32:40Z

cirq-google/cirq_google/engine/simulated_local_job.py

+                        program=program, params=sweeps[0], repetitions=reps
+                    )
+                else:
+                    return [self._sampler.run(program=program, repetitions=reps)]


Does the default on run_sweep here not have the same return signature as the one above ?

I am not sure I understand the question. This line uses run() which returns a single Result. The line above uses run_sweep which returns a List[Result].

Sorry, let me ask instead: Why couldn't we do something like:

self._state = quantum.enums.thing return self._sampler.run_sweep(program=program, sweeps[0] if sweeps else None, repetitions=reps)

and get rid of the need for the if/else block ?

MichaelBroughton · 2021-11-24T17:34:55Z

cirq-google/cirq_google/engine/simulated_local_processor.py

+        for calibration_seconds in self._calibrations:
+            if (
+                earliest_timestamp_seconds is not None
+                and earliest_timestamp_seconds > calibration_seconds
+            ):
+                continue
+            if (
+                latest_timestamp_seconds is not None
+                and latest_timestamp_seconds < calibration_seconds
+            ):
+                continue
+            calibration_list.append(self._calibrations[calibration_seconds])


Nit: can this be tightened up using list comprehensions instead of the these guarding continues ?

MichaelBroughton · 2021-11-24T18:03:28Z

cirq-google/cirq_google/engine/simulated_local_processor.py

+        for program in self._programs.values():
+            if created_before is not None and created_before < program.create_time():
+                continue
+            if created_after is not None and created_after > program.create_time():
+                continue
+            if has_labels is not None:
+                labels = program.labels()
+                if any(key not in labels or labels[key] != has_labels[key] for key in has_labels):
+                    continue
+            programs.append(program)


Nit: can probably just use list comprehension here too.

95-martin-orion

Finished simulated_local_processor_test.py review, just a few additional items.

95-martin-orion · 2021-11-24T18:57:24Z

cirq-google/cirq_google/engine/simulated_local_processor_test.py

+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+"""A helper for jobs that have been created on the Quantum Engine."""


Update file docstring? Same for simulated_local_job[_test].py.

95-martin-orion · 2021-11-24T19:05:37Z

cirq-google/cirq_google/engine/simulated_local_processor.py

+
+VALID_LANGUAGES = [
+    'type.googleapis.com/cirq.google.api.v2.Program',
+    'type.googleapis.com/cirq.google.api.v2.FocusedCalibration',


Since run_calibration isn't implemented, should we remove FocusedCalibration from this list?

Good idea. Removed.

95-martin-orion · 2021-11-24T19:09:34Z

cirq-google/cirq_google/engine/simulated_local_processor_test.py

+    assert np.all(results[1].measurements['m'] == 0)
+    assert job.execution_status() == quantum.enums.ExecutionStatus.State.SUCCESS
+
+    # iteration


Missing test block? Otherwise remove comment.

Removed, was tested above.

MichaelBroughton

LGTM, left two final comments, feel free to ignore if you want.

MichaelBroughton · 2021-11-29T18:51:05Z

cirq-google/cirq_google/engine/simulated_local_job.py

+                        program=program, params=sweeps[0], repetitions=reps
+                    )
+                else:
+                    return [self._sampler.run(program=program, repetitions=reps)]


Sorry, let me ask instead: Why couldn't we do something like:

self._state = quantum.enums.thing return self._sampler.run_sweep(program=program, sweeps[0] if sweeps else None, repetitions=reps)

and get rid of the need for the if/else block ?

MichaelBroughton · 2021-11-29T18:52:17Z

cirq-google/cirq_google/engine/simulated_local_processor.py

+        return list(
+            filter(
+                lambda program: after_limit < program.create_time() < before_limit
+                and all(
+                    (key in program.labels() and program.labels()[key] == labels[key])
+                    for key in labels
+                ),
+                self._programs.values(),
+            )
+        )


Perhaps storing an intermediate list or two here to break up the big compound expression might be a little easier to parse as a reader ?

Made an intermediate function instead.

95-martin-orion

LGTM once simulated_local_program_test comment is resolved.

This PR adds a final group of classes to abstract engine to define implementation for an Engine emulator backed by a sampler, such as the cirq simulatoror other local simulator. SimulatedLocalJob, SimulatedLocalProgram, SimulatedLocalProcessor, SimulatedLocalEngine This will allow a user to create relevant Engine objects backed by a sampler and validator of their choice (such as a simulator based on a device).

dstrain115 added 4 commits November 1, 2021 02:02

Validating Sampler

3a9fe5f

- Wrapper around sampler to do device related validation. - This will be used in AbstractEngine in order to centralize and simplify validation.

Add test for coverage

967c1d1

Address review comments

4b3eef8

dstrain115 requested review from cduck, vtomole, wcourtney and a team as code owners November 8, 2021 21:26

dstrain115 requested a review from viathor November 8, 2021 21:26

google-cla bot added the cla: yes Makes googlebot stop complaining. label Nov 8, 2021

CirqBot added the size: XL lines changed >1000 label Nov 8, 2021

dstrain115 requested a review from mpharrigan November 8, 2021 21:37

dstrain115 added 2 commits November 8, 2021 13:53

Add more zeros to make windows happy.

5c3ef59

Try again. Windows really does not like small timestamps.

2fb3b38

MichaelBroughton requested changes Nov 9, 2021

View reviewed changes

MichaelBroughton self-assigned this Nov 9, 2021

Merge branch 'master' into engine_interface

9448eba

dstrain115 changed the title ~~Engine interface~~ Entire engine interface (draft) Nov 9, 2021

dstrain115 removed request for mpharrigan and viathor November 9, 2021 13:11

dstrain115 changed the title ~~Entire engine interface (draft)~~ Simulated Engine Implementation Nov 9, 2021

dstrain115 added 8 commits November 15, 2021 06:05

Address review comments on abstract interface PR.

fc9e7f6

Resolve Dave comments

d7770db

Filter docstring cleanup.

0ae56bf

Address review comments and fix coverage

93ce2fb

Piece of missing documentation

2b3928a

Update simulated local abstract to use new function types.

9b32368

Merge branch 'engine_interface' of https://github.com/dstrain115/Cirq-1…

013d90c

… into engine_interface

Fix typo

e1cecc6

dstrain115 added 2 commits November 18, 2021 05:32

Merge branch 'master' into engine_interface

c4526ba

Merge branch 'master' into engine_interface

f35c7c1

dstrain115 requested a review from MichaelBroughton November 20, 2021 14:28

dstrain115 requested a review from 95-martin-orion November 23, 2021 18:51

95-martin-orion requested changes Nov 23, 2021

View reviewed changes

MichaelBroughton reviewed Nov 24, 2021

View reviewed changes

95-martin-orion requested changes Nov 24, 2021

View reviewed changes

Address review comments

e64d018

dstrain115 requested review from 95-martin-orion and MichaelBroughton November 29, 2021 18:37

MichaelBroughton approved these changes Nov 29, 2021

View reviewed changes

95-martin-orion approved these changes Nov 29, 2021

View reviewed changes

dstrain115 added 3 commits November 29, 2021 11:34

Get a more reasonable default end date for Windows

c517177

Add tests and address review comments.

f46627d

Merge branch 'master' into engine_interface

23cb91a

dstrain115 merged commit 973b2c7 into quantumlib:master Nov 29, 2021

		from cirq_google.engine.simulated_local_processor import SimulatedLocalProcessor


		class SimulatedLocalProgram(AbstractLocalProgram):

Simulated Engine Implementation #4638

Simulated Engine Implementation #4638

Conversation

dstrain115 commented Nov 8, 2021

MichaelBroughton left a comment • edited Loading

Choose a reason for hiding this comment

dstrain115 commented Nov 9, 2021

dstrain115 commented Nov 20, 2021

95-martin-orion 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

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

MichaelBroughton 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

95-martin-orion 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

MichaelBroughton 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

95-martin-orion left a comment

Choose a reason for hiding this comment

MichaelBroughton left a comment •

edited

Loading