Save experiment and analysis config as artifacts #1424

coruscating · 2024-03-18T17:25:52Z

Summary

Adds BaseExperiment.load and BaseExperiment.save methods and a helper function load_all() that loads both experiment and experiment data, with the option to run analysis:

from qiskit_experiments.framework import load_all
from qiskit_ibm_runtime import QiskitRuntimeService
service = QiskitRuntimeService(channel="ibm_quantum")

exp, exp_data = load_all("9e242995-d793-4940-920f-f3970d7f777b", provider=service, run_analysis=True)

This PR incorporates changes from @ItamarGoldman's in #1423. To enable saving and loading, this PR saves experiment and analysis config as experiment_config and analysis_config in experiment data artifacts. Because analysis_options shouldn't be added as a new artifact every time analysis is rerun, this also adds overwrite_name option to add_artifacts() and renames the current overwrite option to overwrite_id. have been incorporated.

To-do

add tests
add seeds used in final circuit generation
update rerun analysis and cloud database service documentation with usage examples

Notes

load_all() is inefficient because BaseExperiment.load() and ExperimentData.load() make separate and identical calls to the experiment service. Ideally this would be combined in a separate interface class in a future refactoring.

…data

Added load method to `BaseExperiment` Added backend initialization to the experiment if available. `from_config` now also loads analysis if available.

Co-authored-by: ItamarGoldman <itamargoldman@gmail.com>

yaelbh

It's only a partial review for now. I'm publishing it to emphasize some possible issues that I've spotted in BaseAnalysis.run.

I'm intentionally not checking the Request changes box, because I'm going on vacation in a few days.

yaelbh · 2024-04-07T08:39:41Z

qiskit_experiments/framework/configs.py


        Raises:
-            QiskitError: If the experiment class is not stored,
+            QiskitError: If the drawer class is not stored,


Replace "successful" with "successfully" in the subsequent line

yaelbh · 2024-04-07T08:46:24Z

qiskit_experiments/framework/configs.py

-    type is currently stored, or the :meth:`~.BaseExperiment.from_config` class method of the
-    appropriate experiment.
+    This stores the current configuration of a :class:`.BaseDrawer` and can be used to reconstruct
+    the drawer class using either the :meth:`drawer` property if the drawer class type is


Property of which class? And how is it related to whether the class type is stored?

yaelbh · 2024-04-07T09:02:50Z

qiskit_experiments/curve_analysis/base_curve_analysis.py

+    def config(self) -> AnalysisConfig:
+        """Return the config dataclass for this analysis. We replicate the method from `BaseAnalysis`
+        because we cannot directly modify the returned object. This limitation arises from our use of
+        it hashable we use `dataclasses.dataclass(frozen=True)`  to ensure its hashability.


The last sentence is grammatically incorrect (and indeed it's not clear what it's trying to say).

yaelbh · 2024-04-07T11:08:51Z

qiskit_experiments/framework/experiment_data.py

+                else:
+                    self._artifacts[artifact.artifact_id] = artifact
+            else:
+                self._artifacts[artifact.artifact_id] = artifact


The logic here is somewhat strange and unintuitive, I don't know if it's documented anywhere, and it's not clear where it comes from. From the code I see that multiple, say five calls to add_artifacts with overwrite_name=False will create a collection of five different artifacts having the same name. Until the sixth call sets overweite_name to True, in this case all the five existing artifacts with that name will remain in the collection, but will be overridden by the new artifact. However a sixth artifact with artifact_id will not be added.

yaelbh · 2024-04-07T11:18:05Z

qiskit_experiments/framework/base_analysis.py

-                        expdata.add_artifacts(result)
+                        if (
+                            result.name != "analysis_config"
+                        ):  # Not saving constituent analysis configs


What's the flow that will make _run_analysis return an analysis config?

yaelbh · 2024-04-07T11:23:19Z

qiskit_experiments/framework/base_analysis.py

+            ArtifactData(name="analysis_config", data=self.config()),
+            overwrite_name=True,
+            overwrite_id=True,
+        )


If replace_results is False then we create a copy of the ExperimentData object. Both the original object and its copy will now contain the analysis config artifact, although the original one will not contain the corresponding analysis result.

I'm also concerned about the old analysis config that's stored in the experiment data and is now overridden. The concern applies for the two possible values of replace_results.

yaelbh · 2024-04-07T12:04:02Z

qiskit_experiments/curve_analysis/base_curve_analysis.py

+        args = tuple(getattr(self, "__init_args__", OrderedDict()).values())
+        kwargs = dict(getattr(self, "__init_kwargs__", OrderedDict()))
+        # Only store non-default valued options
+        options = dict((key, getattr(self._options, key)) for key in self._set_options)


BaseAnalysis and BaseCurveAnalysis should call a common helper function, instead of duplicating the code. This is possible in spite of everything being hashable.

yaelbh · 2024-04-07T12:55:46Z

qiskit_experiments/framework/configs.py

+
+@dataclasses.dataclass(frozen=True)
+class PlotterConfig:
+    """Store configuration settings for a Drawer class.


Suggested change

"""Store configuration settings for a Drawer class.

"""Store configuration settings for a Plotter class.

yaelbh · 2024-04-07T12:56:12Z

qiskit_experiments/framework/configs.py

+    cls: type = None
+    options: Dict[str, Any] = dataclasses.field(default_factory=dict)
+    figure_options: Dict[str, Any] = dataclasses.field(default_factory=dict)
+    # drawer: Dict[str, Any] = dataclasses.field(default_factory=dict)


Suggested change

# drawer: Dict[str, Any] = dataclasses.field(default_factory=dict)

yaelbh · 2024-04-07T13:01:13Z

qiskit_experiments/framework/configs.py

+    """Store configuration settings for a Drawer class.
+
+    This stores the current configuration of a :class:`.BasePlotter` and can be used to reconstruct
+    the plotter class using either the :meth:`plotter` property if the plotter class type is


Same as before.

yaelbh · 2024-04-07T13:01:27Z

qiskit_experiments/framework/configs.py

+
+        Raises:
+            QiskitError: If the plotter class is not stored,
+                         was not successful deserialized, or reconstruction


Suggested change

was not successful deserialized, or reconstruction

was not successfully deserialized, or reconstruction

yaelbh · 2024-04-07T13:48:16Z

qiskit_experiments/framework/base_experiment.py

+        if not service:
+            raise QiskitError("A service must be provided to save the experiment.")
+        if not self.backend and not backend:
+            raise QiskitError("Backend must be set to save the experiment.")


Why is the backend mandatory?

yaelbh · 2024-04-07T13:54:53Z

qiskit_experiments/framework/base_experiment.py

+                        f"but you have versions {cur_versions}."
+                    )
+                try:
+                    reconstructed_experiment = experiment_config.cls.from_config(experiment_config)


Just out of curiosity, does the cls here have any effect? Or is it only for readability?

yaelbh · 2024-04-07T14:05:06Z

qiskit_experiments/framework/experiment_data.py

@@ -2404,7 +2409,7 @@ def copy(self, copy_results: bool = True) -> "ExperimentData":

        with self._artifacts.lock:
            new_instance._artifacts = ThreadSafeOrderedDict()
-            new_instance.add_artifacts(self._artifacts.values())
+            new_instance.add_artifacts(self._artifacts.values(), overwrite_id=True)


Why does it matter?

yaelbh · 2024-04-07T14:14:41Z

qiskit_experiments/visualization/drawers/base_drawer.py

@@ -552,27 +554,34 @@ def image(
    def figure(self):
        """Return figure object handler to be saved in the database."""

-    def config(self) -> Dict:
+    def config(self) -> DrawerConfig:


This is an interface change, do we have to follow some type of deprecation policy?

yaelbh · 2024-04-07T14:23:28Z

qiskit_experiments/visualization/plotters/base_plotter.py

-        if "figure_options" in value:
-            instance.set_figure_options(**value["figure_options"])
-        return instance
+        return cls.from_config(value)


Now that we have configs, do we still need __json_encode__ and __json_decode__?

If these are still required, with __json_encode__ now returning PlotterConfig and not dict, would that work?

eliarbel

Various questions and comments at this stage. I couldn't fully test it due to issues with BaseExperiment.load; once solved I'll run some manual tests

eliarbel · 2024-04-14T13:35:00Z

qiskit_experiments/visualization/plotters/base_plotter.py

@@ -553,36 +554,39 @@ def _configure_drawer(self):
        # Use drawer.set_figure_options so figure options are serialized.
        self.drawer.set_figure_options(**_drawer_figure_options)

-    def config(self) -> Dict:
+    def config(self) -> PlotterConfig:
        """Return the config dictionary for this drawing."""


Suggested change

"""Return the config dictionary for this drawing."""

"""Return the config dataclass for this plotter."""

eliarbel · 2024-04-14T13:36:01Z

qiskit_experiments/visualization/plotters/base_plotter.py

+
+    @classmethod
+    def from_config(cls, config: Union[PlotterConfig, Dict]) -> "BasePlotter":
+        """Initialize a plotter class from analysis config"""


Suggested change

"""Initialize a plotter class from analysis config"""

"""Initialize a plotter class from a plotter config"""

eliarbel · 2024-04-14T13:43:11Z

qiskit_experiments/visualization/drawers/base_drawer.py

@@ -552,27 +554,34 @@ def image(
    def figure(self):
        """Return figure object handler to be saved in the database."""

-    def config(self) -> Dict:
+    def config(self) -> DrawerConfig:
        """Return the config dictionary for this drawer."""


Suggested change

"""Return the config dictionary for this drawer."""

"""Return the config dataclass for this drawer."""

eliarbel · 2024-04-14T13:43:49Z

qiskit_experiments/visualization/drawers/base_drawer.py

-        }
+    @classmethod
+    def from_config(cls, config: Union[DrawerConfig, Dict]) -> "BaseDrawer":
+        """Initialize a drawer class from analysis config"""


Suggested change

"""Initialize a drawer class from analysis config"""

"""Initialize a drawer class from a drawer config"""

eliarbel · 2024-04-14T13:45:49Z

qiskit_experiments/visualization/drawers/base_drawer.py

+            ret.set_options(**config.options)
+        if config.figure_options:
+            ret.set_figure_options(**config.figure_options)
+        return ret

    def __json_encode__(self):
        return self.config()


With this now returning DrawerConfig and not a dict, would this work?

eliarbel · 2024-04-14T13:47:07Z

qiskit_experiments/visualization/plotters/base_plotter.py

-        if "figure_options" in value:
-            instance.set_figure_options(**value["figure_options"])
-        return instance
+        return cls.from_config(value)


If these are still required, with __json_encode__ now returning PlotterConfig and not dict, would that work?

eliarbel · 2024-04-14T18:05:27Z

qiskit_experiments/framework/helpers.py

+from qiskit_experiments.framework import BaseExperiment, ExperimentData
+
+
+def load_all(


I would change name to something a bit more informative, like load_experiment_and_data

eliarbel · 2024-04-14T18:35:04Z

qiskit_experiments/curve_analysis/base_curve_analysis.py

@@ -422,3 +424,36 @@ def _initialize(
                DeprecationWarning,
            )
            self.set_options(data_subfit_map=data_subfit_map)
+
+    def config(self) -> AnalysisConfig:


Why do we have config/from_config with plotter handling specifically here in BaseCurveAnalysis and not in BaseAnalysis? What about analyzes not derived from BaseCurveAnalysis, e.g. TomographyAnalysis or StarkP1SpectAnalysis?

eliarbel · 2024-04-14T20:07:30Z

qiskit_experiments/framework/base_experiment.py

+        data = service.experiment(experiment_id, json_decoder=ExperimentDecoder)
+
+        # Recreate artifacts
+        experiment_config_filename = "experiment_config.zip"


Better store the name in one place to be used here and in `ExperimentData.init" (maybe in helpers.py to avoid cyclic dependency)

eliarbel · 2024-04-16T11:26:08Z

qiskit_experiments/framework/base_experiment.py

@@ -171,13 +179,20 @@ def config(self) -> ExperimentConfig:
            (key, getattr(self._transpile_options, key)) for key in self._set_transpile_options
        )
        run_options = dict((key, getattr(self._run_options, key)) for key in self._set_run_options)
+


There is an issue with config handling when it comes to serializing a backend object when one is provided in the __init__ method. In this case, the backend is included in the config e.g. via kwargs and is serialized in way that upon deserialization (through BaseExperiment.load) the following warning is emitted:

/tmp/qiskit-experiments/qiskit_experiments/framework/json.py:581: UserWarning: Could not deserialize instance of class <class 'qiskit_ibm_runtime.ibm_backend.IBMBackend'> from settings {}. Value is {'class': <class 'qiskit_ibm_runtime.ibm_backend.IBMBackend'>, 'settings': {}, 'version': '0.23.0'}
The following exception was raised:
Traceback (most recent call last):
File "/tmp/qiskit-experiments/qiskit_experiments/framework/json.py", line 338, in _deserialize_object
return cls(**settings)
TypeError: IBMBackend.init() missing 3 required positional arguments: 'configuration', 'service', and 'api_client'

return _deserialize_object(obj_val)

The experiment still loads, but this warning is annoying.

eliarbel · 2024-04-16T11:35:20Z

qiskit_experiments/framework/base_experiment.py

+        cls,
+        experiment_id: str,
+        service: Optional[IBMExperimentService] = None,
+        provider: Optional[Provider] = None,


If service it not None, but provider is None, we get this warning:

/tmp/qiskit-experiments/qiskit_experiments/framework/base_experiment.py:286: UserWarning: Unable to retrieve backend.
warnings.warn("Unable to retrieve backend.")

I think this should be fixed - either don't allow an empty provider or update the doc string to explain the behavior when provider is empty. Currently the doc string explicitly says that IBMProvider is required

ItamarGoldman and others added 14 commits February 15, 2024 12:52

Update base_experiment.py

183c18b

Merge remote-tracking branch 'upstream/main' into IS-1392_versioning_…

378b306

…data

added load method to base experiment

343eed5

removed metadata env details

872ac46

added load method and load analysis config

c172cac

Added load method to `BaseExperiment` Added backend initialization to the experiment if available. `from_config` now also loads analysis if available.

added config and from_config method

20670a2

added config and from_config method

c0e3ec6

added config and from_config method

41bf46c

Added DrawerConfig and PlotterConfig dataclasses

ab2e162

removed comment

ef18ad9

assign plotter not trough set_options

3cb91bb

fixed error message for DrawerConfig

d095a82

save experiment and analysis options

bc4a29c

updates analysis options to config

0bd48ae

coruscating added this to the Release 0.7 milestone Mar 19, 2024

coruscating and others added 3 commits April 3, 2024 12:19

Merge branch 'versioning_data' into save-config-artifacts

31d73dd

Co-authored-by: ItamarGoldman <itamargoldman@gmail.com>

Merge remote-tracking branch 'upstream/main' into save-config-artifacts

6b128f0

add helper function and fixes

7975955

coruscating requested a review from eliarbel April 3, 2024 17:32

fix composite behavior and lint

15999a0

yaelbh reviewed Apr 7, 2024

View reviewed changes

eliarbel reviewed Apr 14, 2024

View reviewed changes

eliarbel reviewed Apr 16, 2024

View reviewed changes

coruscating mentioned this pull request Apr 22, 2024

Track changes in experiment circuits #1445

Open

added test for analysis config method

c2b4b5b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save experiment and analysis config as artifacts #1424

Save experiment and analysis config as artifacts #1424

coruscating commented Mar 18, 2024 •

edited

yaelbh left a comment

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

yaelbh Apr 7, 2024

eliarbel Apr 14, 2024

eliarbel left a comment

eliarbel Apr 14, 2024

eliarbel Apr 14, 2024

eliarbel Apr 14, 2024

eliarbel Apr 14, 2024

eliarbel Apr 14, 2024

eliarbel Apr 14, 2024

eliarbel Apr 14, 2024

eliarbel Apr 14, 2024

eliarbel Apr 14, 2024

eliarbel Apr 16, 2024

eliarbel Apr 16, 2024

	"""Store configuration settings for a Drawer class.
	"""Store configuration settings for a Plotter class.

	was not successful deserialized, or reconstruction
	was not successfully deserialized, or reconstruction

	"""Return the config dictionary for this drawing."""
	"""Return the config dataclass for this plotter."""

	"""Initialize a plotter class from analysis config"""
	"""Initialize a plotter class from a plotter config"""

	"""Initialize a drawer class from analysis config"""
	"""Initialize a drawer class from a drawer config"""

		from qiskit_experiments.framework import BaseExperiment, ExperimentData


		def load_all(

Save experiment and analysis config as artifacts #1424

Are you sure you want to change the base?

Save experiment and analysis config as artifacts #1424

Conversation

coruscating commented Mar 18, 2024 • edited

Summary

To-do

Notes

yaelbh 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

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

eliarbel 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

coruscating commented Mar 18, 2024 •

edited