Skip to content

Deprecating classes used by global singletons is challenging #5809

@verult

Description

@verult

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

Metadata

Metadata

Assignees

Labels

area/cleancodekind/healthFor CI/testing/release process/refactoring/technical debt itemspriority/reviewReassess the priority of this item at the next opportunity.triage/acceptedA consensus emerged that this bug report, feature request, or other action should be worked on

Type

No type

Projects

Status

Done

Relationships

None yet

Development

No branches or pull requests

Issue actions