Add a new proto DeviceParametersDiff which provides a compact way to bundle multiple DeviceParameters and their values #6583

kmlau · 2024-05-01T23:44:54Z

The new proto DeviceParametersDiff is for a user to compose a RunJobRequest for invoking RunJob rpc on a Cirq server, in particular to populate the RunJobRequest.run_context field with device parameters overrides to customize the circuit(s) execution with some control on the device's samples data.

This is based on a design reviews to add "device parameters overrides" before executing circuits sweeping.

I renamed some proto type names from similar internal data structure to prevent reference to internal infrastructures.

…ay to bundle multiple DeviceParameters and their values

kmlau · 2024-05-01T23:46:09Z

cc @dstrain115

codecov · 2024-05-01T23:59:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.81%. Comparing base (6709046) to head (8e79f46).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6583   +/-   ##
=======================================
  Coverage   97.81%   97.81%           
=======================================
  Files        1061     1063    +2     
  Lines       91455    91501   +46     
=======================================
+ Hits        89454    89500   +46     
  Misses       2001     2001

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dstrain115 · 2024-05-02T13:15:44Z

cirq-google/cirq_google/api/v2/run_context.proto

+// DeviceParametersDiff from a list of (DeviceParameter, ArgValue) pairs.
+message DeviceParametersDiff {
+  message Dir {
+    int32 parent = 1;


This should explain that this is an index into the strs array.
Also should clarify that -1 means root.

comments added to indicate the various int32 fields as indices into which arrays, among groups and strs.

dstrain115 · 2024-05-02T22:33:19Z

cirq-google/cirq_google/api/v2/run_context.py

+    dirs_seen: set[tuple[int, int]] = set()
+
+    for device_param, value in device_params:
+        parent = -1  # no parent for the 1st path component


I think "-1" should be a constant, as we will likely need it for decoding the diff too.

added a constant of -1, with comment.

senecameeks

How will users interact with this new proto? IIUC, the internal diff proto has an abstraction layer that clients can easily interact with for creating a Diff which can then be easily packaged into its proto form. Can a similar well defined abstraction layer e.g class DeviceParameterDiff exist for this proto as well? It doesn't have to include all the APIs as the one linked, but a clear abstraction layer will help me see how this proto will later get integrated into a Job's run context. WDYT?

kmlau · 2024-05-03T16:04:21Z

How will users interact with this new proto? IIUC, the internal diff proto has an abstraction layer that clients can easily interact with for creating a Diff which can then be easily packaged into its proto form. Can a similar well defined abstraction layer e.g class DeviceParameterDiff exist for this proto as well? It doesn't have to include all the APIs as the one linked, but a clear abstraction layer will help me see how this proto will later get integrated into a Job's run context. WDYT?

thanks for the comment.

the PR includes a helper function to construct a DeviceParametersDiff proto out of a list of DeviceParameter and their values. please see the module run_context.py in this PR. Suppose you want to override some readout parameters on 2 qubits, and build a RunContext proto with such overrides, you may use the following code


import run_context

device_params_override = run_context.to_device_parameters_diff([
    (
        run_context_pb2.DeviceParameter(
            path=["q1_2", "readout_default", "readoutDemodDelay"], units="ns"
        ),
        program_pb2.ArgValue(float_value=5.0),
    ),
    (
        run_context_pb2.DeviceParameter(
            path=["q3_4", "readout_default", "readoutFidelities"]),
        program_pb2.ArgValue(
            double_values=program_pb2.RepeatedDouble(values=[0.991, 0.993])
        ),
    ),
])

my_rc = run_context_pb2.RunContext(
    device_parameters_override=device_params_override)

senecameeks · 2024-05-03T17:49:28Z

cirq-google/cirq_google/api/v2/run_context.py

+_EMPTY_DIR_PATH_IDX = -1
+
+
+def to_device_parameters_diff(


My understanding is that users never interact with run_context directly. Instead they call run_sweep and pass in cirq.Sweepable and the number of repetitions, and we construct the run_context on their behalf.

It would be nice to have a similar cirq.DeviceParametesDiff concept that the user can pass into run_sweep that can later be serialized into the run_context. WDYT?

the proto is for a user to construct a RunJobRequest, in order to invoke RunJob on a cirq server.
the DeviceParametersDiff proto is not intended for uses to make python local call to run_sweep.

let me clarify it in the pr description.

I expect that this will be something to specify outside of a run_sweep/Job from the user's perspective - e.g. as part of a sampler - even though we pass it to the Engine API as part of the job spec. +1 to @kmlau that we don't need to add that interface yet though.

oh, I was incorrectly assuming you were referring to our internal run_sweep API, https://github.com/qh-lab/pyle/blob/master/src/pyle/dataking/cirq_sweep/_base.py#L87 , and was not aware there is a similarly named function in cirq-google repo.

yes, I will need to make changes to the cirq run_sweep function to incorporate an optional device parameters diff concept. would it be viable to do it later, since we will need to make changes to cirq server first, before exposing the device parameeters overriding in run_sweep call?

I agree that we will need to figure out how to pass this into a sampler.run_sweep(), but let's just deal with the proto changes in this PR.

wcourtney · 2024-05-03T20:46:51Z

cirq-google/cirq_google/api/v2/run_context.proto

+// See run_context.py for utility function to construct a
+// DeviceParametersDiff from a list of (DeviceParameter, ArgValue) pairs.
+message DeviceParametersDiff {
+  message Dir {


Avoid abbreviation, here and below: go/pystyle#naming

rename from "Dir" to "ResourceGroup"

wcourtney · 2024-05-03T20:50:43Z

cirq-google/cirq_google/api/v2/run_context.proto

@@ -10,6 +12,10 @@ option java_multiple_files = true;
 message RunContext {
  // The parameters for operations in a program.
  repeated ParameterSweep parameter_sweeps = 1;
+
+  // optional override of select device parameters before program


Is it valid for both the parameter_sweeps and device_parameter_override to specify the same parameter(s)? The ordering of these two is ambiguous, which only makes sense if they're disjoint, but either way it should be documented.

good catch. yes the proto definition allows such overlap. however the override will have no effect since it is applied before the circuit sweep is executed.
I have put in proto field comment to document such case.

wcourtney · 2024-05-03T21:08:17Z

cirq-google/cirq_google/api/v2/run_context.py

+_EMPTY_DIR_PATH_IDX = -1
+
+
+def to_device_parameters_diff(


I expect that this will be something to specify outside of a run_sweep/Job from the user's perspective - e.g. as part of a sampler - even though we pass it to the Engine API as part of the job spec. +1 to @kmlau that we don't need to add that interface yet though.

wcourtney · 2024-05-03T21:09:41Z

cirq-google/cirq_google/api/v2/run_context.py

+    """
+    diff = run_context_pb2.DeviceParametersDiff()
+
+    # Maps a string to its token id. A string is a component of a device parameter's path


Nit: add punctuation.

wcourtney · 2024-05-03T22:00:53Z

cirq-google/cirq_google/api/v2/run_context.proto

+    int32 dir = 1;
+    // as index into the strs array.
+    int32 name = 2;
+    ArgValue value = 3;


DeviceParameter - which is meant to represent the same parameters being overridden - can only sweep over float values. Should these two types align on the valid types?

the possible device parameters to be override can take values other than floats.
to allow such flexibility, I would vouch for the ArgValue variant type.

Agree with @kmlau . The sweeps are restricted to floats since sweeps in cirq are restricted to floats. This isn't a sweep and just sets a value, and there are non-float device parameters.

Let's document this inconsistency above though.

add additional docstring to explain the need for ArgValue submessage.

wcourtney · 2024-05-06T18:55:49Z

cirq-google/cirq_google/api/v2/run_context.proto

+  }
+  message Key {
+    // directory hosting this key, as index into dirs array.
+    int32 dir = 1;


Why does Key reference the dir index rather than just being a sub-message?

wcourtney · 2024-05-06T18:58:11Z

cirq-google/cirq_google/api/v2/run_context.proto

+    int32 name = 2;
+    ArgValue value = 3;
+  }
+  message Del {


Could deletions be incorporated into the Dir as a list of strings indicating keys that should be deleted?

wcourtney · 2024-05-06T19:02:30Z

cirq-google/cirq_google/api/v2/run_context.proto

+// values from this bundle before executing a circuit sweep.
+// See run_context.py for utility function to construct a
+// DeviceParametersDiff from a list of (DeviceParameter, ArgValue) pairs.
+message DeviceParametersDiff {


Is there a reason that the parameter hierarchy isn't encoded into the message structure itself? i.e. why do we use indices to reconstruct the path from disjoint lists rather than a recursive structure like this:

message Resource { string name = 1; repeated Resource sub_resources = 2; repeated map<string, ArgValue> attributes = 3; }

If repeated names are a concern, we could still use a token dictionary for identifiers as is done in the current iteration.

what does a repeated attributes field mean?

I think this is just a typo. A map already incorporates multiple map entries.

instead of inventing a new message as you allude to, the proposed DeviceParametersDiff is based on internal RegistryDiff data structure, which is well tested and in production uses.
I would prefer to stay with the current DeviceParametersDiff definition. wdyt?

wcourtney · 2024-05-06T20:22:16Z

cirq-google/cirq_google/api/v2/run_context_test.py

+import google.protobuf.text_format as text_format
+
+
+def test_to_device_parameters_diff() -> None:


Test names should summarize the behavior being tested and its expected outcome.

go/unit-testing-practices#naming

renamed the test to call out the behavior to be tested.

wcourtney · 2024-05-06T20:25:13Z

cirq-google/cirq_google/api/v2/run_context_test.py

+        strs: "phase_i_rad"
+        strs: "q5_6"
+    """
+    assert text_format.Parse(expected_diff_pb_text, run_context_pb2.DeviceParametersDiff()) == diff


Test behaviors, not methods.

Behaviors should be tested independently.

go/unit-testing-practices#behavior-testing-examples

sync up

kmlau · 2024-05-29T21:37:17Z

friendly ping @senecameeks and @dstrain115

dstrain115

Some final comments, then LGTM

dstrain115 · 2024-05-30T19:47:51Z

cirq-google/cirq_google/api/v2/run_context.proto

+  // Optional override of select device parameters before program
+  // execution. Note it is permissible to specify the same device parameter
+  // here and in a parameter_sweeps, as sweep.single_sweep.parameter.
+  // However the override will have no effect since it is applied before


This wording is confusing. Can we clarify it? Maybe something like "If the same parameter is supplied in both places, then XYZ will happen"

reworded the docstring here.

dstrain115 · 2024-05-30T19:54:16Z

cirq-google/cirq_google/api/v2/run_context.proto

+    int32 dir = 1;
+    // as index into the strs array.
+    int32 name = 2;
+    ArgValue value = 3;


Agree with @kmlau . The sweeps are restricted to floats since sweeps in cirq are restricted to floats. This isn't a sweep and just sets a value, and there are non-float device parameters.

Let's document this inconsistency above though.

dstrain115 · 2024-05-30T19:56:39Z

cirq-google/cirq_google/api/v2/run_context.py

+_EMPTY_DIR_PATH_IDX = -1
+
+
+def to_device_parameters_diff(


I agree that we will need to figure out how to pass this into a sampler.run_sweep(), but let's just deal with the proto changes in this PR.

dstrain115 · 2024-05-30T19:59:28Z

cirq-google/cirq_google/api/v2/run_context.py

+    """
+    diff = run_context_pb2.DeviceParametersDiff()
+
+    @functools.lru_cache(maxsize=None)


Is this safe to cache? What happens if we call to_device_parameters_diff twice? token(s) may not be at the same index in a subsequent call? Will the cache of the nested function handle this correctly?

yes, the cache decoration on an inner function def token_id(...) gives correct behavior on multiple calls to the outer function to_device_parameters_diff .

I have added an extra unit test to make multiple calls to to_device_parameters_diff, with correct token id assignments.

dstrain115 · 2024-05-30T20:00:39Z

cirq-google/cirq_google/api/v2/run_context.py

+    # Maps a resource group path to its index in diff.groups.
+    resource_groups_index: dict[tuple[str, ...], int] = {tuple(): _EMPTY_RESOURCE_PATH_IDX}
+
+    def resource_path_id(path: tuple[str, ...]) -> int:


Probably should add a docstring.

docstring for resource_path_id added

sync up to latest

senecameeks

A few nits but overall LGTM

Thanks, Kim-Ming!

senecameeks · 2024-05-31T17:28:59Z

cirq-google/cirq_google/api/v2/run_context.proto

+// The main use case is to set those parameters with the
+// values from this bundle before executing a circuit sweep.
+// Note multiple device parameters may have common ancestor paths
+// and/or share the same parameter names. A DeviceParamaetersDiff


Nit: fix spelling

senecameeks · 2024-05-31T17:29:37Z

cirq-google/cirq_google/api/v2/run_context.proto

+// Note multiple device parameters may have common ancestor paths
+// and/or share the same parameter names. A DeviceParamaetersDiff
+// stores the resource groups hierarchy extracted from the DeviceParameters'
+// paths and maintains a string tables; thereby storing ancestor resource


Nit: should this be maintains a table of strings

updated per suggested.

senecameeks · 2024-05-31T17:33:39Z

cirq-google/cirq_google/api/v2/run_context.py

+def to_device_parameters_diff(
+    device_params: Sequence[tuple[run_context_pb2.DeviceParameter, program_pb2.ArgValue]]
+) -> run_context_pb2.DeviceParametersDiff:
+    """Constructs a DeviceParametersDiff from multiple DeviceParameters and values


Nit: add punctuation

senecameeks · 2024-05-31T17:39:04Z

cirq-google/cirq_google/api/v2/run_context.py

+
+    def resource_path_id(path: tuple[str, ...]) -> int:
+        """Computes the index of a path in diff.groups."""
+        idx = resource_groups_index.get(path)


Nit: prefer explicitly setting a default resource_groups_index.get(path, None)

Add a new proto DeviceParametersDiff proto which provides a compact w…

60023be

…ay to bundle multiple DeviceParameters and their values

kmlau requested review from wcourtney, vtomole, cduck, verult and a team as code owners May 1, 2024 23:44

kmlau changed the title ~~Add a new proto DeviceParametersDiff proto which provides a compact way to bundle multiple DeviceParameters and their values~~ Add a new proto DeviceParametersDiff which provides a compact way to bundle multiple DeviceParameters and their values May 1, 2024

add copyright notices

cecfa17

kmlau added 4 commits May 2, 2024 00:45

fix import order

f2345eb

add device_parameters_override field to RunContext

d787ccc

use parent id of -1 for no parent

2b904c0

remove a unnecessary if

bf8e857

senecameeks self-requested a review May 2, 2024 21:04

dstrain115 reviewed May 2, 2024

View reviewed changes

address comments, and bug fixes

9dd0249

kmlau requested a review from dstrain115 May 2, 2024 23:42

senecameeks requested changes May 3, 2024

View reviewed changes

kmlau requested a review from senecameeks May 3, 2024 16:06

senecameeks reviewed May 3, 2024

View reviewed changes

kmlau requested a review from senecameeks May 3, 2024 18:41

CirqBot added the size: L 250< lines changed <1000 label May 5, 2024

wcourtney reviewed May 6, 2024

View reviewed changes

kmlau added 5 commits May 6, 2024 21:43

address comments

863e119

address comments

961b0e9

fix lint

5cae0a2

Merge branch 'main' into add-device_parameters_diff-proto

29b5383

sync up

rename tests for go/unit-testing-practices#naming

a377591

kmlau requested review from NoureldinYosri and wcourtney May 28, 2024 22:59

dstrain115 approved these changes May 30, 2024

View reviewed changes

kmlau added 3 commits May 31, 2024 01:39

addressed review comments

2d43297

Merge branch 'main' into add-device_parameters_diff-proto

33709a9

sync up to latest

fix format

9fc60e5

senecameeks approved these changes May 31, 2024

View reviewed changes

addressed review comments

8e79f46

pavoljuhas merged commit d077532 into quantumlib:main May 31, 2024
34 checks passed

kmlau deleted the add-device_parameters_diff-proto branch May 31, 2024 20:44

		import google.protobuf.text_format as text_format


		def test_to_device_parameters_diff() -> None:

Add a new proto DeviceParametersDiff which provides a compact way to bundle multiple DeviceParameters and their values #6583

Add a new proto DeviceParametersDiff which provides a compact way to bundle multiple DeviceParameters and their values #6583

Conversation

kmlau commented May 1, 2024 • edited

kmlau commented May 1, 2024

codecov bot commented May 1, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

kmlau May 2, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

senecameeks left a comment

Choose a reason for hiding this comment

kmlau commented May 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wcourtney May 3, 2024 • edited

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

wcourtney May 3, 2024 • edited

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

kmlau commented May 29, 2024

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

Choose a reason for hiding this comment

senecameeks 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

kmlau commented May 1, 2024 •

edited

codecov bot commented May 1, 2024 •

edited

kmlau May 2, 2024 •

edited

kmlau commented May 3, 2024 •

edited

wcourtney May 3, 2024 •

edited

wcourtney May 3, 2024 •

edited