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

Scheduling stage doesn't use custom dt when target provided #11994

Closed
ElePT opened this issue Mar 12, 2024 · 9 comments
Closed

Scheduling stage doesn't use custom dt when target provided #11994

ElePT opened this issue Mar 12, 2024 · 9 comments
Labels
bug Something isn't working

Comments

@ElePT
Copy link
Contributor

ElePT commented Mar 12, 2024

Environment

  • Qiskit version: 1.1.0
  • Python version: 3.10.0
  • Operating system: MacOS

What is happening?

The expected behavior of transpile when a custom dt is specified (i.e. sth like transpile(qc, backend=b, scheduling_method="asap", dt=dt / 2) is that the scheduling stage will use this custom dt instead of the backend's one, but this behavior is not respected if the input backend contains a target. I believe that the root of the issue is that dt is encoded in the inst_durations object that is passed around, but the TimeUnitConversion transformation pass is designed to overwrite inst_durations if a target is specified, so the custom dt never gets to be used:

# TimeUnitConversion init
self.inst_durations = inst_durations or InstructionDurations()
if target is not None:
     self.inst_durations = target.durations()

In other words, the priorities are not consistent between the preset passmanager/transpile and the TimeUnitConversion pass (one prioritizes the custom durations, the other prioritizes the target).

How can we reproduce the issue?

This snippet works for the V1 backend and doesn't for the V2 backend:

from qiskit import QuantumCircuit
from qiskit import transpile
from qiskit_ibm_runtime.fake_provider import FakeHanoi, FakeHanoiV2

backend_v1 = FakeHanoi()
backend_v2 = FakeHanoiV2()
dt = 2.2222222222222221e-10

# from test_change_dt_in_transpile
qc = QuantumCircuit(1, 1)
qc.x(0)
qc.measure(0, 0)

for b in [backend_v1, backend_v2]:
  # default case
  scheduled = transpile(qc, backend=b, scheduling_method="asap")
  org_duration = scheduled.duration

  # halve dt in sec = double duration in dt
  scheduled = transpile(
    qc, backend=b, scheduling_method="asap", dt=dt / 2
  )
  print(b, scheduled.duration == org_duration * 2)

What should happen?

The custom dt shouldn't be bypassed.

Any suggestions?

I think that this issue could be fixed if we establish a consistent priority of the custom inst_durations over the target's. A PR with a fix proposal will follow.

@jakelishman
Copy link
Member

The expected behavior of transpile when a custom dt is specified (i.e. sth like transpile(qc, backend=b, scheduling_method="asap", dt=dt / 2) is that the scheduling stage will use this custom dt instead of the backend's one, but this behavior is not respected if the input backend contains a target.

For most options, this is currently the expected behaviour: the Target supersedes all the other V1-based individual settings that transpile takes. Is there a way to encode the custom dt within the Target? If so, I think the most consistent path for the rest of the options will be to require that a new target is constructed with a modified dt.

(Fwiw, we should also make implicit ignoring of settings a user-facing warning for certain.)

@jakelishman
Copy link
Member

The usual argument for why the target supersedes the BackendV1-era properties is that the Target is a more general heterogeneous mix of information than the V1 era, and it's not at all clear how to override just one component of V1 data in a Target in general (see #11405 (comment)). That might not apply the same way here, in which case it may well make more sense to allow a specific override.

@ElePT
Copy link
Contributor Author

ElePT commented Mar 12, 2024

@jakelishman Yes, we could easily make a copy of the target and change the dt attribute (and close #11995), but if we expect that as we move towards a fully target-based pipeline this transpile option is encoded in the target, does it make sense to keep it in the future? My assumption of "option takes precedence over target" came from explicit unit tests that tested this behavior (only for backend V1, which is what led to this confusion), and I think it's fairly intuitive. We could also just add a warning indicating that dt will be overridden by the target if both are provided.

@jakelishman
Copy link
Member

From an API perspective, I don't really like our current situation of many arguments to transpile getting ignored if you give a Target-based backend - overall, I'd generally rather the arguments did something well-defined - but I'm also concerned with consistency across the interface, and "Target takes priority except for a, b, and c" is also a problem to understand.

The "option takes precedence" stuff is from the BackendV1 era, where the backend-related options to transpile are precisely the configurable components of BackendV1, so there's a clear meaning to the precedence. That model doesn't map to Target, though, so we get problems. In the case of dt and the InstructionDurations, though, where I think the map is much more 1:1 from the argument to the state within Target, it definitely could make a lot more sense to allow those arguments to persist.

I'm not super certain one way or the other - I don't particularly love either situation, tbh. I think the transpiler interface should feel consistent, though, is I guess my overriding concern. Maybe us more aggressively warning on options that are being ignored will be enough to make it feel consistent if dt can be passed separately and does override things.

@ElePT
Copy link
Contributor Author

ElePT commented Mar 12, 2024

I see your point, I think that I will try to gather more context to raise the issue internally with the team, as long as the behavior is intentional and documented I think that there is no single answer. In the meantime, I have pushed 69ca591 to #11996 (which is the PR that led me to this issue in the first place) to encode these options in the converted target so that the behavior of transpile is maintained for both BackendV1 and BackendV2 inputs (so for V1 the options take precedence, for V2 the target takes precedence). I don't think this is a good final solution, but given that this conversion step is temporary (will disappear with backend V1), it's a non-blocking way to continue.

@jakelishman
Copy link
Member

Sorry, I'm not trying to be super difficult here, I'm mostly just trying to see what we can do to sort out our current interface problem. It could well be that the way you were already going is the right way, if we can clarify the story around why the rest of the options don't override Target but dt does, for users to understand.

@ElePT
Copy link
Contributor Author

ElePT commented Mar 12, 2024

Yes, I get it, and I just want to understand all the angles (beyond what is needed for #11996), so no worries at all. I think there's also the perspective of ergonomics, especially with instruction_durations (dt is very straightforward). If we want users to only specify custom durations through the target (unless there is no target), the only way I currently found to update them is manually iterating over the instruction properties (69ca591). Maybe we could extend the target class to make this manipulation easier? (or maybe I am missing an easier way to do it)

@1ucian0
Copy link
Member

1ucian0 commented Apr 1, 2024

Hi @ElePT , is this issue fixed via #12042 ?

@ElePT
Copy link
Contributor Author

ElePT commented Apr 2, 2024

Yes, this issue was fixed in #12042.

@ElePT ElePT closed this as completed Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants