Skip to content
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

Add Transformer API Interface and @cirq.transformer decorator #4797

Merged
merged 16 commits into from Jan 21, 2022

Conversation

tanujkhattar
Copy link
Collaborator

@tanujkhattar tanujkhattar commented Jan 4, 2022

Defines TRANSFORMER_TYPE and Implements the @transformer decorator, as proposed in https://tinyurl.com/cirq-circuit-transformers-api

All existing transformers will be rewritten to follow the new API once this is checked-in.
Implementation of TransformerStatsLoggerBase will follow in a separate PR.

Part of #4483

cc @maffoo PTAL at all the mypy magic.

@tanujkhattar tanujkhattar requested review from cduck, vtomole and a team as code owners January 4, 2022 22:19
@CirqBot CirqBot added the size: L 250< lines changed <1000 label Jan 4, 2022
Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. One high level question is do we need an interface for the statslogger or could we just make it concrete and put the implementation for register_initial and final inside that class ? I'm having trouble seeing what major differences each subclass might have.

cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@tanujkhattar tanujkhattar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed the comments.

For the logger, each subclass can chose to process, store and plot different information for the registered initial/final circuits (and their delta).

cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
@tanujkhattar
Copy link
Collaborator Author

@maffoo @dstrain115 I've resolved most of the comments. The two big pending questions are:

  • What should be the mypy type of the TRANSFORMER? See the discussion thread below for my comments.
  • Do you have strong opinions of whether we should we use two different ways -- base class and decorators, for appending the same logging functionality to class vs method transformers? (see comment on the discussion thread below)

@tanujkhattar
Copy link
Collaborator Author

@MichaelBroughton @dstrain115 @maffoo I've now also added the implementation of TransformerStatsLogger in this PR. Some additional changes are:

  • logger: TransformerStatsLogger is not an optional field anymore and defaults to NoOpTransformerStatsLogger(), which ignores all logging calls to avoid any performance degradations. This is done mainly to avoid the inconvenience of checking if context.logger is not None every time a user implementing a transformer wants to log something.
  • TransformerStatsLogger implements a logger which stores the initial/final circuits and the corresponding logs added in between for an active transformer. The implementation maintains an implicit dfs ordered graph for the nested transformer cases and a call to the show() method prints out the DFS ordered logs of each transformer stage (nested stages are appropriately indented).
  • Taking inspiration from common logging frameworks, I've renamed VERBOSE to DEBUG in LogLevels.

One TODO left is to add support for writing the logs to a file instead of printing to stdout when logger.show() is called (the feature would be a controlled by an additional parameter). I'll add this in a separate PR.

Please take another look and give feedback.

Copy link
Collaborator

@MichaelBroughton MichaelBroughton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me after some nits. Quick question: Do we expect to need a lot of class based transformers with __call__ defined ? Would we be able to keep things simple and just make them all standalone functions ?

cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api_test.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
@tanujkhattar
Copy link
Collaborator Author

@MichaelBroughton @maffoo I've made all changes as per the feedback. The major change now is the change in transformer API that I described in #4797 (comment)

Do we expect to need a lot of class based transformers with call defined ? Would we be able to keep things simple and just make them all standalone functions?

@MichaelBroughton The major problem in keeping transformers as standalone functions right now is the fact that we don't support additional arguments except circuit and context due to limitations in mypy.

It should be possible to add support for additional args in the decorator using Concatenate and ParamSpec introduced in https://www.python.org/dev/peps/pep-0612/. The mypy bug which tracks the implementation is still open python/mypy#8645

Copy link
Contributor

@maffoo maffoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for simplifying the types here; it is much easier to understand what is going on. I do still have a concern about using cirq.Circuit by default, especially since the implementation is copying the circuit on each transformer invocation, despite the requirement that transformers not mutate their inputs. I'd suggest using AbstractCircuit which should remove the need for separate TRANSFORMER and TRANSFORMER_TO_DECORATE types. But I think this is very close.

cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
cirq-core/cirq/transformers/transformer_api.py Outdated Show resolved Hide resolved
@tanujkhattar
Copy link
Collaborator Author

@dstrain115 Addressed all your comments.

@maffoo Incorporated your suggestions, as per our offline discussions, for further simplifying the mypy types. A transformer is now just

TRANSFORMER = Callable[['cirq.AbstractCircuit', TransformerContext], 'cirq.AbstractCircuit']

and the decorator implementation ensures that we don't modify the input type of a callable to preserve stricter return types wherever possible.

There is still a small glitch that the transformer type does not enforce any constraint on the argument names. This is only a problem in a small corner case where

@cirq.transformer
def f(c: cirq.AbstractCircuit, t: cirq.TransformerContext) -> cirq.Circuit:
    pass

f(circuit, context) # This works fine because positional arguments are used for invocation. 
f(c = circuit, t = context) # This will fail because we are explicitly using keyword arguments. 

To fix this, we'll have to change the transformer type to a protocol instead of a callable so we can enforce constraints on the argument names as well. However, I'd suggest let's do that in a separate PR as it's a small corner case and checking in this PR will help unblock all migration of all existing transformers.

WDYT ?

Copy link
Contributor

@maffoo maffoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the internals of the trasformer decorator can be cleaned up a bit, but that doesn't affect the exported API, so we can address that in a follow-up. LGTM!

@tanujkhattar
Copy link
Collaborator Author

Thanks @maffoo! Merging this now.

@tanujkhattar tanujkhattar added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Jan 21, 2022
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Jan 21, 2022
@CirqBot CirqBot merged commit 6119620 into quantumlib:master Jan 21, 2022
@CirqBot 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 Jan 21, 2022
maffoo added a commit that referenced this pull request Jan 21, 2022
Also cleans up some of the internals of the transformer decorator and
simplifies the types.

Follow-up to #4797
MichaelBroughton pushed a commit to MichaelBroughton/Cirq that referenced this pull request Jan 22, 2022
…tumlib#4797)

Defines `TRANSFORMER_TYPE` and Implements the `@transformer` decorator, as proposed in https://tinyurl.com/cirq-circuit-transformers-api

All existing transformers will be rewritten to follow the new API once this is checked-in. 
Implementation of `TransformerStatsLoggerBase` will follow in a separate PR. 

Part of quantumlib#4483

cc @maffoo PTAL at all the mypy magic.
tanujkhattar added a commit that referenced this pull request Jan 24, 2022
* Use a Protocol for TRANSFORMER to ensure common arg names

Also cleans up some of the internals of the transformer decorator and
simplifies the types.

Follow-up to #4797

* Fix Protocol import for 3.7

* Fixes from review

* Add type annotations in transformer implementation

Co-authored-by: Tanuj Khattar <tanujkhattar@google.com>
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
…tumlib#4797)

Defines `TRANSFORMER_TYPE` and Implements the `@transformer` decorator, as proposed in https://tinyurl.com/cirq-circuit-transformers-api

All existing transformers will be rewritten to follow the new API once this is checked-in. 
Implementation of `TransformerStatsLoggerBase` will follow in a separate PR. 

Part of quantumlib#4483

cc @maffoo PTAL at all the mypy magic.
rht pushed a commit to rht/Cirq that referenced this pull request May 1, 2023
…#4871)

* Use a Protocol for TRANSFORMER to ensure common arg names

Also cleans up some of the internals of the transformer decorator and
simplifies the types.

Follow-up to quantumlib#4797

* Fix Protocol import for 3.7

* Fixes from review

* Add type annotations in transformer implementation

Co-authored-by: Tanuj Khattar <tanujkhattar@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: L 250< lines changed <1000
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants