Dispatch a builder with backendV1 and backendV2 #10150

to24toro · 2023-05-24T03:28:14Z

Summary

Added some codes of dispatching backends under pulse.builder. The number of the dispatching codes is not so many and we need not to use BackendV2Converter.

Details and comments

I have not implemented the tests about TestGates, TestBuilderComposition, and TestSubroutineCall in the version of backendV2 because they are the tests including the process with scheduling with backendV2 and this process don't work well.
related with Fix the output macros.measure with backendV2 #10135.
related with this issue.

qiskit-bot · 2023-05-24T03:28:23Z

One or more of the the following people are requested to review this:

@Qiskit/terra-core
@nkanazawa1989

to24toro · 2023-05-24T03:37:05Z

test/python/pulse/test_builder.py

+        self.assertScheduleEqual(schedule, reference)
+
+
+class TestChannelsV2(TestBuilderV2, TestChannels):


Is it Ok this implementation ? is it better to split the tests about the builder with backendV2 to another file or not inherit the tests of the builder with backendV1?

This is good question. Alternatively we can use data driven test (ddt) to test against both V1 and V2 models (and we need a bit of overhaul of the test). At least you can split the file because the file may become too long if you also mirror other tests for V2.

coveralls · 2023-05-24T03:55:15Z

Pull Request Test Coverage Report for Build 5242207642

37 of 38 (97.37%) changed or added relevant lines in 2 files are covered.
23 unchanged lines in 4 files lost coverage.
Overall coverage increased (+0.004%) to 85.908%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/pulse/builder.py	34	35	97.14%

Files with Coverage Reduction	New Missed Lines	%
qiskit/transpiler/passes/synthesis/unitary_synthesis.py	1	90.39%
crates/qasm2/src/lex.rs	2	90.63%
qiskit/extensions/quantum_initializer/squ.py	2	80.0%
crates/qasm2/src/parse.rs	18	96.65%

Totals
Change from base Build 5199973880:	0.004%
Covered Lines:	71239
Relevant Lines:	82925

💛 - Coveralls

nkanazawa1989

Thanks @to24toro. This implementation looks better than previous one. Please also add release note with fix. I think this must be backported to 0.24 unless it requires API change, because without this fix pulse users need to use V1 backend which is deprecated.

nkanazawa1989 · 2023-05-24T14:26:37Z

qiskit/pulse/builder.py

-        qubit_mem_slots={qubit: register.index for qubit, register in zip(qubits, registers)},
-    )
+    # backendV2
+    if hasattr(backend, "target"):


I think you can just pass backend without branching, because macros.measure implicitly dispatches the scheduler logic for backends with different models.

Fixed at a21cc84.

nkanazawa1989 · 2023-05-24T14:27:05Z

qiskit/pulse/builder.py

-        qubit_mem_slots={qubit: qubit for qubit in qubits},
-    )
+    # backendV2
+    if hasattr(backend, "target"):


Fixed at a21cc84.

nkanazawa1989 · 2023-05-24T14:30:54Z

qiskit/providers/backend.py

@@ -505,6 +505,26 @@ def qubit_properties(
            return self.target.qubit_properties[qubit]
        return [self.target.qubit_properties[q] for q in qubit]

+    def get_qubit_channels(self, qubit: int):


Can you implement this method as a helper function in the pulse.builder or pulse.utils? I want to backport this PR if possible, but this introduces API change and prevents us from the patch release with this. I also don't think there is demand for this method (i.e. V1 and V2 backend interface doesn't need to be fully compatible).

Fixed at a21cc84.

nkanazawa1989 · 2023-05-24T14:34:50Z

test/python/pulse/test_builder.py

+        self.assertScheduleEqual(schedule, reference)
+
+
+class TestChannelsV2(TestBuilderV2, TestChannels):


This is good question. Alternatively we can use data driven test (ddt) to test against both V1 and V2 models (and we need a bit of overhaul of the test). At least you can split the file because the file may become too long if you also mirror other tests for V2.

nkanazawa1989 · 2023-05-24T14:43:08Z

qiskit/pulse/builder.py

@@ -677,6 +677,9 @@ def get_context(self) -> ScheduleBlock:
    @_requires_backend
    def num_qubits(self):
        """Get the number of qubits in the backend."""
+        # backendV2
+        if hasattr(self.backend, "target"):


Is there any reason not to use isinstance(self.backend, BackendV2), maybe circular import? If required information is stored in the backend.target then it looks reasonable to use this condition. Because we can dynamically add any attribute to the instance, it could have .target that may provide unexpected value. I think instance check is much safer in this sense.

Fixed at a21cc84.

to24toro · 2023-05-31T06:20:01Z

I splited test codes for pulse.builder with backendV2 to test_builder_v2.py.

nkanazawa1989

Thanks for the update. Seems like the test for V2 backend dependency is not much, so probably we can combine both tests in a single file. Also we should have avoided new API for patch release.

nkanazawa1989 · 2023-06-02T03:18:07Z

test/python/pulse/test_builder_v2.py

+        self.assertEqual(target_qobj_transform(program), target_qobj_transform(target))
+
+
+class TestBuilderBaseV2(TestBuilderV2):


Does this test have V2 backend dependency? You can remove all the tests that builder context is not initialized with self.backend. How must test will remain then? If the number of test is not many, you can just add class TestBuilderBackendV2 to test_builder.py rather than creating new file.

Sorry. I overlooked it. However the number of test and the code volume of the test are almost same as 93a9262 even if I will delete the test you mention. Is it Ok?

I was probably overestimating the number of tests, but when I briefly checked your code I realized that number of test is not really large. For example, test_builder_v2.TestContextsV2 class, only three cases

test_transpiler_settings

test_scheduler_settings

test_phase_compensated_frequency_offset

take backend. I think if you remove other cases without backend dependency you can drastically reduce the amount of code to add. But having separate test file is probably okey.

[Q] Is there any reason why self.backend is not set in test_default_alignment_right and test_default_alignment_sequential while it is used in test_default_alignment_left?

Hmm good question. I don't think test_default_alignment_left needs backend. Perhaps I just copied it from another test case when I wrote this test.

I deleted duplicated and unnecesasry test codes at 891c1bd.

nkanazawa1989 · 2023-06-02T03:24:35Z

qiskit/pulse/utils.py

@@ -125,3 +130,24 @@ def deprecated_functionality(func):
        stacklevel=2,
        since="0.22.0",
    )(func)
+
+
+def get_qubit_channels(backend: BackendV2, qubit: int):


Sorry my previous comment was wrong. This is also API change (i.e. new feature) and we should wait for another release. To make this PR a part of patch release, you need to embed this logic in the qubit_channels to avoid adding new public API.

Fixed at 9b2b80d.

nkanazawa1989

The PR now looks good to me as one for patch release. Could you please redundant file changes to keep logical change minimum, because this is a patch to fix backend v2 problem (so unrelated files must not be included).

releasenotes/notes/fix-dispatching-backends-28aff96f726ca9c5.yaml

nkanazawa1989 · 2023-06-07T11:09:36Z

qiskit/pulse/utils.py

@@ -20,6 +22,9 @@
 from qiskit.pulse.exceptions import UnassignedDurationError, QiskitError
 from qiskit.utils.deprecation import deprecate_func, deprecate_function

+if TYPE_CHECKING:
+    from qiskit.providers.backend import BackendV2


Why this is needed? Likely BackendV2 type is not newly introduced in this file with this PR.

I forgot to remove it. FIxed at 9fee85e .

nkanazawa1989 · 2023-06-07T11:11:19Z

test/python/quantum_info/test_analyzation.py

@@ -100,7 +100,7 @@ def test_average_data_matrix_observable(self):
        self.assertAlmostEqual(mean_iz, 0, places=1)

    def test_make_dict_observable(self):
-        """Test make_dict_observable."""
+        """Test which makes_dict_observable."""


Is this necessary for this PR? Please keep logical change minimum.

It is my mistake. Fixed at c8d13f5 .

nkanazawa1989 · 2023-06-07T11:22:07Z

qiskit/pulse/builder.py

@@ -1163,6 +1178,31 @@ def qubit_channels(qubit: int) -> Set[chans.Channel]:
        such as in the case where significant crosstalk exists.

    """
+
+    # implement as the inner function to avoid API change in 0.24.


Suggested change

# implement as the inner function to avoid API change in 0.24.

# implement as the inner function to avoid API change for a patch release in 0.24.2.

Do you plan to move this to pulse.utils after this PR? I think it's worth having this in 0.25.

I consider it makes sense to implement it as a method in backendV2.

This qubit channels assume some coupler or CR architecture. For example if one wants to use ControlChannels for flux bias of flux tunable qubits, they don't appear in the coupling map.

nkanazawa1989 · 2023-06-07T11:23:35Z

qiskit/pulse/builder.py

+            if isinstance(active_backend(), BackendV2):
+                dt = active_backend().dt
+            else:
+                dt = active_backend().configuration().dt


I see this logic to get dt everywhere in this file. This is of course not the scope of this PR, but could you please add new builder command to get backend dt in 0.25 for cleanup?

Do you want me to add like below?

def get_dt_from_backend(backend): if isinstance(backend. backendV2): return backend.dt else: return backend.configuration().dt

Yes, in the followup PR for 0.25. I think this makes some logic cleaner.

qiskit/pulse/builder.py

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

…eature/dispatch_backend

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

…quest

nkanazawa1989

Thanks @to24toro . I'm really glad the pulse builder can work with V2 backend now.

* fix measure_v2 * modify measure_all * dispatch backend * add test of the builder with backendV2 * reconfigure test codes and some func * refactoring * add reno * fix _measure_v2 * fix backend.meas_map in measure_v2 * fix reno * delete get_qubit_channels from utils * add get_qubits_channels in qubit_channels * recostruct test about the builder with backendV2 * fix descriptions of test_macros * fix descriptions of test_macros again * delete import of backendV2 in utils * revert no need to modify code * Update releasenotes/notes/fix-dispatching-backends-28aff96f726ca9c5.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Update a commnet in qiskit/pulse/builder.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * remove import TYPE_CHECKING * removed test_builder.py utils.py and test_analyzation.py from pull request --------- Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> (cherry picked from commit a1615a8)

* fix measure_v2 * modify measure_all * dispatch backend * add test of the builder with backendV2 * reconfigure test codes and some func * refactoring * add reno * fix _measure_v2 * fix backend.meas_map in measure_v2 * fix reno * delete get_qubit_channels from utils * add get_qubits_channels in qubit_channels * recostruct test about the builder with backendV2 * fix descriptions of test_macros * fix descriptions of test_macros again * delete import of backendV2 in utils * revert no need to modify code * Update releasenotes/notes/fix-dispatching-backends-28aff96f726ca9c5.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Update a commnet in qiskit/pulse/builder.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * remove import TYPE_CHECKING * removed test_builder.py utils.py and test_analyzation.py from pull request --------- Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

* fix measure_v2 * modify measure_all * dispatch backend * add test of the builder with backendV2 * reconfigure test codes and some func * refactoring * add reno * fix _measure_v2 * fix backend.meas_map in measure_v2 * fix reno * delete get_qubit_channels from utils * add get_qubits_channels in qubit_channels * recostruct test about the builder with backendV2 * fix descriptions of test_macros * fix descriptions of test_macros again * delete import of backendV2 in utils * revert no need to modify code * Update releasenotes/notes/fix-dispatching-backends-28aff96f726ca9c5.yaml Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * Update a commnet in qiskit/pulse/builder.py Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> * remove import TYPE_CHECKING * removed test_builder.py utils.py and test_analyzation.py from pull request --------- Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com> (cherry picked from commit a1615a8) Co-authored-by: Kento Ueda <38037695+to24toro@users.noreply.github.com>

to24toro added 4 commits May 22, 2023 10:42

fix measure_v2

6a3b145

modify measure_all

3cd1192

dispatch backend

58023f5

add test of the builder with backendV2

93a9262

to24toro requested review from a team, eggerdj, wshanks and jyu00 as code owners May 24, 2023 03:28

to24toro commented May 24, 2023

View reviewed changes

to24toro mentioned this pull request May 24, 2023

Introduce BackendV2Converter to PulseBuilder #9501

Closed

nkanazawa1989 reviewed May 24, 2023

View reviewed changes

nkanazawa1989 self-assigned this May 24, 2023

nkanazawa1989 added stable backport potential The bug might be minimal and/or import enough to be port to stable mod: pulse Related to the Pulse module labels May 24, 2023

to24toro added 2 commits May 26, 2023 23:08

reconfigure test codes and some func

2cc5451

refactoring

a21cc84

to24toro added 4 commits May 31, 2023 15:29

add reno

b3607ee

fix _measure_v2

bd1e169

fix backend.meas_map in measure_v2

d70796f

fix reno

2a651dd

nkanazawa1989 reviewed Jun 2, 2023

View reviewed changes

to24toro added 3 commits June 5, 2023 13:54

delete get_qubit_channels from utils

8d92531

add get_qubits_channels in qubit_channels

9b2b80d

recostruct test about the builder with backendV2

891c1bd

to24toro force-pushed the feature/dispatch_backend branch from e1e8cc6 to 891c1bd Compare June 5, 2023 06:02

to24toro and others added 2 commits June 5, 2023 16:41

Merge branch 'main' into feature/dispatch_backend

1d3b471

fix descriptions of test_macros

a6363c3

to24toro requested a review from ikkoham as a code owner June 5, 2023 08:52

to24toro and others added 2 commits June 7, 2023 13:04

fix descriptions of test_macros again

3aeba17

Merge branch 'main' into feature/dispatch_backend

f3127df

nkanazawa1989 added this to the 0.24.2 milestone Jun 7, 2023

nkanazawa1989 reviewed Jun 7, 2023

View reviewed changes

to24toro and others added 7 commits June 7, 2023 21:14

delete import of backendV2 in utils

9fee85e

revert no need to modify code

c8d13f5

Update releasenotes/notes/fix-dispatching-backends-28aff96f726ca9c5.yaml

fecdcae

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

Merge remote-tracking branch 'origin/feature/dispatch_backend' into f…

f0ee72f

…eature/dispatch_backend

Update a commnet in qiskit/pulse/builder.py

c7a36dc

Co-authored-by: Naoki Kanazawa <nkanazawa1989@gmail.com>

Merge branch 'main' into feature/dispatch_backend

117931d

remove import TYPE_CHECKING

817d04a

This was referenced Jun 11, 2023

Add get_dt_from_backend func to the builder command #10253

Merged

Add a new argument concurrent_measurements to target class #10258

Merged

removed test_builder.py utils.py and test_analyzation.py from pull re…

0e204eb

…quest

nkanazawa1989 approved these changes Jun 12, 2023

View reviewed changes

nkanazawa1989 added this pull request to the merge queue Jun 12, 2023

Merged via the queue into Qiskit:main with commit a1615a8 Jun 12, 2023

mergify bot mentioned this pull request Jun 12, 2023

Dispatch a builder with backendV1 and backendV2 (backport #10150) #10265

Merged

nkanazawa1989 mentioned this pull request Jul 10, 2023

Update pulse builder to support (fake) V2 backend #9420

Closed

		self.assertScheduleEqual(schedule, reference)


		class TestChannelsV2(TestBuilderV2, TestChannels):

		self.assertEqual(target_qobj_transform(program), target_qobj_transform(target))


		class TestBuilderBaseV2(TestBuilderV2):

	# implement as the inner function to avoid API change in 0.24.
	# implement as the inner function to avoid API change for a patch release in 0.24.2.

Dispatch a builder with backendV1 and backendV2 #10150

Dispatch a builder with backendV1 and backendV2 #10150

Conversation

to24toro commented May 24, 2023 • edited Loading

Summary

Details and comments

qiskit-bot commented May 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 24, 2023 • edited Loading

Pull Request Test Coverage Report for Build 5242207642

💛 - Coveralls

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

to24toro commented May 31, 2023

nkanazawa1989 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

to24toro Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkanazawa1989 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

to24toro Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkanazawa1989 left a comment

Choose a reason for hiding this comment

to24toro commented May 24, 2023 •

edited

Loading

coveralls commented May 24, 2023 •

edited

Loading

to24toro Jun 2, 2023 •

edited

Loading

nkanazawa1989 left a comment •

edited

Loading

to24toro Jun 7, 2023 •

edited

Loading