Experiment run hook methods

[nkanazawa1989]

Summary

This is the replacement of #358 . After the long discussion, we came up with the conclusion

• We prefer as-simple-as possible implementation
  • add explicit hook methods to transpile and analysis
  • no externally defined routine should be used (i.e. no pass-manager type approach)

Details and comments

This PR adds

• _post_transpile_hook
• _post_analysis_hook
• _pre_transpile_hook

along with

• run_transpile

as a counterpart of run_analysis method.

The _pre_transpile_hook is necessary to implement delay reschedule (i.e. alignment) configuration for T1 type experiment because this needs to update the transpile configs based on the given backend type. Currently it is implemented in the base class, however, when an experiment doesn't involve explicit delays, this pass results in the performance regression because it lets the transpiler regenerate the DAG circuit for scheduling. This will induce significant performance regression in the QV experiment. In this PR, the alignment configuration becomes a part of T1 type experiments with the pre hook.

Composite Experiment

In the current implementation, we assume nested experiments should have the same transpile configurations and composite experiments first combine logical circuits and then transpile them. However, since this PR adds transpiler hooks, we should transpile the circuit experiment-wise, then combine. Composite experiments are updated in this way. A new abstract method _flatten_circuits are introduced to control merger of transpiled circuits.

Note that the circuits method of composite experiments raises an error because we cannot generate logical circuit without transpiling it (we cannot partly transpile a circuit with different configurations). Indeed, from a viewpoint of definition, this is not quite strange because composite experiment itself doesn't generate any experimental circuits, but it rather just combine circuits generated by nested experiments.

[eggerdj]

I prefer this to the previous PR: it is a lot easier to see the flow of the experiment.

[eggerdj]

Same comment as for T1.

[eggerdj]

One extra thing that dawned on me which I like with this PR is that it introduces run_transpile a function that users can call so you can see the transpiled circuits that the experiment will run.

[wshanks]

This looks good to me and provides the hook we would like to use in #251. I'll hold off from approving because someone familiar with the composite experiment classes should carefully review those changes.

I wonder if the signatures to the hook methods are missing anything. I don't think so. A lot of the possible inputs come through the options rather than function arguments any way. _post_analysis could return the experiment data but it's mutable any way and already in the calling function so there is not much point.

Similar to Daniel's in line comments, I think it might be nice to suggest a use case for each hook in the base class method docstrings (pre_transpile: set backend-dependent transpile options, post_transpile: perform analysis of transpiled circuits to add more metadata to the circuits, post_analysis: perform additional operations based on analysis results such as updating calibration parameters).

[yaelbh]

Overall it's good, my comments are not big

[yaelbh]

Please transfer this code as a function in some utils file, which returns transpile options for circuits with delay gates. Then call this function from the pre_transpile_hook methods of T1 and T2Ramsey.

[nkanazawa1989]

This is bit conflicting with #358 (comment)

It splits up the logic of BaseExperiment and moves it into several different files

though @eliarbel suggested to allow internal callbacks #358 (comment). What do you think?

[yaelbh]

I'm thinking of something much simpler than these two comments. I don't want to change the design or anything, just to put the code that repeats in a static function somewhere. We're about to have additional experiments with delays, so it's important to have only one copy of these code lines. The functions pre_transpile_hook will still exist separately in T1 and in T2Ramsey, and they'll both call the static function.

[nkanazawa1989]

Okey, done in 26c6dd4

I also realized that most of backends started to report constraints, and now we can simplify the logic. Do you prefer to update in a separate PR? or update in this PR?

[yaelbh]

This check should still take place for the run options. Which makes me think, that CompositeExperiment.set_transpile_options should raise an error.

[nkanazawa1989]

Good point. You are 100% correct. I added warning instead of rising an error because giving an option doesn't collapse execution though given option doesn't have any effect. See 2abc6d8

[yaelbh]

Thanks. Did you also return test_parallel_options?

[nkanazawa1989]

Not exactly. The previous test assumes we cannot apply individual transpile options (this is true for only run options in this PR). So I added separate test that individually evaluate that we cannot apply composite transpile option. Please check this 4319d70

[nkanazawa1989]

(I also added reno to explain the difference of behavior)

[eliarbel]

Looks good Naoki! Plesae see my inline comments.
The changes in the composite classes are good, I wonder though if all of them are required for this PR or maybe there should be a separate PR for those? For example I'm not sure that the circuit flattening approach is an inherent change to this PR.

[eliarbel]

I think those calls should be at the run method level instead of within the run_transpile and run_analysis methods. This will keep the run_<transpile|analysis> methods more self contained and atomic, hence facilitating reuse and full customizability in the rare or advanced cases where the run method should be overriden and tweak the flow.
Furthermore, in a CompositeExperiment setting, do we always want the pre|post methods to be called for each invocation of a child experiment, or maybe only in certain cases or even not at all ? So I think that by moving the pre|post method calls to the run method we'll have full control of it.

[nkanazawa1989]

I don't assume any situation we need to customize run_transpile and run_analysis, and we should be able to control all experiments with pre and post methods. Having them in the run method, I found that we need to write almost the same run logic (but slightly different) for composite experiment, and I came up with integration into run_transpile / run_analysis (again, given these methods are not expected to be updated). If we allow child class to override them, indeed, we don't need to provide pre/post methods.

The another important point in my mind is

exp1 = Experiment1(**options1)
exp2 = Experiment2(**options2)

data1 = exp1.run(backend)
data2 = exp2.run(backend)

par_exp = ParallelExperiment(exp1, exp2)
par_data = par_exp.run(backend)

assert parallel_result.component_experiment_data(0) == data1 
assert parallel_result.component_experiment_data(1) == data2

This example means a composite experiment is just a macro that combines experiments for efficient execution, and it should NOT implement own pre/post methods not to override the experimental configuration of sub experiments. I think the composite experiment should always call pre/post methods of nested experiments.

[eliarbel]

I think it's better to have this interface instead:

 def run_transpile(self, circuits, backend: Backend, **options) -> List[QuantumCircuit]: 

That is to have the the circuits generation phase called explicitly in the run method and have its results passed to run_transpile. Better from single responsibility and modularity principle

[nkanazawa1989]

This causes a problem in CompositeExperiment.run_transpile. We need to apply transpile options and pre/post method for each experiment, thus circuits here will be List[List[QuantumCircuits]] for CompositeExperiment while it will be List[QuantumCircuit] for BaseExperiment. This is why circuit generation is not done in the run method, because we cannot combine un-transpiled circuits as currently implemented in the composite experiments (current logic doesn't support mixture of different experiments).

Another advantage of this signature would be #380 (comment). User can easily check what will be executed. If we assume circuit is always generated by run method, the run_transpile should be a protected method.

[nkanazawa1989]

According to your comment, probably run_transpile is not correct name for this function. Something like run_circuit_generation would make more sense?

[eliarbel]

Why not keeping the name circuits ? What if we'll have a new type of a composite experiment subclass in which we the circuit generation is not really flattening or concatenation ? Maybe some variational algorithm for example. I think circuits is a proper general term. Just a thought...

[nkanazawa1989]

Oh this is due to poor gitdiff. The circuits method still exists but not used because we always get circuit from nested experiments.
https://github.com/Qiskit/qiskit-experiments/blob/d5ca18d207a6deb6ed690b6ebdc68c2ed8b5246b/qiskit_experiments/framework/composite/composite_experiment.py#L130-L143

Actually this is one of my concerns in this PR. Previously, this circuits method provides a logic to combine circuits without transpiling. However, this doesn't allow us to combine different types of experiments, e.g. RB and calibration, due to different transpilation behavior.

I think an inner-loop of a variational algorithm can be implemented as a BaseExperiment rather than composite. The loop type experiment should be different base class because these base classes don't implements a logic for iteration.

[nkanazawa1989]

Thanks everyone for pretty helpful feedback. Especially for the composite experiments. The motivation to have update for them in this PR is to check consistency of new API between a single and composite experiment, though we can (in principle) separate the PR. Indeed, the update made for the BaseExperiment is still valid for CompositeExperiment, and I think this is one of correct approaches to implement flexible execution logic.

Please resolve comments if you are happy with update or my reply. I'm happy to carefully continue the discussion since this is very important PR which may influence future scalability of this tool.

[yaelbh]

I'm thinking of something much simpler than these two comments. I don't want to change the design or anything, just to put the code that repeats in a static function somewhere. We're about to have additional experiments with delays, so it's important to have only one copy of these code lines. The functions pre_transpile_hook will still exist separately in T1 and in T2Ramsey, and they'll both call the static function.

[yaelbh]

Now that you've copied this check to CompositeAnalysis, you can either remove it from here, or keep it here but in this case modify the error message, replace CompositeExperimentData with self.__experiment_data__.__name__, and similarly also for CompositeAnalysis.

[nkanazawa1989]

Thanks. You're right. The first one in composite experiment can be removed. Also message is updated. d56612c

[yaelbh]

Thanks. Did you also return test_parallel_options?

[chriseclectic]

Overall this functionality looks good, but I don't like adding so many extra public API methods to the base class. It feels like most of these should be internal methods. A couple of other comments:

• the pre_transpile function seems unnecessary and i think anything that goes here could just go in the subclass circuits method
• I think what you call run_transpile in this PR should probably just be the circuits method of the base experiment, and what is currently called circuits can be made an internal abstract method liek _circuits that subclasses implement.
• The callback for the experiment data job should remain in the run method, not be moved into the run_analysis method. We should re-work how this is implemented so calling run_analysis on an existing experiment data can also use the callback to wait for the jobs to finish.
• There are changes to composite experiment that seem quite separate to the rest of this PR and should probably be moved to a separate PR for independent review.

[chriseclectic]

Why is this method needed? I don't see why this is necessary and can't just be done as part of the circuits (or _circuits) method that experiments already have to implement.

[nkanazawa1989]

Yes, that is true. However this might be useful to write calibration. Because pulse gate pass allows us to automatically attach calibration with instruction schedule map, we can create the instmap and set this to transpiler options in the pre_transpile_action method. Then, we can avoid having two programing models in the same circuits method, i.e. creating schedule and circuit in the same place seems very complicated to novice users. What do you think @eggerdj ?

[eggerdj]

I'd like to move towards a seamless and easy schedule management in experiments. Currently, in experiments like Rabi, FineAmp, and Drag we have the schedule which is attached to a particular gate. However, the experiment circuits often feature other gates (e.g. a y-rotation to map a z-rotation error to a qubit population, etc.). This means that to get a self-consistent calibration framework we need to

• Either add the different schedules manually through options which is what we are currently doing. This is unscalable, complicated, and messy.
• Find an automatic way to attach the schedules to the circuits. Fortunately for us Terra 0.19.0 will allow us to do this with the instruction schedule map in an automatic way through a transpiller pass (thanks to Naoki).

This may justify having a pre_transpile_action. I have the following question: Could this transpiller pass be done in the current call to the transpiler with an option inst_map=inst_map_from_cals? If so I'm not sure we need pre_transpile_action if not then we need a pre_transpile_action.

[nkanazawa1989]

Yes, this can be done from everywhere you can access to self, i.e. self.set_transpile_options(inst_map=inst_map_from_cals). However, this should be called after you get backend, and you run the transpile. Alternative approach would be write something like

class BaseCalibration:
    def circuits(self, backend):
        inst_map_from_cals = self.calibration_options().calibration.get_instmap(...)
        self.set_transpile_options(inst_map=inst_map_from_cals)
        ...

class MyCalExp(BaseCalibration):
    def circuits(self, backend):
        circus = super().circuits(backend)
        ....

this is equivalent to add pre_transpile_action. So this is mainly readability issue.

[nkanazawa1989]

i.e. separation of experiment program and dynamic configuration.

[eggerdj]

I'd like to keep the circuits method clean of any calibration related variables. The following would be nice:

class BaseCalibrationExperiment(BaseExperiment, ABC):  # as per PR 251

    ...
    
    def pre_transpile_action():
        """Set the instruction schedule map for transpiling."""
        self.set_transpile_options(inst_map = self._cals.default_inst_map)

alternatively, without the pre_transpile_action hook we could probably do:

class BaseCalibrationExperiment(BaseExperiment, ABC):  # as per PR 251

    def __init__(self, ..., calibrations=cals):
        ...
        self.set_transpile_options(inst_map=cals.default_inst_map)

would this second option work? I think it should be okay as default_inst_map is a property and is not deep copied around so if I run my calibration experiment twice the update of the cals (and therefore the default_inst_map) should also happen. For a regular experiment that does not do any schedule management would the following workflow be sufficient?

exp = MyCoolExperiment(qubits, ...)
exp.set_transpile_options(inst_map=my_backend.defaults().instruction_schedule_map)
exp.run(my_backend)

[chriseclectic]

Is there any reason to have this function instead of just having a circuits function that returns the transpiled circuit. You could change the existing circuit function to be _circuits, and then have the public circuit function take a backend as argument and function as:

def _circuits(self, backend=None):
    # equivalent to existing circuit method for current experiments

def circuits(self, backend=None, transpile=True, **options):
     circuits = self._circuits(backend)
     if transpile:
         transpile_options = ...
         circuits = transpile(self._circuits(backend), backend, **transpile_options)
         self._post_transpile_action(circuits, backend)
     return circuits

Maybe you don't need the transpile kwarg and can just always transpile

[nkanazawa1989]

I like this approach. However, this always returns transpiled circuit, i.e. even single qubit experiment returns full qubit circuits. Sometime this make it difficult to understand what is happening in the experiment.

[chriseclectic]

As mentioned above this method seems unnecessary and can just be done in circuits and seems to have no major difference in this functions execution flow

[nkanazawa1989]

Right, but this is mainly for readability improvements. See my comments above :)

[chriseclectic]

This gives this function a confusing signature, if i call it with data and a job it is going to try and add that job to the data, but adding job data to experiment data should probably always be done by the run method, not the run_analysis method.

[nkanazawa1989]

This is fixed in bf34b77

[chriseclectic]

Suggested change

	self.post_analysis_action(experiment_data)
	self._post_analysis_action(experiment_data)

If the callback is handled in the run method as was originally done instead of being moved to this method, then calling post_analysis_action will automatically be run as part of the run analysis callback when doing experiment.run.

Maybe we could rework how the experiment data callback works so that run_analysis could be added as a callback to an existing container if its still running the experiment.

[nkanazawa1989]

The motivation of including hooks in this method is to reduce maintenance overhead in the composite experiment. If we call hooks directly in the run method, we need to override entire run method in the composite experiment. In this implementation, composite just need to update run_transpile and (optionally) run_analysis method.

[nkanazawa1989]

Done in bf34b77 (personally I don't prefer this cyclic reference, i.e. experiment.analysis.run -> experiment_data -> experiment_data.experiment)

[chriseclectic]

Does this need to be in framework? It really feels like all the content of this function is something that should be handled correctly by terra transpiler/scheduler.

[nkanazawa1989]

Half of this code is now automatically handled by transpiler since backends recently started to report the timing constraints information. However, we still need to set scheduling options, i.e. transpiler options of scheduling_method. This can be a default transpiler options of corresponding experiments, however, a fake backend doesn't require scheduling since it implicitly calls simulator and no timing constraints there. But we can still set scheduling option for simulator backend at the expense of transpiler overhead (this is heavy compute pass since it recreates DAG circuits).

[nkanazawa1989]

I tried not to change currently implemented code, but I can also update that code in this PR. There are two options

• Keep this pre-transpile. Remove the code to set acquire_alignment.
• Entirely remove this code and set "scheduling_method": "asap" to default transpiler options. This will induce non-necessary overhead in the simulator execution.

[chriseclectic]

This added docs should probably be moved into module level docs somewhere since they are very long for class docs

[nkanazawa1989]

I guess this will not be shown in api docs if we move this to class docs. This contains useful information about how circuits are combined, and I think this is worth showing.

[nkanazawa1989]

Thanks Chris, I turned hooks into protected members. Regarding the separation of PR, I don't think I can justify the reason to integrate pre/post transpile actions into run_transpile without showing how composite works.

[nkanazawa1989]

Based on feedback from @chriseclectic and @eliarbel , I open new PR #404 that is a part of this PR without composite experiment upgrade. I'll turn this PR into draft until #404 is merged.