Aquileo | Add a new proto DeviceParametersDiff which provides a compact way to bundle multiple DeviceParameters and their values by kmlau · Pull Request #6583 · quantumlib/Cirq

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

+// 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

+    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)

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

+// 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

  // 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:09:41Z

+    """
+    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-06T18:55:49Z

+  }
+  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

+    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-06T20:22:16Z

+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

+        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

+  // 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.

sync up to latest

senecameeks

A few nits but overall LGTM

Thanks, Kim-Ming!

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

+// 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

+// 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

+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

+
+    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)

…bundle multiple DeviceParameters and their values (quantumlib#6583) 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.

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

60023be

…ay to bundle multiple DeviceParameters and their values

kmlau requested review from a team, cduck, verult, vtomole and wcourtney 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

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

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

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:

Conversation

kmlau commented May 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kmlau commented May 1, 2024

Uh oh!

codecov Bot commented May 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmlau May 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

senecameeks left a comment

Choose a reason for hiding this comment

Uh oh!

kmlau commented May 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmlau commented May 29, 2024

Uh oh!

dstrain115 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

senecameeks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmlau commented May 1, 2024 •
edited

Loading

codecov Bot commented May 1, 2024 •
edited

Loading

kmlau May 2, 2024 •
edited

Loading

kmlau commented May 3, 2024 •
edited

Loading