Cirq icon indicating copy to clipboard operation
Cirq copied to clipboard

Deprecating classes used by global singletons is challenging

Open verult opened this issue 3 years ago • 1 comments

Description of the issue

Example: SerializableGateSets

When we have a global singleton such as:

SYC_GATESET = cg.SerializableGateSet(...)

deprecating SerializableGateSet will cause tests to fail because all calls which raise deprecations must be declared in tests (using cirq.testing.assert_deprecated()), and because SYC_GATESET is loaded in init files, it's not possible to declare them within any particular unit test.

Possible solutions:

  • Prior to 1.0, we could've first deprecated global singletons and then remove global singletons and deprecate the class in two separate minor releases. This is no longer possible after 1.0 and before 2.0.
  • [Doesn't apply to the PR above] If SYC_GATESET doesn't need to be deprecated, SYC_GATESET could have been annotated with a more general type than SerializableGateSet, if possible, and the instance could have been swapped with a replacement class.
  • [Used by the PR above] deprecate an empty SerializableGateSet shell class that subclasses from _SerializableGateSet, which contains the class implementation. Change SYC_GATESET to cg._SerializableGateSet(...). A bit hacky.
  • Lazy load SYC_GATESET using PEP 562. If SYC_GATESET also needs to be deprecated, it's unclear how PEP 562 interacts with _compat.deprecate_attributes.
  • Relax the constraint that every deprecation must be declared in tests. Is it possible to enforce deprecation declarations in the companion test file only, i.e. serializable_gate_set_test.py?

Relaxing the constraint, if possible, will also reduce the workload for large deprecations. Adding deprecation declarations in tests is a tedious manual process. Large regex-based search & replace helps but only to an extent - the declarations need to be inserted slightly differently depending on how the test code is structured. If there's a looser guarantee that protects against most deprecation errors and has significantly less manual work, that'd be ideal.

Cirq version Cirq 1.0

verult avatar Jul 22 '22 22:07 verult

Pytest fixtures could potentially help us eliminate the hard requirement of declaring deprecations in tests. @pavoljuhas

verult avatar Jul 27 '22 18:07 verult