Fixes to benchmark test code due to missing updates in dependency methods#1035
Fixes to benchmark test code due to missing updates in dependency methods#1035kenya-sk wants to merge 9 commits intotensorflow:masterfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the benchmarking infrastructure by refining build dependencies, optimizing tensor conversions in gradient benchmarks, and implementing a custom random circuit generator. The changes include converting symbol_names to a TensorFlow tensor to avoid repeated conversions during benchmarking and replacing a library-dependent circuit generator with a manual implementation. Feedback suggests simplifying the redundant logic for determining circuit interaction orientation and improving the connectivity of the generated random circuits to ensure they are more representative for benchmarking purposes.
|
|
||
| def _choice(rand_gen: float, sequence: Sequence[T]) -> T: | ||
| """Choose a pseudo-random element from a non-empty sequence.""" | ||
| return sequence[int(rand_gen * len(sequence))] |
There was a problem hiding this comment.
Can you check that the Python function random.choice(sequence) does not already do what this _choice() does?
There was a problem hiding this comment.
I checked random.choice(sequence), and it is not fully equivalent here. _choice() is intentionally aligned with the original ReCirq implementation: it uses the random.Random(seed).random float stream directly to preserve the same seeded circuit generation behavior.
random.choice() uses a different internal mechanism (_randbelow/getrandbits), which can change the selected gates even with the same seed.
To preserve reproducibility relative to the source implementation, I’ll keep _choice() and add a comment explaining why it is used instead of random.choice().
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the benchmarking suite by implementing a local version of the Boixo 2018 random circuit generator and refining build configurations. In benchmark_op_gradients.py, symbol names are now converted to tensors to optimize execution. Feedback suggests renaming the rand_gen parameter in _choice to better reflect its type and recommends passing a pre-defined qubit list to _add_cz_layer to avoid the overhead and non-determinism associated with circuit.all_qubits().
| def _choice(rand_gen: float, sequence: Sequence[T]) -> T: | ||
| """Choose a pseudo-random element from a non-empty sequence.""" | ||
| return sequence[int(rand_gen * len(sequence))] |
There was a problem hiding this comment.
The parameter name rand_gen is misleading as it represents a single float value (the result of a random generation) rather than a generator function or object. While functional, renaming it to something like rand_val or r would improve clarity and align with standard naming conventions for values.
| def _choice(rand_gen: float, sequence: Sequence[T]) -> T: | |
| """Choose a pseudo-random element from a non-empty sequence.""" | |
| return sequence[int(rand_gen * len(sequence))] | |
| def _choice(rand_val: float, sequence: Sequence[T]) -> T: | |
| """Choose a pseudo-random element from a non-empty sequence.""" | |
| return sequence[int(rand_val * len(sequence))] |
There was a problem hiding this comment.
I updated _choice() to match the original ReCirq pattern by passing the RNG callable itself, which makes rand_gen accurate here and keeps the seeded circuit-generation behavior explicit.
| """Add the next non-empty CZ layer and return the updated layer index.""" | ||
| cz_layer = None | ||
| while not cz_layer: | ||
| qubits = cast(Iterable[cirq.GridQubit], circuit.all_qubits()) |
There was a problem hiding this comment.
Iterating over circuit.all_qubits() inside a loop can be inefficient as it reconstructs the qubit set each time. Additionally, since all_qubits() returns a FrozenSet, the iteration order is not guaranteed to be stable across different Python environments due to hash randomization. For a benchmark, using the pre-defined and ordered qubits list from the caller would be more efficient and ensure deterministic circuit generation.
There was a problem hiding this comment.
I updated _add_cz_layer() to take the ordered qubits sequence from the caller instead of reconstructing circuit.all_qubits() each time. That makes the data flow clearer, avoids repeated set construction.
|
@kenya-sk I made one suggestion about the |
|
@mhucka |
2 participants
This PR fixes benchmark breakages in
benchmarks/scriptsand restores Bazel test coverage for the benchmark targets.Changes
benchmarks/scripts/BUILDand maderandom_clifford_circuitimportable by switching to py_library inbenchmarks/scripts/models/BUILD.benchmark_op_gradients.pyby passing symbol_names as a tensor.benchmark_random_circuit.pywith an in-file implementation based on ReCirq’sgoogle_v2_beyond_classical.pylogic (with source attribution), while preserving existing benchmark behavior (fixed-depth output).Note
Direct ReCirq adoption was intentionally deferred in this PR because it introduces a broader dependency migration than needed for a benchmark-fix change. In this repo, adding ReCirq as a required dependency impacts Python lockfile resolution and Bazel external dependency wiring, and we hit dependency-resolution conflicts while trying to make it mandatory. To keep this PR focused, low-risk, and test-stable, we inlined the minimal circuit-generation logic with source attribution, and left full ReCirq integration as follow-up work once dependency constraints are aligned.
Results
All benchmark tests have passed.