-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deprecate XmonDevice, Foxtail and Bristlecone. #4864
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Left a few minor comments. Will approve once they are fixed.
@@ -150,3 +151,13 @@ | |||
from cirq_google.json_resolver_cache import _class_resolver_dictionary | |||
|
|||
_register_resolver(_class_resolver_dictionary) | |||
|
|||
__spec_copy__ = sys.modules[__name__].__spec__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to explicitly copy the spec and then pass it to deprecate_attributes
? Can't we just do:
sys.modules[__name__] = _compat.deprecate_attributes(
sys.modules[__name__],
{
'Bristlecone': ('v0.15', 'Bristlecone will no longer be supported.'),
'Foxtail': ('v0.15', 'Foxtail will no longer be supported.'),
},
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for some reason this update clobbers the spec entry in sys.modules
I'm not a python wizard, but this worked well enough. So it is needed.
@@ -43,7 +43,8 @@ def qubits(self): | |||
raise NotImplementedError() | |||
|
|||
|
|||
def test_init(): | |||
@mock.patch.dict(os.environ, clear='CIRQ_TESTING') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just add a ALLOW_DEPRECATION_IN_TEST = 'ALLOW_DEPRECATION_IN_TEST'
at the beginning of the file instead of adding a patch to each individual test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try as I might I couldn't get this for some reason or other, so I just left as mocks.
Automerge cancelled: A status check is failing. |
* Deprecate XmonDevice, Foxtail and Bristlecone. * Tanuj feedback. Co-authored-by: Cirq Bot <[email protected]>
* Deprecate XmonDevice, Foxtail and Bristlecone. * Tanuj feedback. Co-authored-by: Cirq Bot <[email protected]>
* Deprecate XmonDevice, Foxtail and Bristlecone. * Tanuj feedback. Co-authored-by: Cirq Bot <[email protected]>
Resolves #4856 .
qcs_notebook fxn now returns a Sycamore device which feels like "more correct" behavior.
BREAKING_CHANGE=Json and repr files were removed for these objects since it looks like they were never really needed.
The Foxtail and Bristlecone objects were just static singleton datastores we had in the library itself - in their current form there would never be any need to serialize them. Plus working in the deserialization of global singletons to avoid some warnings isn't something that seemed worth doing for this removal.
xref #4744