Replace AbstractEngineProcessorShim #4842

mpharrigan · 2022-01-14T19:37:56Z

We were using AbstractEngineProcessorShim pending #4644 and friends. Those PRs have landed. For data hygiene as part of the workflow tools, we need to record what processor was used. This PR introduces an abstract ProcessorRecord interface and three implementations for mapping (processor_id, whether it's a simulator, what sort of noise) tuples to an AbstractProcessor instance for doing the work.

Remove AbstractEngineProcessorShim
Add dataclasses for data hygene
Now we can have proper serialization tests for anything that saves a QuantumRuntimeConfiguration, hence the new json files
We don't need to shim over cirq.read_json in tests that make use of AbstractEngineProcessorShim any more
Side effect: the new engine-processor-backed instances mean we have to use "real" qubits in the tests, so there are some knock-on test changes.

cc @dstrain115 @MichaelBroughton

mpharrigan · 2022-01-14T19:39:11Z

I don't think we can use SimulatedLocalProcessor directly because it isn't serializable -- either philosophically or in practice.

mpharrigan · 2022-01-19T00:07:57Z

related: #2820

mpharrigan · 2022-01-27T00:01:00Z

@MichaelBroughton @dstrain115 this should be ready for your review, PTAL

dstrain115 · 2022-01-28T02:25:03Z

cirq-google/cirq_google/workflow/processor_record.py

+    def get_processor(self) -> 'cg.EngineProcessor':
+        """Return a `cg.EngineProcessor` for the specified processor_id."""
+        engine = cg.get_engine()
+        return cg.EngineProcessor(


Why not just engine.get_processor(self.processor_id)?

ah. (1) I didn't think to do that; (2) I thought I was going to have to put in more environment variable magic into this function that turned out not to be necessary.

changed as suggested.

dstrain115 · 2022-01-28T02:26:21Z

cirq-google/cirq_google/workflow/processor_record.py

+        This implementation hardcodes the `cg.SQRT_ISWAP_GATESET` to construct
+        the sampler until this argument is made optional.
+        """
+        return self.get_processor().get_sampler(cg.SQRT_ISWAP_GATESET)


Should the default be the CircuitSerializer? It's more general and can handle the union of the other gatesets (at the cost of being less tested, but please use this to help test it!)

oh wow. when was that merged in? will test it out

dstrain115 · 2022-01-28T02:30:44Z

cirq-google/cirq_google/workflow/processor_record.py

+        Only 'rainbow' and 'weber' are recognized processor_ids and the device information
+        may not be up-to-date, as it is completely local.
+        """
+        return _DEVICES_BY_ID[self.processor_id]


Do you want to use the devices in devices/specifications?

For instance, there is this call which is very similar:
create_noiseless_virtual_engine_from_latest_templates

yes I do.

I'm going to try to see what I have to add to that module so I can request the latest template by name instead of getting them in a list.

I think I'd need something like #4909 to re-use as much as possible

merged in #4909 to use virtual_engine_factory stuff to make these local devices

dstrain115 · 2022-01-28T02:40:54Z

Also, did #4850 solve your optional gate set issues? Or is there further work needed?

verult · 2022-02-09T20:23:53Z

cirq-google/cirq_google/workflow/processor_record.py

+from cirq._compat import dataclass_repr
+from cirq_google.engine.virtual_engine_factory import (
+    MOST_RECENT_TEMPLATES,
+    _create_device_spec_from_template,


Should this function not have a _ prefix if it's imported here?

hmm I think since it's all inside cirq_google it's fine.

Do you think it should be a public method? Easier to make public later than vice versa

My impression is that _ implies it's private within a module, but I'm not a Python guru so not a strong preference :) According to The naming guide from "PEP 8 -- Style Guide for Python Code":

"_single_leading_underscore: weak "internal use" indicator. E.g. from M import * does not import objects whose names start with an underscore."

This seems to imply that objects used outside a module shouldn't have an underscore.

that still depends on your interpretation of the meaning of "internal", right? module internal or package internal. As an example, we have cirq._compat that's used throughout cirq but external users should not import.

In any event, if folks (i.e. you) think this is a valuable method for users to use, we can make it "public"

OK sounds good, no strong preference from me on whether to have _. As for whether we should make it available for users to use, I'd say not until there's a specific need. So keeping the underscore sgtm.

verult · 2022-02-09T20:32:59Z

cirq-google/cirq_google/workflow/processor_record.py

+        engine = cg.get_engine()
+        return engine.get_processor(self.processor_id)
+
+    def get_sampler(self) -> 'cg.QuantumEngineSampler':


Does this need to be reimplemented if the abstract class logic is the same?

Is the more specific cg.QuantumEngineSampler type used anywhere over the cirq.Sampler?

good catch. this was vestigial from when I had to hack in a gateset

verult · 2022-02-09T20:33:05Z

cirq-google/cirq_google/workflow/processor_record.py

+        """Return a `cg.QuantumEngineSampler` for the specified processor_id."""
+        return self.get_processor().get_sampler()
+
+    def get_device(self) -> 'cirq.Device':


- Factor out `create_noiseless_virtual_processor_from_proto` for use in `create_noiseless_virtual_engine_from_proto` - Factor out `_create_device_spec_from_template` for use in `create_noiseless_virtual_processor_from_template` and `create_noiseless_virtual_engine_from_templates` - Factor out `MOST_RECENT_TEMPLATES` for use in `create_noiseless_virtual_engine_from_latest_templates` and (manually) in `create_noiseless_virtual_processor_from_template` This unblocks creating one processor at a time for use in #4842

mpharrigan · 2022-02-10T01:55:18Z

@verult @dstrain115 ptal. With #4909 merged in, the diff should be fixed (smaller)

verult

Did a surface-level review and lgtm. Please feel free to ignore nits.

verult · 2022-02-10T19:35:55Z

cirq-google/cirq_google/workflow/processor_record.py

+    def _json_dict_(self):
+        return cirq.dataclass_json_dict(self)
+
+    def descriptive_name(self) -> str:


nit: Should this be __str__()?

verult · 2022-02-10T21:42:56Z

cirq-google/cirq_google/workflow/processor_record_test.py

+        },
+        clear=True,
+    ):
+        proc_rec = cg.EngineProcessorRecord('rainbow')


nit: declare processor ID as a constant at top of file. Makes it easier to change when we remove device templates in the future.

gonna definitely keep it 1) to keep the tests descriptive and readable 2) to make sure we don't remove device templates in the future!

verult

Approving per discussion in chat

- Factor out `create_noiseless_virtual_processor_from_proto` for use in `create_noiseless_virtual_engine_from_proto` - Factor out `_create_device_spec_from_template` for use in `create_noiseless_virtual_processor_from_template` and `create_noiseless_virtual_engine_from_templates` - Factor out `MOST_RECENT_TEMPLATES` for use in `create_noiseless_virtual_engine_from_latest_templates` and (manually) in `create_noiseless_virtual_processor_from_template` This unblocks creating one processor at a time for use in quantumlib#4842

@dstrain115

We were using `AbstractEngineProcessorShim` pending quantumlib#4644 and friends. Those PRs have landed. For data hygiene as part of the workflow tools, we need to record what processor was used. This PR introduces an abstract `ProcessorRecord` interface and three implementations for mapping (processor_id, whether it's a simulator, what sort of noise) tuples to an AbstractProcessor instance for doing the work. - Remove `AbstractEngineProcessorShim` - Add dataclasses for data hygene - Now we can have proper serialization tests for anything that saves a `QuantumRuntimeConfiguration`, hence the new json files - We don't need to shim over `cirq.read_json` in tests that make use of `AbstractEngineProcessorShim` any more - Side effect: the new engine-processor-backed instances mean we have to use "real" qubits in the tests, so there are some knock-on test changes. cc @dstrain115 @MichaelBroughton

- Factor out `create_noiseless_virtual_processor_from_proto` for use in `create_noiseless_virtual_engine_from_proto` - Factor out `_create_device_spec_from_template` for use in `create_noiseless_virtual_processor_from_template` and `create_noiseless_virtual_engine_from_templates` - Factor out `MOST_RECENT_TEMPLATES` for use in `create_noiseless_virtual_engine_from_latest_templates` and (manually) in `create_noiseless_virtual_processor_from_template` This unblocks creating one processor at a time for use in quantumlib#4842

@dstrain115

We were using `AbstractEngineProcessorShim` pending quantumlib#4644 and friends. Those PRs have landed. For data hygiene as part of the workflow tools, we need to record what processor was used. This PR introduces an abstract `ProcessorRecord` interface and three implementations for mapping (processor_id, whether it's a simulator, what sort of noise) tuples to an AbstractProcessor instance for doing the work. - Remove `AbstractEngineProcessorShim` - Add dataclasses for data hygene - Now we can have proper serialization tests for anything that saves a `QuantumRuntimeConfiguration`, hence the new json files - We don't need to shim over `cirq.read_json` in tests that make use of `AbstractEngineProcessorShim` any more - Side effect: the new engine-processor-backed instances mean we have to use "real" qubits in the tests, so there are some knock-on test changes. cc @dstrain115 @MichaelBroughton

Remove AbstractEngineProcessorShim

3bb026d

mpharrigan requested review from cduck, vtomole, wcourtney and a team as code owners January 14, 2022 19:37

mpharrigan requested a review from viathor January 14, 2022 19:38

CirqBot added the size: M 50< lines changed <250 label Jan 14, 2022

mpharrigan added 2 commits January 18, 2022 11:38

quantum backend

6890a8a

no _MockEngineProcessor

a2fca45

mpharrigan added 10 commits January 18, 2022 16:24

tests

98a54f4

Tests

f19dd3b

"has a" relationship

ea94f30

Merge remote-tracking branch 'origin/master' into 2022-01-ae

a55e573

Merge remote-tracking branch 'origin/master' into 2022-01-ae

b6a7f42

tests

38c0748

json

3039b65

coverage

99b305c

lint

4108e7f

mypy

2c30c16

mpharrigan changed the title ~~Remove AbstractEngineProcessorShim~~ Replace AbstractEngineProcessorShim Jan 26, 2022

docstrings

36a1db9

mpharrigan mentioned this pull request Jan 27, 2022

loschmidt.tilted_square_lattice gen+run quantumlib/ReCirq#259

Merged

dstrain115 reviewed Jan 28, 2022

View reviewed changes

mpharrigan added 3 commits January 28, 2022 11:42

engine.get_processor

faa9f7c

optional gate sets!

a6a94e5

Refactor virtual_engine_factory

b42ab1c

mpharrigan added 8 commits February 1, 2022 16:10

docstring

66415ef

docstring 2

d5ada23

docstring 3

14aad0a

move template dict to the top

4bbff7d

Merge remote-tracking branch 'origin/master' into 2022-01-virtproc

19781b5

Merge branch '2022-01-virtproc' into 2022-01-ae

a6b1c81

use virtual_engine_factory devices

2ab7a74

Merge branch 'master' into 2022-01-virtproc

b9e25af

verult reviewed Feb 9, 2022

View reviewed changes

CirqBot and others added 3 commits February 9, 2022 17:21

Merge branch 'master' into 2022-01-virtproc

dbabda3

Merge branch '2022-01-virtproc' into 2022-01-ae

2303b80

just use base class's method

c8574e6

mpharrigan added 2 commits February 9, 2022 17:50

mock out credentials I hope this works

76dc311

Merge remote-tracking branch 'origin/master' into 2022-01-ae

368a7af

verult reviewed Feb 10, 2022

View reviewed changes

str

1f88111

verult approved these changes Feb 11, 2022

View reviewed changes

mpharrigan added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Feb 11, 2022

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Feb 11, 2022

Merge branch 'master' into 2022-01-ae

802923d

CirqBot merged commit 7895028 into quantumlib:master Feb 11, 2022

CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Feb 11, 2022

mpharrigan mentioned this pull request Mar 29, 2022

Name <--> Device <--> Processor ID <--> Simulator correspondence #2820

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace AbstractEngineProcessorShim #4842

Replace AbstractEngineProcessorShim #4842

mpharrigan commented Jan 14, 2022 •

edited

Loading

mpharrigan commented Jan 14, 2022

mpharrigan commented Jan 19, 2022

mpharrigan commented Jan 27, 2022

dstrain115 Jan 28, 2022

mpharrigan Jan 28, 2022

dstrain115 Jan 28, 2022

mpharrigan Jan 28, 2022

dstrain115 Jan 28, 2022

mpharrigan Jan 28, 2022

mpharrigan Jan 28, 2022

mpharrigan Feb 2, 2022

dstrain115 commented Jan 28, 2022

verult Feb 9, 2022

mpharrigan Feb 10, 2022

verult Feb 10, 2022

mpharrigan Feb 10, 2022

verult Feb 10, 2022

verult Feb 9, 2022

mpharrigan Feb 10, 2022

verult Feb 9, 2022

mpharrigan commented Feb 10, 2022

verult left a comment

verult Feb 10, 2022

mpharrigan Feb 11, 2022

verult Feb 10, 2022

mpharrigan Feb 11, 2022

verult left a comment

Replace AbstractEngineProcessorShim #4842

Replace AbstractEngineProcessorShim #4842

Conversation

mpharrigan commented Jan 14, 2022 • edited Loading

mpharrigan commented Jan 14, 2022

mpharrigan commented Jan 19, 2022

mpharrigan commented Jan 27, 2022

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

dstrain115 commented Jan 28, 2022

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

mpharrigan commented Feb 10, 2022

verult 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

verult left a comment

Choose a reason for hiding this comment

mpharrigan commented Jan 14, 2022 •

edited

Loading