Discard cache for standard gates in assign_parameters
#13557
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
raynelfss
added
bug
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 12285836607Details
💛 - Coveralls |
There was a problem hiding this comment.
Thanks @raynelfss, this seems to work nicely.
I left just one comment and will merge once addressed.
crates/circuit/src/circuit_data.rs
Outdated
Comment on lines
1394
to
1396
| // Standard gates can all rebuild their definitions, so if the | ||
| // cached py_op exists, update the `params` attribute and clear out | ||
| // any existing cache. |
There was a problem hiding this comment.
Should we update this comment and perhaps add a GitHub link to the relevant issue?
Cryoris
reviewed
Dec 12, 2024
| Fix incorrect behavior in :class:`.CircuitData` in which, upon parameter assignment, | ||
| we attempted to modify the cached operation inside of a ``PackedInstruction``. Now | ||
| we instead discard said cache prompting the ``PackedInstruction`` to build a new Python | ||
| operation should it be needed. |
There was a problem hiding this comment.
It would be nice to link the issue here, but let's do that in #13556.
Cryoris
added
stable backport potential
mergify bot
pushed a commit
that referenced
this pull request
Dec 12, 2024
* Fix: Discard cache when assigning new parameters in `CircuitData`. * Docs: Add release note * Test: Add test case * Docs: Add relevant comment (cherry picked from commit 17a2ccc)
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 12, 2024
) * Fix: Discard cache when assigning new parameters in `CircuitData`. * Docs: Add release note * Test: Add test case * Docs: Add relevant comment (cherry picked from commit 17a2ccc) Co-authored-by: Raynel Sanchez <87539502+raynelfss@users.noreply.github.com>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This is an initial fix to #13504, and should precede #13543.
The following commits discard the cached gate when assigning parameters for a standard gate rather than trying to modify the Python object.
Details and comments
A more in-depth fix is present in #13543, but since it is such a big change, we've decided to include this smaller fix to allow more time to re-evaluate our approach.
When retrieving a
PackedInstructionfrom theEquivalenceLibraryduring thecompose_transformsphase of theBasisTranslatorthe parameter modifications done in other places are not reflected on the cached gate, therefore leading to issues when runningQISKIT_PARALLEL=TRUEdue to a modified object reference when callingCircuitData::assign_parameters().In the case of a standard gate, we were not removing the old cache but instead modifying it in place (which would not result in the expected outcome), while for other gate instances, we rebuild the non-cached item. Now we instead discard the cache in the
StandardGatecase too, getting rid of the stale shared reference.