Add `cg.ProcessorSampler` #5361

mpharrigan · 2022-05-13T01:24:54Z

We aim to have interfaces and classical/simulated implementations of all of the cg.engine. classes.

Wheras QuantumEngineSampler turns a cg.Engine into a Sampler, ProcessorSampler turns a AbstractProcessor into a Sampler. Unfortunately, QuantumEngineSampler does not work with an AbstractEngine; hence the new class.

Engine, Engine.get_sampler(), and QuantumEngineSampler can accept a list of processor_ids, so they are preserved for backwards compatibility. QuantumEngineSampler and this behavior should be considered for deprecation.

This PR unifies the implementations of AbstractProcessor.get_sampler() to always just return ProcessorSampler(processor=self).

fixes #5360

Backwards incompatible changes:

the return type of AbstractProcessor.get_sampler() has changed. The behavior of the resulting object has not changed for EngineProcessors (unless you're doing something bad, see below), but has changed for SimulatedLocalProcessors. This fixes the issues like SimulatedLocalProcessor .run() vs .get_sampler().run() #5360
ProcessorSampler doesn't handle program argument types of EngineProgram. We'd need to teach AbstractProgram some run methods. cc QuantumEngineSampler doesn't allow any control over program id or job id. #1958 for context
Previously, you could have done something weird where you use the sampler from EngineProcessor.get_sampler() to run on processors other than the one pointed to by EngineProcessor. This is no longer possible.

Backwards compatible parts:

QuantumEngineSampler still exists and is returned by Engine.get_sampler or cg.get_engine_sampler()

@dstrain115 @verult

dstrain115

I am good with these changes modulo some double-checking and fixing tests. Thanks for cleaning this interface up!

dstrain115 · 2022-05-13T20:30:57Z

cirq-google/cirq_google/engine/engine_processor.py

@@ -105,7 +106,7 @@ def engine(self) -> 'engine_base.Engine':
    @util.deprecated_gate_set_parameter
    def get_sampler(
        self, gate_set: Optional[serializer.Serializer] = None
-    ) -> engine_sampler.QuantumEngineSampler:
+    ) -> 'cg.engine.QuantumProcessorSampler':


@wcourtney Will changing this cause any backwards incompatibility problems? This affects the QCS engine in addition to the simulated version.

I think it should be fine, but want to double-check.

This is a backwards incompatible change for anyone using processor.get_sampler() and trying to use multiple processor_ids. However, this workflow doesn't make any sense and I don't think many people are using processor.get_sampler().

Most people using cg.get_engine().get_sampler() or cg.get_engine_sampler() will be unaffected

I updated the PR description

dstrain115 · 2022-05-13T20:33:38Z

cirq-google/cirq_google/engine/processor_sampler.py

+# distributed under the License is distributed on an "AS IS" BASIS,
+# 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.


Optional: should EngineSampler use this class underneath?

I don't think it (easily) can.

Engine.run_sweep and Processor.run_sweep have meaningfully different semantics: The first will send out a job to one of potentially many processors. This logic happens server-side. While we could theoretically replicate it using a collection of Processors / ProcessorSamplers underneath EngineSampler, it wouldn't be a small change.

I am starting to agree with maffoo. It seems like ProcessorSampler is basically the same as a QuantumEngineSampler with a processor set already. Seems like these two can/should be combined.

After off-line conversations, I am fully on-board with this change.

I think we should deprecate QuantumEngineSampler and switch engine.get_sampler(processor_id) to use engine.get_processor(processor_id).get_sampler() (or, alternatively, make QuantumEngineSampler a thin wrapper around this ProcessorSampler functionality). Since this functionality currently supports multiple processor ids and sending an EngineProgram, it probably needs more thought and should be done in a follow-up PR.

Short story, I approve this PR as is.

mpharrigan · 2022-05-13T20:59:54Z

Thanks for the preliminary check. I'll flesh this out

mpharrigan · 2022-05-13T22:23:57Z

@dstrain115 @wcourtney PTAL now that this is complete

maffoo

The relationship between QuantumEngineSampler and QuantumProcessorSampler is confusing to me. I'd suggest that we only have one of these and that any custom code that currently lives in QuantumEngineSampler methods be moved into EngineProcessor methods that the sampler calls. As for names, I think call this something like ProcessorSampler would probably be best, since it implements the Sampler interface given an AbstractProcessor. We could use that name and deprecate the existing QuantumEngineSampler name.

maffoo · 2022-05-16T17:09:31Z

cirq-google/cirq_google/engine/processor_sampler.py

+        )
+
+    @property
+    def processor(self):


nit: add return type.

mpharrigan · 2022-05-16T18:16:25Z

any custom code that currently lives in QuantumEngineSampler methods be moved into EngineProcessor methods that the sampler calls.

We can't do this because QuantumEngineSampler can dispatch to multiple processors.

mpharrigan · 2022-05-16T18:21:47Z

The other glue code relates to repetitions: cirq.Sampler accepts a list of values but AbstractProcessor accepts only one. Lots of discussion about this here: #3265

maffoo · 2022-05-16T18:40:16Z

We can't do this because QuantumEngineSampler can dispatch to multiple processors.

Do we ever use this? To me it makes more sense to just create separate sampler instances for each processor you want to talk to (just like you'd create separate AbstractProcessor instances for each of them). I know we thought early-on that it might make sense to allow sending a program and letting the service decide which of a set of processors to dispatch to, but given NISQ realities and how much device-specific calibration is needed to make this run well this seems unlikely to be useful.

The other glue code relates to repetitions: cirq.Sampler accepts a list of values but AbstractProcessor accepts only one.

True, but the code to handle this is almost identical in both QuantumEngineSampler and QuantumProcessorSampler, so this doesn't seem like a good argument for having separate classes.

mpharrigan · 2022-05-16T18:47:50Z

I agree that QuantumEngineSampler has annoying semantics and should be deprecated. This would have to be done with some care.

Is your only remaining concern the duplicate code that exists in QuantumEngineSampler?

One cannot refactor QuantumEngineSampler to use ProcessorSampler because of the multi-processor_id and engineprogram support.

maffoo

Consider renaming ProcessorSampler to EngineSampler (it is a cirq.Sampler that returns EngineResult). As discussed, we plan to deprecate and remove QuantumEngineSampler, but that can be a separate PR.

mpharrigan · 2022-05-16T22:20:51Z

I think I'm going to keep it as ProcessorSampler to highlight the link to AbstractProcessor (which happens to return EngineResults). I suspect most folks will not notice or care that it returns EngineResults, which are a subclass of cirq.Result and the cirq.Sampler interface only guarantees the methods and properties on cirq.Result.

mpharrigan · 2022-05-16T22:27:39Z

opened #5371 for follow-on work

dstrain115 · 2022-05-16T22:27:59Z

cirq-google/cirq_google/engine/processor_sampler.py

+# distributed under the License is distributed on an "AS IS" BASIS,
+# 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.


After off-line conversations, I am fully on-board with this change.

I think we should deprecate QuantumEngineSampler and switch engine.get_sampler(processor_id) to use engine.get_processor(processor_id).get_sampler() (or, alternatively, make QuantumEngineSampler a thin wrapper around this ProcessorSampler functionality). Since this functionality currently supports multiple processor ids and sending an EngineProgram, it probably needs more thought and should be done in a follow-up PR.

Short story, I approve this PR as is.

@dstrain115

We aim to have interfaces and classical/simulated implementations of all of the `cg.engine.` classes. Wheras `QuantumEngineSampler` turns a `cg.Engine` into a `Sampler`, `ProcessorSampler` turns a `AbstractProcessor` into a `Sampler`. Unfortunately, `QuantumEngineSampler` does **not** work with an `AbstractEngine`; hence the new class. `Engine`, `Engine.get_sampler()`, and `QuantumEngineSampler` can accept a _list_ of processor_ids, so they are preserved for backwards compatibility. `QuantumEngineSampler` and this behavior should be considered for deprecation. This PR unifies the implementations of `AbstractProcessor.get_sampler()` to always just return `ProcessorSampler(processor=self)`. fixes quantumlib#5360 Backwards incompatible changes: - the return type of `AbstractProcessor.get_sampler()` has changed. The behavior of the resulting object has not changed for EngineProcessors (unless you're doing something bad, see below), but has changed for SimulatedLocalProcessors. This fixes the issues like quantumlib#5360 - ProcessorSampler doesn't handle `program` argument types of `EngineProgram`. We'd need to teach `AbstractProgram` some run methods. cc quantumlib#1958 for context - Previously, you could have done something weird where you use the sampler from EngineProcessor.get_sampler() to run on processors other than the one pointed to by EngineProcessor. This is no longer possible. Backwards compatible parts: - QuantumEngineSampler still exists and is returned by Engine.get_sampler or cg.get_engine_sampler() @dstrain115 @verult

QuantumProcessorSampler

c3437bc

mpharrigan requested review from wcourtney, a team, vtomole, cduck and verult as code owners May 13, 2022 01:24

mpharrigan requested a review from viathor May 13, 2022 01:24

CirqBot added the size: M 50< lines changed <250 label May 13, 2022

always return it from abstractproc

94e8c48

dstrain115 approved these changes May 13, 2022

View reviewed changes

mpharrigan added 3 commits May 13, 2022 14:21

fixy fixy

fde41eb

fixy fixy

0680011

testy testy

848ac63

mpharrigan mentioned this pull request May 13, 2022

SimulatedLocalProcessor .run() vs .get_sampler().run() #5360

Closed

maffoo reviewed May 16, 2022

View reviewed changes

mpharrigan added 3 commits May 16, 2022 11:23

type annotate

ef0943a

rename to ProcessorSampler

442a099

Merge remote-tracking branch 'origin/master' into 2022-05-procsamp

cd9d846

mpharrigan changed the title ~~QuantumProcessorSampler~~ ProcessorSampler May 16, 2022

mpharrigan changed the title ~~ProcessorSampler~~ Add cg.ProcessorSampler May 16, 2022

maffoo approved these changes May 16, 2022

View reviewed changes

mpharrigan mentioned this pull request May 16, 2022

Deprecate QuantumEngineSampler #5371

Closed

dstrain115 approved these changes May 16, 2022

View reviewed changes

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

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

Merge branch 'master' into 2022-05-procsamp

8c5ea28

CirqBot merged commit 0a16517 into quantumlib:master May 16, 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 May 16, 2022

mpharrigan mentioned this pull request Jun 3, 2022

Deprecate QuantumEngineSampler #5432

Merged

mpharrigan mentioned this pull request Jun 13, 2022

Add async support in EngineClient, EngineSampler, etc. #5219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `cg.ProcessorSampler` #5361

Add `cg.ProcessorSampler` #5361

mpharrigan commented May 13, 2022 •

edited

Loading

dstrain115 left a comment

dstrain115 May 13, 2022

mpharrigan May 13, 2022

mpharrigan May 13, 2022

dstrain115 May 13, 2022

mpharrigan May 13, 2022

dstrain115 May 16, 2022

dstrain115 May 16, 2022

mpharrigan commented May 13, 2022

mpharrigan commented May 13, 2022

maffoo left a comment

maffoo May 16, 2022

mpharrigan May 16, 2022

mpharrigan commented May 16, 2022

mpharrigan commented May 16, 2022

maffoo commented May 16, 2022

mpharrigan commented May 16, 2022

maffoo left a comment

mpharrigan commented May 16, 2022

mpharrigan commented May 16, 2022

dstrain115 May 16, 2022

Add cg.ProcessorSampler #5361

Add cg.ProcessorSampler #5361

Conversation

mpharrigan commented May 13, 2022 • edited Loading

dstrain115 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

mpharrigan commented May 13, 2022

mpharrigan commented May 13, 2022

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

mpharrigan commented May 16, 2022

mpharrigan commented May 16, 2022

maffoo commented May 16, 2022

mpharrigan commented May 16, 2022

maffoo left a comment

Choose a reason for hiding this comment

mpharrigan commented May 16, 2022

mpharrigan commented May 16, 2022

Choose a reason for hiding this comment

Add `cg.ProcessorSampler` #5361

Add `cg.ProcessorSampler` #5361

mpharrigan commented May 13, 2022 •

edited

Loading