-
Notifications
You must be signed in to change notification settings - Fork 984
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
Tools to disable op validation #5530
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.
Consider using a ContextVar
rather than modifying global state.
except: | ||
raw_types._validate_qid_shape = temp | ||
raise | ||
finally: | ||
raw_types._validate_qid_shape = temp |
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.
Only need the finally
clause here. It'll run on failure too.
from cirq.ops import raw_types | ||
|
||
temp = raw_types._validate_qid_shape | ||
raw_types._validate_qid_shape = lambda *args: None |
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.
We might consider using contextvars
instead:
# raw_types.py
validate_qids: contextvars.ContextVar[bool] = contextvars.ContextVar('validate_qids', default=True)
def _validate_qid_shape(...) -> None:
if validate_qids.get():
return
...
# here
def disable_op_validation(...):
...
token = raw_types.validate_qids.set(False)
try:
yield
finally:
raw_types.validate_qids.reset(token)
This would be more scoped than modifying global state, which could otherwise cause hard-to-debug issues with threads or asynchrony.
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.
Done. My only misgiving with this is that it exposes the hack in raw_types.py
, which could lead to users applying it in ill-advised ways. Left a comment to mitigate this somewhat.
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.
After thinking about it, I think I agree with you that it's better to keep the hack itself contained and not modify raw_types.py
. If this turns out to be useful and we can't otherwise optimize the qid validation then we could migrate it into raw_types in the future. So feel free to ignore my suggestions and revert to your original version.
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.
Can revert the contextvars
suggestion, then LGTM.
This reverts commit c6aa29b.
Ended up needing to twiddle the tests to get coverage happy, but changes were fairly minor. |
Automerge cancelled: A status check is failing. |
Fixes quantumlib#5517. Does what it says on the tin: turns off op validation in the provided context. The try/except block in the manager ensures that validation is **always** re-enabled, regardless of what terrible things happen in the context.
Fixes #5517.
Does what it says on the tin: turns off op validation in the provided context. The try/except block in the manager ensures that validation is always re-enabled, regardless of what terrible things happen in the context.