Lock down CircuitOperation and ParamResolver #5548

95-martin-orion · 2022-06-17T17:16:43Z

Fixes #4327. Fixes #5507.

This PR converts all ParamResolver and CircuitOperation attributes to _private attributes with read-only @property accessors. The CircuitOperation.extern_keys field is not given an accessor as it is exclusively for decomposition and should never be referenced outside the class.

Attributes with mutable types (e.g. dicts) are still technically possible to mutate, but will fail mypy checks if this is attempted. Comments have been added to further clarify the prohibition on mutation.

This is technically a breaking change, but the workflows it breaks are limited to mutations of CircuitOperation and ParamResolver which were advised against in docstrings.

95-martin-orion · 2022-06-17T18:12:32Z

Changes to resolve type conflicts were made with the goal of minimizing impact to the code. This may result in slightly reduced performance in the affected code (specifically any of the new dict calls to avoid retyping Dict args to Mapping) but should not break any additional workflows.

If we really wanted to, we could retype the affected protocols to use Mapping to regain the lost performance, but that would be a huge breaking change.

maffoo · 2022-06-17T21:13:08Z

cirq-core/cirq/work/observable_measurement.py

@@ -531,7 +531,7 @@ def measure_grouped_settings(
    for max_setting, param_resolver in itertools.product(
        grouped_settings.keys(), study.to_resolvers(circuit_sweep)
    ):
-        circuit_params = param_resolver.param_dict
+        circuit_params = dict(param_resolver.param_dict)


Let's file an issue about fixing the types so we can avoid the copies here and in sample_expectation_values below. I think these should be fixable without worrying too much about compatibility, since AFAICT the dicts are not exposed anywhere in a way that is intended to be mutable, but we can deal with that in a future PR.

Opened #5554.

maffoo · 2022-06-17T21:18:27Z

cirq-core/cirq/circuits/circuit_operation.py

+                    frozenset(self.measurement_key_map.items()),
+                    self.param_resolver,
+                    self.parent_path,
+                    tuple([] if self.repetition_ids is None else self.repetition_ids),


alternately:

Suggested change

tuple([] if self.repetition_ids is None else self.repetition_ids),

() if self.repetition_ids is None else tuple(self.repetition_ids),

maffoo · 2022-06-17T21:20:11Z

cirq-core/cirq/circuits/circuit_operation.py

@@ -259,7 +315,7 @@ def _measurement_key_objs_(self) -> AbstractSet['cirq.MeasurementKey']:
                self,
                '_cached_measurement_key_objs',
                {
-                    protocols.with_measurement_key_mapping(key, self.measurement_key_map)
+                    protocols.with_measurement_key_mapping(key, dict(self.measurement_key_map))


Let's file an issue to fix the with_measurement_key_mapping protocol to we can avoid the copy here and on line 361 below.

Done: #5552. Unclear if this should be before or after 1.0 (may be more effort than it's worth likely breaking but not 100% certain), so I marked as triage-discuss.

maffoo · 2022-06-17T21:21:46Z

cirq-core/cirq/circuits/circuit_operation.py

+        kwargs = {
+            'circuit': self.circuit,
+            'repetitions': self.repetitions,
+            'qubit_map': self.qubit_map,
+            'measurement_key_map': self.measurement_key_map,
+            'param_resolver': self.param_resolver,
+            'repetition_ids': self.repetition_ids,
+            'parent_path': self.parent_path,
+            'extern_keys': self._extern_keys,
+            'use_repetition_ids': self.use_repetition_ids,
+            'repeat_until': self.repeat_until,
+        }
+        kwargs.update(changes)


can use literal unpacking since later value overwrite earlier ones:

Suggested change

kwargs = {

'circuit': self.circuit,

'repetitions': self.repetitions,

'qubit_map': self.qubit_map,

'measurement_key_map': self.measurement_key_map,

'param_resolver': self.param_resolver,

'repetition_ids': self.repetition_ids,

'parent_path': self.parent_path,

'extern_keys': self._extern_keys,

'use_repetition_ids': self.use_repetition_ids,

'repeat_until': self.repeat_until,

}

kwargs.update(changes)

kwargs = {

'circuit': self.circuit,

'repetitions': self.repetitions,

'qubit_map': self.qubit_map,

'measurement_key_map': self.measurement_key_map,

'param_resolver': self.param_resolver,

'repetition_ids': self.repetition_ids,

'parent_path': self.parent_path,

'extern_keys': self._extern_keys,

'use_repetition_ids': self.use_repetition_ids,

'repeat_until': self.repeat_until,

**changes,

}

maffoo · 2022-06-17T21:24:38Z

cirq-core/cirq/circuits/circuit_operation.py

+        # These fields are purely private: they should not be accessed
+        # from outside this class.
+        self._hash = None
+        self._cached_measurement_key_objs = None
+        self._cached_control_keys = None
+        self._cached_mapped_single_loop = None
+        self._extern_keys = extern_keys


Can make these cached properties instead:

from cirq._compat import cached_property ... @cached_property def _hash(self) -> int: return hash(...)

This should simplify some code below and let us get rid of a few more object.__setattr__ calls.

Done. (The __setattr__ calls were no longer needed anyhow, but it's a good change anyways)

Didn't know this existed! There are lots of other places we do manual caching for our immutable circuit and op classes and final results. (I looked into using functools.lru_cache for those, but that caused memory leaks)

Yes, cached_property is very useful (it's part of functools in the standard library starting in python 3.8, but we still have to use a backport for now until we drop support for 3.7).

maffoo

Minor comments, then LGTM.

maffoo · 2022-06-17T23:05:15Z

cirq-core/cirq/circuits/circuit_operation.py

+        }
+
+    def _measurement_key_objs_(self) -> AbstractSet['cirq.MeasurementKey']:
+        return self._measurement_key_objs  # type: ignore


What's the type error here?

Looks like it was carry-over from the old code. Removed.

maffoo · 2022-06-17T23:07:52Z

cirq-core/cirq/circuits/circuit_operation.py

+            circuit = protocols.resolve_parameters(
+                circuit, self.param_resolver, recursive=False
+            )
+        return cast(circuits.Circuit, circuit)


Do we have to return cirq.Circuit here or could this be cirq.AbstractCircuit? If the former then I'd suggest unfreezing instead:

Suggested change

return cast(circuits.Circuit, circuit)

return circuit.unfreeze(copy=False)

Public methods that consume this expect Circuit, so I think we're stuck with that. unfreeze seems to work well, though.

maffoo · 2022-06-17T23:09:51Z

cirq-core/cirq/circuits/circuit_operation.py

-                    )
-                ),
+    @cached_property
+    def _hash(self):


nit: add return type

Suggested change

def _hash(self):

def _hash(self) -> int:

maffoo · 2022-06-17T23:10:03Z

cirq-core/cirq/circuits/circuit_operation.py

            )
+        )
+
+    def __hash__(self):


here, too, can add return type.

daxfohl · 2022-06-17T23:24:50Z

cirq-core/cirq/circuits/circuit_operation.py

+        extern_keys: FrozenSet['cirq.MeasurementKey'] = frozenset(),
+        use_repetition_ids: bool = True,
+        repeat_until: Optional['cirq.Condition'] = None,
+    ):


Should the docstring be moved from the class to here?

daxfohl · 2022-06-17T23:26:46Z

cirq-core/cirq/circuits/circuit_operation.py

+            if q_new.dimension != q.dimension:
+                raise ValueError(f'Qid dimension conflict.\nFrom qid: {q}\nTo qid: {q_new}')
+
+        self._measurement_key_map = measurement_key_map or {}


Defensive copy for the dicts? Or comment in the docstring to state no defensive copy will be made?

This is a good call, as neither should be large enough for copy to be an issue and the dict must not change. Technically a breaking change, but that's par for the course on this PR.

CirqBot · 2022-06-22T18:58:50Z

Automerge cancelled: A required status check is not present.

Missing statuses: ['Pytest Ubuntu (3.7)', 'Pytest Ubuntu (3.8)', 'Pytest Ubuntu (3.9)', 'Pytest Windows (3.7)', 'Pytest Windows (3.8)', 'Pytest Windows (3.9)', 'Typescript lint check', 'Typescript tests', 'Typescript tests coverage']

Fixes #5552. Since this change only* affects type annotations, it is non-breaking. Any breakages due to `ParamResolver.param_dict` becoming immutable should instead refer to #5548, where this change was applied. *Okay, it also removes some `dict` calls, but those were added in #5548.

* Lock down CircuitOperation attributes. * Reduce attribute lockdown * Resolve type conflicts * review comments * docs and defensive copies * document error modes Co-authored-by: Cirq Bot <craiggidney+github+cirqbot@google.com>

Fixes quantumlib#5552. Since this change only* affects type annotations, it is non-breaking. Any breakages due to `ParamResolver.param_dict` becoming immutable should instead refer to quantumlib#5548, where this change was applied. *Okay, it also removes some `dict` calls, but those were added in quantumlib#5548.

* Lock down CircuitOperation attributes. * Reduce attribute lockdown * Resolve type conflicts * review comments * docs and defensive copies * document error modes Co-authored-by: Cirq Bot <craiggidney+github+cirqbot@google.com>

Fixes quantumlib#5552. Since this change only* affects type annotations, it is non-breaking. Any breakages due to `ParamResolver.param_dict` becoming immutable should instead refer to quantumlib#5548, where this change was applied. *Okay, it also removes some `dict` calls, but those were added in quantumlib#5548.

95-martin-orion added 2 commits June 17, 2022 09:56

Lock down CircuitOperation attributes.

7d4e446

Reduce attribute lockdown

b930f14

95-martin-orion requested a review from mpharrigan June 17, 2022 17:16

95-martin-orion requested review from a team, vtomole and cduck as code owners June 17, 2022 17:16

CirqBot added the size: M 50< lines changed <250 label Jun 17, 2022

95-martin-orion added the BREAKING CHANGE For pull requests that are important to mention in release notes. label Jun 17, 2022

Resolve type conflicts

1c7b2af

maffoo self-requested a review June 17, 2022 18:56

maffoo reviewed Jun 17, 2022

View reviewed changes

This was referenced Jun 17, 2022

Convert with_measurement_key_mapping to use Mapping #5552

Closed

Fix param_dict types in cirq/work #5554

Closed

review comments

cd02e2b

95-martin-orion requested a review from maffoo June 17, 2022 22:43

maffoo approved these changes Jun 17, 2022

View reviewed changes

daxfohl reviewed Jun 17, 2022

View reviewed changes

MichaelBroughton assigned maffoo Jun 18, 2022

95-martin-orion added 2 commits June 21, 2022 09:45

docs and defensive copies

4daf54f

document error modes

424ddee

MichaelBroughton added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jun 22, 2022

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

CirqBot added 2 commits June 22, 2022 11:47

Merge branch 'master' into cirq-private-subcirq

5d8be78

Merge branch 'master' into cirq-private-subcirq

082c4d4

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 Jun 22, 2022

Merge branch 'master' into cirq-private-subcirq

a0c3657

95-martin-orion merged commit 61fefe6 into quantumlib:master Jun 22, 2022

95-martin-orion deleted the cirq-private-subcirq branch June 22, 2022 21:08

95-martin-orion mentioned this pull request Jun 24, 2022

Make with_measurement_key_mapping live up to its name #5610

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lock down CircuitOperation and ParamResolver #5548

Lock down CircuitOperation and ParamResolver #5548

95-martin-orion commented Jun 17, 2022 •

edited

Loading

95-martin-orion commented Jun 17, 2022

maffoo Jun 17, 2022

95-martin-orion Jun 17, 2022

maffoo Jun 17, 2022

95-martin-orion Jun 17, 2022

maffoo Jun 17, 2022

95-martin-orion Jun 17, 2022

maffoo Jun 17, 2022

95-martin-orion Jun 17, 2022

maffoo Jun 17, 2022

95-martin-orion Jun 17, 2022

daxfohl Jun 17, 2022

maffoo Jun 18, 2022

maffoo left a comment

maffoo Jun 17, 2022

95-martin-orion Jun 21, 2022

maffoo Jun 17, 2022

95-martin-orion Jun 21, 2022

maffoo Jun 17, 2022

95-martin-orion Jun 21, 2022

maffoo Jun 17, 2022

95-martin-orion Jun 21, 2022

daxfohl Jun 17, 2022

95-martin-orion Jun 21, 2022

daxfohl Jun 17, 2022

95-martin-orion Jun 21, 2022

CirqBot commented Jun 22, 2022

	tuple([] if self.repetition_ids is None else self.repetition_ids),
	() if self.repetition_ids is None else tuple(self.repetition_ids),

	return cast(circuits.Circuit, circuit)
	return circuit.unfreeze(copy=False)

Lock down CircuitOperation and ParamResolver #5548

Lock down CircuitOperation and ParamResolver #5548

Conversation

95-martin-orion commented Jun 17, 2022 • edited Loading

95-martin-orion commented Jun 17, 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

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

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

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

CirqBot commented Jun 22, 2022

95-martin-orion commented Jun 17, 2022 •

edited

Loading