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

Transforms shallow copy qnode #4736

Merged
merged 9 commits into from
Oct 26, 2023
Merged

Transforms shallow copy qnode #4736

merged 9 commits into from
Oct 26, 2023

Conversation

albi3ro
Copy link
Contributor

@albi3ro albi3ro commented Oct 25, 2023

Closes #4731 [sc-48599]

Currently, transforms deep copy the qnode when they are applied. Unfortunately, this also deep copies the device.

Copying the device leads to issues like:

  1. pickling errors for lightning.kokkos (see above issue)

  2. Unnecessary allocation of large amounts of memory

  3. No longer being able to use the original devices' tracker:

dev = qml.device('default.qubit')

@qml.transforms.compile
@qml.qnode(dev)
def circuit():
    return qml.state()

with dev.tracker:
    circuit()
dev.tracker.totals
{}

This bugfix downgrades the deep copy of a qnode to a shallow copy that preserves the device.

To better separate the original from the copy, QNode now defines a custom __copy__ method that shallow copies the execute_kwargs, transform_program, and gradient_kwargs. These were the three fully mutable properties of the qnode.

@albi3ro albi3ro requested a review from rmoyard October 25, 2023 20:09
@github-actions
Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@albi3ro albi3ro requested a review from a team October 25, 2023 20:09
Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Just two comments, otherwise looks good! Thanks 💯

pennylane/qnode.py Outdated Show resolved Hide resolved
pennylane/transforms/core/transform_dispatcher.py Outdated Show resolved Hide resolved
@timmysilv
Copy link
Contributor

if it's just the device, what about a simpler copy, like:

device = self.device
self.device = None
qnode_copy = super().__copy__(self)
self.device = qnode_copy.device = device
return qnode_copy

@albi3ro
Copy link
Contributor Author

albi3ro commented Oct 25, 2023

if it's just the device, what about a simpler copy, like:

device = self.device
self.device = None
qnode_copy = super().__copy__(self)
self.device = qnode_copy.device = device
return qnode_copy

@timmysilv

Is this a proposal for QNode.__copy__, or for use in TransformDispatcher.default_qnode_transform?

super().__copy__ is not defined, so we can't use that.

@timmysilv
Copy link
Contributor

it was for qnode.copy. is there no default copy behaviour if an object doesn't override the dunder? maybe not super(), but I assumed there was, and that we could access that. anyway, nvm if it's more of a pain

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (v0.33.0-rc0@a9ad7a0). Click here to learn what that means.

Additional details and impacted files
@@              Coverage Diff               @@
##             v0.33.0-rc0    #4736   +/-   ##
==============================================
  Coverage               ?   99.64%           
==============================================
  Files                  ?      380           
  Lines                  ?    33986           
  Branches               ?        0           
==============================================
  Hits                   ?    33864           
  Misses                 ?      122           
  Partials               ?        0           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rmoyard rmoyard self-requested a review October 26, 2023 15:53
Copy link
Contributor

@rmoyard rmoyard left a comment

Choose a reason for hiding this comment

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

Thanks it looks good 👍

@albi3ro albi3ro enabled auto-merge (squash) October 26, 2023 15:58
@albi3ro albi3ro merged commit 599b651 into v0.33.0-rc0 Oct 26, 2023
32 checks passed
@albi3ro albi3ro deleted the qnode-copy branch October 26, 2023 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants