get_engine_sampler #2767

mpharrigan · 2020-02-13T19:24:52Z

Remove tons of boilerplate

cirq/google/engine/engine_sampler_test.py

cirq/google/gate_sets.py

vtomole

Something about those creds are breaking the Macos check..

mpharrigan · 2020-02-19T19:09:50Z

hmm I'll have to dig into the mac ci

maffoo · 2020-02-20T23:10:50Z

cirq/google/engine/engine_sampler.py

+        raise ValueError(f"Please use one of the following gateset names: "
+                         f"{sorted(gate_sets.NAMED_GATESETS.keys())}")
+
+    return engine.Engine(project_id=os.environ['GOOGLE_CLOUD_PROJECT'],


FWIW, in situations such as this I like to add an arg so the environment dict can be injected in tests, rather than monkey-patching:

def get_engine_sampler(processor_id: str, gate_set_name: str, env: Dict[str, str] = os.environ): ... return engine.Engine(project_id=env['GOOGLE_CLOUD_PROJECT'], ...) # in tests: get_engine_sampler('foo', 'gates', {'GOOGLE_CLOUD_PROJECT': 'my-project'})

Some people are not too opposed to monkey-patching, but it always scares me :-)

Actually yeah, I should add an argument that defaults to the environment variable

I'm going to leave the monkey patching test in as well. I think the primary utility of this method is dispatching to the environment variable for project id so people can share code that doesn't have their personal project id hardcoded

mpharrigan · 2020-02-24T23:36:43Z

Anyone else have an opinion on this?

kevinsung · 2020-02-25T01:41:23Z

cirq/google/engine/engine_sampler.py

@@ -64,3 +64,34 @@ def run_sweep(
    @property
    def engine(self) -> 'cirq.google.Engine':
        return self._engine
+
+
+def get_engine_sampler(processor_id: str, gate_set_name: str,


I think that instead of taking a gate set name and having the NAMED_GATESETS dictionary, we should just take the gate set directly. There's no reason to add another layer of translation.

at the risk of angering @dstrain115 by invoking the following argument: it's more pythonic to expose enum's using strings, especially in a user-facing api. See e.g. many numpy functions. In this case there are several advantages

the gateset can be specified by command line arg and/or environment variable

you don't have to find the correct import to find the gateset objects

gate_set_name can live comfortably in a pandas dataframe or database table

What is pythonic anyway?!

Having a string for this is inconsistent with other methods that we already have, such as engine.run().

I thought we were discouraging (if not deprecating) engine.run as it does not follow the sampler api

The sampler api isn't flexible enough IMO to replace run. It requires that you stick any extra information about your run into the sampler constructor, so if you really do care about this information you will end up creating samplers all over the place.

Matt gave some good reasons to make it a string, so I'm fine with that. But we should be consistent.

the only good reason I can think of not to use a string is that it makes it less extensible, but since there are only a set number of blessed gate sets, this seems fine to me.

ok with string

dabacon · 2020-05-13T04:16:15Z

cirq/google/gate_sets.py

 document(XMON, """Gate set for XMON devices.""")
+
+NAMED_GATESETS = {
+    'sqrt-iswap': SQRT_ISWAP_GATESET,


Strange that these names don't match the gate set names (which are used when they are serialized).

dabacon

This seems useful and good to me. Think others agree strings are fine. Few minor comment.s

dabacon · 2020-07-16T20:27:18Z

cirq/google/engine/engine_sampler.py

@@ -64,3 +64,34 @@ def run_sweep(
    @property
    def engine(self) -> 'cirq.google.Engine':
        return self._engine
+
+
+def get_engine_sampler(processor_id: str, gate_set_name: str,


the only good reason I can think of not to use a string is that it makes it less extensible, but since there are only a set number of blessed gate sets, this seems fine to me.

dabacon · 2020-07-16T20:28:23Z

cirq/google/engine/engine_sampler.py

+    """Get an EngineSampler assuming some sensible defaults.
+
+    This uses the environment variable GOOGLE_GLOUD_PROJECT for the Engine
+    project_id, unless set explicitly. We assume you are using ProtoVersion.V2.


I think we can remove comment about protoversion since no one should be using v1.

dabacon · 2020-07-16T20:29:39Z

cirq/google/engine/engine_sampler.py

+    project_id, unless set explicitly. We assume you are using ProtoVersion.V2.
+
+    Args:
+        processor_id: Engine processor ID (from cloud console)


or from Engine.list_processors. Also nit: period to be consist with other args.

mpharrigan · 2020-07-16T21:22:09Z

I merged master and noted there's a new global variable GOOGLE_GATESETS that was missing the new "fsim" gateset; which I also added to this function

mpharrigan · 2020-07-17T00:14:17Z

We good?

#2767 introduced `get_engine_sampler`. This adds `get_engine` via a similar pattern, and in particular uses the same shared environment variable for projects, GOOGLE_PROJECT_ID. Also * deprecates `engine_from_environment` which used a different environment variable and a different nomenclature. It also fixes a bug in this methods id. * corrects a typo in the doc of `get_engine_sampler` * Gives the a sensible small string for an Engine object.

Remove tons of boilerplate

quantumlib#2767 introduced `get_engine_sampler`. This adds `get_engine` via a similar pattern, and in particular uses the same shared environment variable for projects, GOOGLE_PROJECT_ID. Also * deprecates `engine_from_environment` which used a different environment variable and a different nomenclature. It also fixes a bug in this methods id. * corrects a typo in the doc of `get_engine_sampler` * Gives the a sensible small string for an Engine object.

get_engine_sampler

5db9aa2

Remove tons of boilerplate

mpharrigan requested review from Strilanc, dabacon and kevinsung February 13, 2020 19:24

googlebot added the cla: yes Makes googlebot stop complaining. label Feb 13, 2020

vtomole reviewed Feb 18, 2020

View reviewed changes

cirq/google/engine/engine_sampler_test.py Show resolved Hide resolved

cirq/google/gate_sets.py Show resolved Hide resolved

vtomole approved these changes Feb 18, 2020

View reviewed changes

Merge branch 'master' into get-engine-sampler

9284a09

mpharrigan mentioned this pull request Feb 20, 2020

Observable Measurement #2781

Closed

Patch all the monkeys

98f9ab6

maffoo reviewed Feb 20, 2020

View reviewed changes

Also allow explicit project_id

2b5468f

kevinsung previously requested changes Feb 25, 2020

View reviewed changes

dabacon reviewed May 13, 2020

View reviewed changes

dabacon approved these changes Jul 16, 2020

View reviewed changes

mpharrigan added 2 commits July 16, 2020 14:17

Review comments

e8227f4

Merge remote-tracking branch 'origin/master' into get-engine-sampler

161c2a2

Order api.rst

613964a

balopat approved these changes Jul 17, 2020

View reviewed changes

balopat added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jul 17, 2020

CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jul 17, 2020

Merge branch 'master' into get-engine-sampler

9e1acec

CirqBot merged commit 8ad5186 into quantumlib:master Jul 17, 2020

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 Jul 17, 2020

dabacon mentioned this pull request Jul 18, 2020

Add get_engine and deprecate engine_from_environment #3151

Merged

mpharrigan deleted the get-engine-sampler branch July 20, 2020 18:55

mpharrigan mentioned this pull request Aug 11, 2020

Factor out parts of engine_utils.py quantumlib/ReCirq#53

Open

tonybruguier pushed a commit to tonybruguier/Cirq that referenced this pull request Aug 23, 2020

get_engine_sampler (quantumlib#2767)

adeae93

Remove tons of boilerplate

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

get_engine_sampler #2767

get_engine_sampler #2767

mpharrigan commented Feb 13, 2020

vtomole left a comment

mpharrigan commented Feb 19, 2020

maffoo Feb 20, 2020 •

edited

Loading

mpharrigan Feb 21, 2020

mpharrigan Feb 21, 2020

mpharrigan commented Feb 24, 2020

kevinsung Feb 25, 2020

mpharrigan Mar 5, 2020

dstrain115 Mar 5, 2020

mpharrigan Mar 6, 2020

dabacon May 11, 2020

kevinsung May 11, 2020

dabacon Jul 16, 2020

dabacon May 13, 2020

mpharrigan Jul 16, 2020

dabacon left a comment

dabacon Jul 16, 2020

dabacon Jul 16, 2020

mpharrigan Jul 16, 2020

dabacon Jul 16, 2020

mpharrigan Jul 16, 2020

mpharrigan commented Jul 16, 2020

mpharrigan commented Jul 17, 2020

get_engine_sampler #2767

get_engine_sampler #2767

Conversation

mpharrigan commented Feb 13, 2020

vtomole left a comment

Choose a reason for hiding this comment

mpharrigan commented Feb 19, 2020

maffoo Feb 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpharrigan commented Feb 24, 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

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

Choose a reason for hiding this comment

mpharrigan commented Jul 16, 2020

mpharrigan commented Jul 17, 2020

maffoo Feb 20, 2020 •

edited

Loading