Update backend model up-conversion logic #11095

MozammilQ · 2023-10-23T17:03:21Z

Summary

This updates the logic of backend model up-conversion. The logic is highly motivated with the one in qiskit_ibm_provider which is ‎target_from_server_data in qiskit_ibm_provider/utils/json_decoder.py

The following code:

from qiskit.providers.fake_provider import FakeNairobiV2
FakeNairobiV2().instruction_schedule_map.get('measure', [0]).draw()

should produce this:

instead, it produces this:

Details and comments

The problem occurs here
I tried using convert_to_target() from qiskit/providers/fake_provider/utils/json_decoder.py but that also didn't solve the problem.
target_from_server_data() from qiskit_ibm_provider/utils/json_decoder.py does solve the problem, so I replaced the contents of convert_to_target() in qiskit/providers/backend_compat.py with the content of target_from_server_data().

Credit: @to24toro , has been kind enough to provide a helping hand here :)

…_target from qiskit.providers.backend_compat.py with contents of method target_from_server_data from qiskit_ibm_provider/utils/json_decoder.py

qiskit-bot · 2023-10-25T11:57:56Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

@Qiskit/terra-core
@nkanazawa1989

MozammilQ · 2023-10-25T11:58:57Z

@to24toro , please have a look if this is good enough :)

to24toro · 2023-11-03T07:32:38Z

qiskit/providers/backend_compat.py

+    return qubit_props
+
+
+def convert_to_target_legacy(


We would like to replace this convert_to_target_legacy with new one.
Maybe you separate them because these have two different inputs.
I think that it might be better to deprecate unnecessary inputs in new convert_to_target.
In Qiskit development process, we need to add deprecate_arg before change API. (you can see some examples if you search @deprecate_arg in qiskit.)

I have not deprecated any argument of convert_to_target because these arguments are used in BackendV2Converter of backend_compat.py.
Aer specifically uses argument custom_name_mapping to pass in all aer related instructions to target. The reason I separated old codes into convert_to_target_legacy was because in this PR I just focused on introducing convert_to_target to FakeBackendV2.
Now, there is no convert_to_target_legacy, only one function convert_to_target is passed to both FakeBackendV2 and BackendV2Converter.
All arguments of convert_to_target are relevant and works fine to my knowledge :)

to24toro · 2023-11-03T07:33:25Z

qiskit/providers/backend_compat.py

+    from qiskit.transpiler.target import Target
+    from qiskit.circuit.controlflow import ForLoopOp, IfElseOp, SwitchCaseOp, WhileLoopOp
+    from qiskit.circuit.library.standard_gates import get_standard_gate_name_mapping
+    from qiskit.qobj.pulse_qobj import PulseLibraryItem
+    from qiskit.qobj.converters.pulse_instruction import QobjToInstructionConverter
+    from qiskit.providers.models.pulsedefaults import Command
+    from qiskit.pulse.calibration_entries import PulseQobjDef
+    from qiskit.circuit.parameter import Parameter
+    from qiskit.circuit.gate import Gate


All of them leads to cyclic-import?

Only from qiskit.transpiler.target import Target causes the cyclic-import problem.
But, since these imports are only used in convert_to_target I have kept it into this function only.
This is because, had I put it outside(on the top) it would cause the same cyclic-import in the future if any other function finds a way into this file which could use imports that cause cyclic-import problems, so the best solution here is to do imports at the site of usage! :)
I would change these if you say so :)

to24toro

Thank you for your PR and sorry for late response.
I think that it is almost well done.

My suggestions consist of

IBMQubitProperties
Careful discussion is needed before adding the new class. If possible, we would like to avoid adding it.
Input consistency between the new convert_to_target and the legacy one.
It would be better to keep inputs as they are. If we want to drop it, we can do that after warning users in the next release.

qiskit/providers/backend_compat.py

to24toro · 2023-11-03T08:37:46Z

@nkanazawa1989
I would appreciate your comment if my suggestion and comment is not right.

coveralls · 2023-11-05T16:21:32Z

Pull Request Test Coverage Report for Build 7140509075

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

87 of 97 (89.69%) changed or added relevant lines in 1 file are covered.
1550 unchanged lines in 120 files lost coverage.
Overall coverage increased (+0.7%) to 87.538%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
qiskit/providers/backend_compat.py	87	97	89.69%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/sabre_layout.rs	1	98.31%
crates/accelerate/src/stochastic_swap.rs	1	87.96%
crates/qasm2/src/error.rs	1	97.56%
crates/qasm2/src/lex.rs	1	92.17%
qiskit/circuit/classicalregister.py	1	90.48%
qiskit/circuit/classical/types/types.py	1	96.43%
qiskit/circuit/controlflow/control_flow.py	1	95.0%
qiskit/circuit/controlflow/for_loop.py	1	98.55%
qiskit/circuit/controlflow/while_loop.py	1	98.18%
qiskit/circuit/library/generalized_gates/pauli.py	1	91.18%

Totals
Change from base Build 6615597584:	0.7%
Covered Lines:	59761
Relevant Lines:	68269

💛 - Coveralls

…emoved source code for convert_to_target_legacy function from backend_compat.py.

qiskit-bot · 2023-11-08T10:42:20Z

Thank you for opening a new pull request.

Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient.

While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone.

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

@Qiskit/terra-core
@nkanazawa1989

MozammilQ · 2023-11-08T10:43:17Z

@to24toro , please see if this is ok :)

MozammilQ · 2023-11-08T10:44:49Z

@nkanazawa1989 I would appreciate your comment if my suggestion and comment is not right.

@nkanazawa1989

nkanazawa1989

Thanks @MozammilQ ! We wanted to unify the machinery for backend model up-conversion. So far we have been maintaining only one in the IBM Provider, and current Qiskit code is bit fragile. This PR will make the up-conversion logic more robust. Probably @mtreinish also wants to take a look?

This is nitpick but the scope of this PR seems wider than what the PR title describes. To avoid confusion in the commit log, I prefer other PR title such as "Update backend model up-conversion logic". You can write upgrade and fix in the release note, i.e. new logic provides better handling for control flow instructions.

qiskit/providers/backend_compat.py

MozammilQ · 2023-11-15T14:11:10Z

@nkanazawa1989 , please see if this is good enough :)

to24toro · 2023-11-26T13:25:52Z

qiskit/providers/backend_compat.py

 def convert_to_target(
    configuration: BackendConfiguration,
-    properties: BackendProperties = None,
-    defaults: PulseDefaults = None,
+    properties: Optional[Union[BackendProperties, Dict]] = None,
+    defaults: Optional[Union[PulseDefaults, Dict]] = None,
    custom_name_mapping: Optional[Dict[str, Any]] = None,
-    add_delay: bool = False,
-    filter_faulty: bool = False,
+    add_delay: bool = True,
+    filter_faulty: bool = True,
 ):


@MozammilQ
I apologize for the delayed response.

I would like to make one comment.
The default value for this part(e.g. add_delay) of the input has been changed. Ideally, in the upcoming 0.46 version, the deprecate_aeg decorator should be added on this function, and the in 0.47 and beyond, these input should be changed from the default. However, this PR for the main branch signified the merge for the Qiskit 1.0, and we don’t allow API change for a year.
As a special case, we are considering allowing the direct modification of the default values for input without the decorator in this PR.
Therefore, after this PR merge, we would like you to submit a new PR to the 0.46 branch with the added decorator.

Thank you.

qiskit/providers/backend_compat.py

nkanazawa1989

Thanks @MozammilQ the PR looks almost good to me except for handling of calibration data. I'll approve once after the change is applied :)

qiskit/providers/backend_compat.py

MozammilQ · 2023-11-29T15:13:55Z

@nkanazawa1989 , please see if this is good enough :)

nkanazawa1989

Thanks @MozammilQ and @to24toro . Overall the PR looks good. I just added some suggestions to make code more readable and efficient. This doesn't block merge, so it's up to you. I'll approve if you want go as-is.

qiskit/providers/backend_compat.py

…pplied suggestions, reverted a visualization test to its original form

MozammilQ · 2023-12-08T13:20:09Z

@nkanazawa1989 , I have added the suggestions :) Please see, if this is good enough :)

nkanazawa1989

Thanks @MozammilQ for updating the conversion logic. New code seems more readable :)

mtreinish

I know that this already merged, I was just taking a quick look through after see it in the git log this morning. I had a few quick comments and questions inline nothing huge and things that are easy to fix in a follow up PR.

mtreinish · 2023-12-11T12:51:22Z

qiskit/providers/backend_compat.py

+                "Definition of instruction %s is not found in the Qiskit namespace and "
+                "GateConfig is not provided by the BackendConfiguration payload. "
+                "Qiskit Gate model cannot be instantiated for this instruction and "
+                "this instruction is silently excluded from the Target. "
+                "Please add new gate class to Qiskit or provide GateConfig for this name.",


This isn't 100% accurate, the input instruction could be in the Qiskit namespace, but it also could have been user provided with custom_name_mapping which allows end users to specify a custom name to gate object mapping as a dictionary input. Personally I'd have just warned with something like:

No gate definition for %s can be found and is being excluded from the generated target. You can use `custom_name_mapping` to provide a definition for this operation.

I probably would have used a RuntimeWarning or a UserWarning instead of logging a warning so it's a bit more visible.

mtreinish · 2023-12-11T12:51:42Z

qiskit/providers/backend_compat.py

+    add_delay: bool = True,
+    filter_faulty: bool = True,


This is an API change and should be documented in an upgrade note

mtreinish · 2023-12-11T12:52:10Z

qiskit/providers/backend_compat.py

+        add_delay: bool = True,
+        filter_faulty: bool = True,


This is an api change and should be documented in the release notes.

mtreinish · 2023-12-11T12:53:44Z

qiskit/providers/backend_compat.py

+    .. note::
+        Passing in legacy objects like BackendProperties as properties and PulseDefaults
+        as defaults will be deprecated in the future.


I would remove this from the docstring, the point at which we no longer support BackendProperties and PulseDefaults this converter function can be removed from qiskit. We're not at the point of deprecating the legacy interface though.

Fair enough

mtreinish · 2023-12-11T12:56:51Z

qiskit/providers/backend_compat.py

+        elif name in gate_configs:
+            this_config = gate_configs[name]
+            params = list(map(Parameter, getattr(this_config, "parameters", [])))
+            coupling_map = getattr(this_config, "coupling_map", [])
+            inst_name_map[name] = Gate(
+                name=name,
+                num_qubits=len(coupling_map[0]) if coupling_map else 0,
+                params=params,
+            )


Is the assumption here that if a gate has some details definition in the configuration payload that something like PulseDefaults will provide a calibration for it? My first thought here was that adding an opaque gate definition to the target is of little value because nothing will be able to generate a bespoke gate without a definition and people won't know what to do with it.

Yes, you're absolutely right. But this is what exactly GateConfig model provides. In my understanding this just translates some opcode into Qiskit gate model, where the opcode (e.g. Qasm) doesn't have quantum definition and no optimization is necessary in quantum domain.

mtreinish · 2023-12-11T12:59:19Z

qiskit/providers/backend_compat.py

-            qubits = tuple(gate.qubits)
-            if filter_faulty:
-                if any(not properties.is_qubit_operational(qubit) for qubit in qubits):
+            faulty_qubits = properties.faulty_qubits()


Using a list here causes the faulty qubit check below to be O(n^2) is there a reason you changed this to a list?

mtreinish · 2023-12-11T13:02:00Z

qiskit/providers/backend_compat.py

-                if any(not properties.is_qubit_operational(qubit) for qubit in qubits):
+            faulty_qubits = properties.faulty_qubits()
+
+        for name in prop_name_map.keys():


In general you don't need to use keys() like this because the iter(dict) will iterate over the keys already and this just creates a temporary object.

mtreinish · 2023-12-11T13:04:03Z

qiskit/providers/backend_compat.py

+                in_param = {
+                    "error": params["gate_error"][0] if "gate_error" in params else None,
+                    "duration": params["gate_length"][0] if "gate_length" in params else None,
+                }
+                inst_prop = InstructionProperties(**in_param)


Is there a reason you need to use an intermediate dictionary here? Wouldn't it create one less object if you just did:

Suggested change

in_param = {

"error": params["gate_error"][0] if "gate_error" in params else None,

"duration": params["gate_length"][0] if "gate_length" in params else None,

}

inst_prop = InstructionProperties(**in_param)

error = params["gate_error"][0] if "gate_error" in params else None

duration = params["gate_length"][0] if "gate_length" in params else None

inst_prop = InstructionProperties(error=error, duration=duration)

yeah this is copied from provider which I wrote where I needed to apply schema. This code is not really inefficient as you say.

nkanazawa1989 · 2023-12-12T15:18:56Z

Let me take a look (seems like this also introduced a bug)

* This marks an initiative to replace the contents of method convert_to_target from qiskit.providers.backend_compat.py with contents of method target_from_server_data from qiskit_ibm_provider/utils/json_decoder.py * removed usage of supported_instructions from conf_*.json * fixed lint * added test, to check if the issue has been fixed * added release note * added suggestions and refactored code * removed argument deprecation, removed covert_to_target_legacy * refactor code * Added working add_delay argument in convert_to_target function, and removed source code for convert_to_target_legacy function from backend_compat.py. * refactor code * fixed lint * changed a test * altered release note * applied suggestions * add test * refactor code * altered handling of calibration data * docs * fix docs error * lint * refactor code * refactor code(removed inefficiencies and improved code legibility), applied suggestions, reverted a visualization test to its original form

* Followup - Add exception handling for the edge case in which a basis gate property is not reported - Cleanup docs - Replace logging with RuntimeWarning - Add more inline comments - Fix wrong typehints - Update handling of faulty qubits with set operation * bugfix + more warning message * Update reno * Add more check for filter option

This marks an initiative to replace the contents of method convert_to…

255e90b

…_target from qiskit.providers.backend_compat.py with contents of method target_from_server_data from qiskit_ibm_provider/utils/json_decoder.py

qiskit-bot added the Community PR PRs from contributors that are not 'members' of the Qiskit repo label Oct 23, 2023

MozammilQ added 3 commits October 24, 2023 17:21

removed usage of supported_instructions from conf_*.json

0dea19a

fixed lint

0fee0f7

added test, to check if the issue has been fixed

b64c91c

MozammilQ changed the title ~~[WIP]Replace the contents of convert_to_target() with target_from_server_data()~~ Fixing return of improper schedule by backend.instruction_schedule_map.get("measure", [0]) Oct 25, 2023

MozammilQ marked this pull request as ready for review October 25, 2023 11:57

MozammilQ requested review from eggerdj, wshanks, a team and jyu00 as code owners October 25, 2023 11:57

added release note

aa3f2eb

to24toro reviewed Nov 3, 2023

View reviewed changes

to24toro suggested changes Nov 3, 2023

View reviewed changes

added suggestions and refactored code

f43903c

MozammilQ marked this pull request as draft November 5, 2023 17:50

MozammilQ added 3 commits November 5, 2023 18:40

removed argument deprecation, removed covert_to_target_legacy

4062aa1

refactor code

f4f255a

Added working add_delay argument in convert_to_target function, and r…

e2c7594

…emoved source code for convert_to_target_legacy function from backend_compat.py.

MozammilQ marked this pull request as ready for review November 8, 2023 10:42

nkanazawa1989 requested changes Nov 13, 2023

View reviewed changes

refactor code

0bf026c

MozammilQ changed the title ~~Fixing return of improper schedule by backend.instruction_schedule_map.get("measure", [0])~~ Update backend model up-conversion logic Nov 14, 2023

MozammilQ added 3 commits November 14, 2023 14:53

altered release note

d2d206d

applied suggestions

cc924ae

add test

c8241c8

to24toro reviewed Nov 26, 2023

View reviewed changes

refactor code

692f4d2

to24toro reviewed Nov 27, 2023

View reviewed changes

qiskit/providers/backend_compat.py Outdated Show resolved Hide resolved

nkanazawa1989 reviewed Nov 27, 2023

View reviewed changes

qiskit/providers/backend_compat.py Outdated Show resolved Hide resolved

MozammilQ added 4 commits November 29, 2023 10:15

altered handling of calibration data

7df4558

docs

33882eb

fix docs error

421099c

lint

ce99505

refactor code

9d515bf

nkanazawa1989 reviewed Dec 8, 2023

View reviewed changes

qiskit/providers/backend_compat.py Outdated Show resolved Hide resolved

qiskit/providers/backend_compat.py Outdated Show resolved Hide resolved

qiskit/providers/backend_compat.py Outdated Show resolved Hide resolved

qiskit/providers/backend_compat.py Show resolved Hide resolved

refactor code(removed inefficiencies and improved code legibility), a…

4c73a21

…pplied suggestions, reverted a visualization test to its original form

to24toro approved these changes Dec 11, 2023

View reviewed changes

nkanazawa1989 approved these changes Dec 11, 2023

View reviewed changes

nkanazawa1989 added this pull request to the merge queue Dec 11, 2023

Merged via the queue into Qiskit:main with commit 7a41140 Dec 11, 2023
13 checks passed

mtreinish reviewed Dec 11, 2023

View reviewed changes

nkanazawa1989 mentioned this pull request Dec 12, 2023

Followup of #11095 #11404

Merged

nkanazawa1989 mentioned this pull request Feb 26, 2024

Bugfix for missing qubit property #11880

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update backend model up-conversion logic #11095

Update backend model up-conversion logic #11095

MozammilQ commented Oct 23, 2023 •

edited

Loading

qiskit-bot commented Oct 25, 2023

MozammilQ commented Oct 25, 2023

to24toro Nov 3, 2023

MozammilQ Nov 8, 2023 •

edited

Loading

to24toro Nov 3, 2023

MozammilQ Nov 4, 2023

to24toro left a comment

to24toro commented Nov 3, 2023

coveralls commented Nov 5, 2023 •

edited

Loading

qiskit-bot commented Nov 8, 2023

MozammilQ commented Nov 8, 2023 •

edited

Loading

MozammilQ commented Nov 8, 2023

nkanazawa1989 left a comment

MozammilQ commented Nov 15, 2023

to24toro Nov 26, 2023 •

edited

Loading

nkanazawa1989 left a comment

MozammilQ commented Nov 29, 2023

nkanazawa1989 left a comment

MozammilQ commented Dec 8, 2023

nkanazawa1989 left a comment

mtreinish left a comment

mtreinish Dec 11, 2023

mtreinish Dec 11, 2023

mtreinish Dec 11, 2023

mtreinish Dec 11, 2023

nkanazawa1989 Dec 12, 2023

mtreinish Dec 11, 2023

nkanazawa1989 Dec 12, 2023

mtreinish Dec 11, 2023

mtreinish Dec 11, 2023

mtreinish Dec 11, 2023

nkanazawa1989 Dec 12, 2023

nkanazawa1989 commented Dec 12, 2023

Update backend model up-conversion logic #11095

Update backend model up-conversion logic #11095

Conversation

MozammilQ commented Oct 23, 2023 • edited Loading

Summary

Details and comments

qiskit-bot commented Oct 25, 2023

MozammilQ commented Oct 25, 2023

Choose a reason for hiding this comment

MozammilQ Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

to24toro left a comment

Choose a reason for hiding this comment

to24toro commented Nov 3, 2023

coveralls commented Nov 5, 2023 • edited Loading

Pull Request Test Coverage Report for Build 7140509075

Warning: This coverage report may be inaccurate.

💛 - Coveralls

qiskit-bot commented Nov 8, 2023

MozammilQ commented Nov 8, 2023 • edited Loading

MozammilQ commented Nov 8, 2023

nkanazawa1989 left a comment

Choose a reason for hiding this comment

MozammilQ commented Nov 15, 2023

to24toro Nov 26, 2023 • edited Loading

Choose a reason for hiding this comment

nkanazawa1989 left a comment

Choose a reason for hiding this comment

MozammilQ commented Nov 29, 2023

nkanazawa1989 left a comment

Choose a reason for hiding this comment

MozammilQ commented Dec 8, 2023

nkanazawa1989 left a comment

Choose a reason for hiding this comment

mtreinish 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkanazawa1989 commented Dec 12, 2023

MozammilQ commented Oct 23, 2023 •

edited

Loading

MozammilQ Nov 8, 2023 •

edited

Loading

coveralls commented Nov 5, 2023 •

edited

Loading

MozammilQ commented Nov 8, 2023 •

edited

Loading

to24toro Nov 26, 2023 •

edited

Loading