Use ParameterVector(uuid=...) in QPY deserialisation#16221
Conversation
jakelishman
added
on hold
Coverage Report for CI Build 26246719048Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Warning No base build found for commit Coverage: 87.48%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
When QPY deserialises `ParameterVectorElement`s, it first created a dummy `ParameterVectorElement` with a random UUID, then mutated an internal Python-space cache of the elements individually, as they were encountered, to reset them to the `root_uuid + offset` form. The idea that we _can_ find the root vector during deserialisation is predicated on the assumption that the `root_uuid + offset` form _must_ hold; if it doesn't, then the attempt to look up a cached version of the vector by looking at the `element_uuid - offset` would fail, so we'd never return the same vector. When `ParameterVectorElement` serialisation was introduced in QPY v3[^1], vectors were still being looked up by name, and the elements had completely disconnected UUIDs, which motivated the original system. This is no longer the case on both counts; it is now much easier simply to generate the vector correctly from the start, and never need to mutate internal private state. [^1]: 5c4cd2b: Fix ParameterVector handling in QPY serialization (Qiskitgh-7381)
|
One or more of the following people are relevant to this code:
|
| # vector[1] is not part of the circuit | ||
| self.assertTrue(vector[1] != new_vector[1]) |
There was a problem hiding this comment.
This is removed because it's generated equal even though it's not used. It was never an API contract that unused values must be different - that was an undesirable consequence.
raynelfss
left a comment
raynelfss
left a comment
There was a problem hiding this comment.
I don't get to play with QPY much here but it seems we're just creating ParameterVectors that match the serialized vector's UUID since uuid is now public (see #16216). This means we don't need to hack its first element to make it match the original's uuid.
This all makes sense to me so far. I had one comment about how things are being imported here, but other than that I don't have much to add.
| py.import("uuid")? | ||
| .getattr("UUID")? | ||
| .call((), Some(&kwargs))? |
There was a problem hiding this comment.
Why is the import done this way and not through the imports module with a OnceCell?
There was a problem hiding this comment.
Honestly, because it gets deleted in two PRs' time (#16228), and this was just me patching together a functional intermediate state to make review clearer.
|
Yeah, this is a simple stepping stone that makes the deserialisation a little faster, and avoids non-public internal-state mutation. The next PR (#16222) clears out a warning that (with this in place) is supporting a long-gone representation, and then the last moves all of |
raynelfss
left a comment
raynelfss
left a comment
There was a problem hiding this comment.
Thank you for explaining the changes here, this LGTM!
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
4 participants
When QPY deserialises
ParameterVectorElements, it first created a dummyParameterVectorElementwith a random UUID, then mutated an internal Python-space cache of the elements individually, as they were encountered, to reset them to theroot_uuid + offsetform.The idea that we can find the root vector during deserialisation is predicated on the assumption that the
root_uuid + offsetform must hold; if it doesn't, then the attempt to look up a cached version of the vector by looking at theelement_uuid - offsetwould fail, so we'd never return the same vector.When
ParameterVectorElementserialisation was introduced in QPY v31, vectors were still being looked up by name, and the elements had completely disconnected UUIDs, which motivated the original system. This is no longer the case on both counts; it is now much easier simply to generate the vector correctly from the start, and never need to mutate internal private state.Depends on #16216
AI/LLM disclosure
Footnotes
5c4cd2b: Fix ParameterVector handling in QPY serialization (Fix ParameterVector handling in QPY serialization #7381) ↩