Add analysis callback to ExperimentData

[chriseclectic]

Summary

Adds add_analysis_callback to DbExperimentData that allows running analysis functions as a done callback to the running job futures, or as a separate future if no jobs are running. This separate the processes for job execution and analysis post-processing and allows multiple post-processing functions to be added and run in order, but still collectively as an asynchronous background processes.

Details and comments

Depends on #402

[jyu00]

I think this would work. However, a maybe simpler solution is to continue using add_done_callback() but wrap the real callback function with a helper that 1) indicate post processing is still running (e.g. a dictionary of callback id & status) so status() reports the true status and 2) catch and save any exception. Since you already wrote this code, it's up to you whether you want to keep it.

[chriseclectic]

@jyu00 I've updated PR to go back to using a mix of job_done_callback if there is already a job futures. Or to create a new analysis future if there are no job futures so analysis added through callback still runs async (and then adding any additional analysis callbacks will get added as job done callbacks to that analysis future so they should still run in order they were added).

[jyu00]

Other than some minor comments, I think this looks good. Although you might want to add at least 1 test on adding multiple callback functions.

[jyu00]

There seems to be a bug in the old code. How would the caller know the wait timed out if all exceptions are suppressed lol

[chriseclectic]

I never quite understood why this was in the code! So should the with suppress lines be removed then?

[nkanazawa1989]

Probably IntEnum (this is not what you implemented though)?

[nkanazawa1989]

Seems this is bit problematic from usability point of view. When user adds one more scan point and it triggers job splitting, it suddenly increases a chance to get timeout error though jobs are executed at normal speed. The problem here is splitting is performed implicitly.

[chriseclectic]

That's the point though. If you set timeout for 60 seconds when calling block for results, you would expect it to time out in 60 seconds. Previously if you have a bunch of jobs and analysis callbacks the timeout before would have been 60 seconds for each jobs/callback, so potentially much longer than 60 seconds total.

[nkanazawa1989]

If only one job left and it reaches timeout, it is hard for most of users to resubmit last one of automatically split job. Also it is difficult to predict number of split jobs (one needs to check the count of .circuits and backend max experiments before execution, to scale timeout value). Probably many people don't use timeout feature (it's rarely explained in the tutorial/textbook), so this will not quite hurt usability.

[wshanks]

I was wondering how this would interact with #251. There BaseExperiment.run() is overridden like:

https://github.com/Qiskit/qiskit-experiments/blob/fe9b24451d2248d128adc6fb156e7ffe8f93874a/qiskit_experiments/calibration_management/base_calibration_experiment.py#L336-L361

With add_analysis_callback, this could be refactored to:

    def run(self, ...):
         experiment_data = super().run(backend, analysis, experiment_data, **run_options) 
  
         if self.auto_update:
             experiment_data.add_analysis_callback(self.update_calibrations)
  
         return experiment_data

So the block_for_results() can be dropped (which opens up other issues but not ones related to this PR) but run() still needs to be overridden because there is no other way to add analysis callbacks after the default one.

Is overriding run the intended usage, or should there be another way for experiments to add the analysis chain? Or is add_analysis_callback intended for higher level code that calls run() and then calls add_analysis_callback itself?

[yaelbh]

I'm not following what's happening in this PR, but, working on #364, I noticed that things that are done with experiment data don't always work with composite experiment data. #364 contains tests for composite experiment data, that are currently missing. Since this PR has "ExperimentData" in its title, please verify that it doesn't break anything with composite experiment data.

[chriseclectic]

@wshanks Either run could be overridden like you suggest, or run_analysis, or we could build some standard post-analysis hook into the base class such as the approach in #404 (which might need some slight refactoring to use these callbacks), though maybe there is little need for the hooks if the run / run_analysis override is minimal.

With the callback the update will always run after jobs and analysis without needing to block. Though I think it would likely be better if the calibration update method could update a separate calibration container passed as an argument so same container could be used in multiple experiments. We would need to make that container thread safe like the data structures used in DbExperimentData so only one experiment can update it at a time (This is stuff that is probably better discussed in #251)

[nkanazawa1989]

What is the status of this PR? I think this is good to go in. Regarding what @yaelbh suggested, I think this doesn't break any composite workflow because, in the composite, this will just add a run_analysis of base class to the thread safe dictionary, then each nested analysis will be separately called in the _run_analysis method of composite analysis. At this moment, the data is no longer future object (I'm not 100% sure).