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

Remove fake backends based on BackendV1 and related tools #13805

Merged
merged 10 commits into from
Feb 28, 2025

Conversation

ElePT
Copy link
Contributor

@ElePT ElePT commented Feb 6, 2025

Summary

This PR branches off #13793 and contains the changes related to the fake backend removal. This PR is built on top of #13722 and currently BLOCKED by it.

Details and comments

TODOs:

  • remove stray prints
  • fix benchmarking test
  • fix visualization tests
  • reno

@coveralls
Copy link

coveralls commented Feb 10, 2025

Pull Request Test Coverage Report for Build 13587359090

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 1343 unchanged lines in 55 files lost coverage.
  • Overall coverage decreased (-0.9%) to 86.961%

Files with Coverage Reduction New Missed Lines %
qiskit/primitives/backend_estimator_v2.py 1 97.71%
qiskit/pulse/instructions/acquire.py 1 89.13%
qiskit/pulse/library/symbolic_pulses.py 1 92.75%
qiskit/pulse/library/waveform.py 1 97.83%
qiskit/result/counts.py 1 98.78%
qiskit/result/distributions/quasi.py 1 97.96%
qiskit/user_config.py 1 86.87%
qiskit/utils/units.py 1 93.55%
qiskit/providers/models/init.py 2 77.78%
qiskit/circuit/classical/expr/expr.py 4 96.67%
Totals Coverage Status
Change from base Build 13534039243: -0.9%
Covered Lines: 76032
Relevant Lines: 87432

💛 - Coveralls

@ElePT ElePT marked this pull request as ready for review February 11, 2025 12:51
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @enavarro51
  • @Qiskit/terra-core
  • @ajavadia
  • @levbishop
  • @nkanazawa1989
  • @t-imamichi

@ElePT ElePT changed the title [WIP] Remove fake backends based on BackendV1 and related tools Remove fake backends based on BackendV1 and related tools Feb 11, 2025
@ElePT ElePT added the on hold Can not fix yet label Feb 17, 2025
@ElePT ElePT added this to the 2.0.0 milestone Feb 17, 2025
…y tests that checked V1 functionality, mitigators (independent removal). Update rest of unit tests to use GenericBackendV2
@ElePT ElePT removed the on hold Can not fix yet label Feb 18, 2025
@eliarbel eliarbel self-assigned this Feb 20, 2025
Copy link
Contributor

@eliarbel eliarbel left a comment

Choose a reason for hiding this comment

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

Looks good overall. I've left some minor comments regarding a reno and some tests.

ElePT and others added 2 commits February 26, 2025 09:55
Co-authored-by: Eli Arbel <46826214+eliarbel@users.noreply.github.com>
Copy link
Contributor

@eliarbel eliarbel left a comment

Choose a reason for hiding this comment

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

Thanks @ElePT for making all the changes. One last typo fix in the reno which I've missed in my previous review and we're done (better to address now rather than wait for the big reno review just before the release)

Co-authored-by: Eli Arbel <46826214+eliarbel@users.noreply.github.com>
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

This LGTM. I had a few inline comments but nothing critical mostly idle musings as a I read through the code. There was one question about one test being added but it's not a blocker.

@@ -201,7 +201,7 @@ def transpile( # pylint: disable=too-many-return-statements
If unit is omitted, the default is 'dt', which is a sample time depending on backend.
If the time unit is 'dt', the duration must be an integer.
dt: Backend sample time (resolution) in seconds.
If ``None`` (default), ``backend.configuration().dt`` is used.
If ``None`` (default), ``backend.dt`` is used.
Copy link
Member

Choose a reason for hiding this comment

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

This might have slipped in from the backendv1 removal pr. Not a big deal though

@@ -153,7 +154,8 @@ def setup(self, _):
self.qasm_path = os.path.abspath(os.path.join(os.path.dirname(__file__), "qasm"))
large_qasm_path = os.path.join(self.qasm_path, "test_eoh_qasm.qasm")
self.large_qasm = QuantumCircuit.from_qasm_file(large_qasm_path)
self.melbourne = Fake20QV1()
self.melbourne = GenericBackendV2(num_qubits=20, coupling_map=MELBOURNE_CMAP, seed=0)
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately this will invalidate the benchmarking results for these benchmarks. There's no helping it since we're removing the code they rely on. But I'm going to miss the data:

https://qiskit.github.io/qiskit/#transpiler_levels.TranspilerLevelBenchmarks.time_quantum_volume_transpile_50_x_20?python=3.11

Screenshot_2025-02-28_07-02-54

@@ -183,6 +186,20 @@ def test_example_swap_bits(self):
).result()
self.assertEqual(result.get_counts(qc), {"010000": 1024})

def test_parallel_compile(self):
Copy link
Member

Choose a reason for hiding this comment

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

Did this test get lost? Also this won't trigger parallelism by default, we have a dedicated class that overrides the defaults to ensure we run under multiprocessing here: https://github.com/Qiskit/qiskit/blob/main/test/python/compiler/test_transpiler.py#L2628

But there is no harm in having this test.

def setUp(self):
super().setUp()
with self.assertWarns(DeprecationWarning):
backend = FakeOpenPulse2Q()
Copy link
Member

Choose a reason for hiding this comment

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

I find it disappointing that we didn't have tests in this file that just built a BackendConfiguration standalone. Not a comment on this PR, but more just a general comment. Removing these makes sense since we're removing BackendV1 after this, I was just surprised to see the file deleted here.

@mtreinish mtreinish dismissed eliarbel’s stale review February 28, 2025 12:11

Comments addressed

@mtreinish mtreinish enabled auto-merge February 28, 2025 12:11
@mtreinish mtreinish added this pull request to the merge queue Feb 28, 2025
Merged via the queue into Qiskit:main with commit fcb5d29 Feb 28, 2025
19 checks passed
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.

5 participants