Soft-deprecate combine_with_last_data arg, setting it as not overwrite_existing_data

Summary:
Diff contents:
* Soft-deprecates `combine_with_last_data` arg in `Experiment.attach_data` signature, docs, and logic.
* For all passthrough callsites, updates signature and docs.
* Removes all direct specification of `combine_with_last_data=True/False`, ensuring that the same behavior remains after the change (i.e., `overwrite_existing_data is not combine_with_last_data`).

Reviewed By: saitcakmak

Differential Revision: D75696517

Author: bernardbeckerman

ax/adapter/tests/test_base_adapter.py

@@ -646,10 +646,7 @@ def test_set_status_quo_with_multiple_observations(self) -> None:
             if additional_fetch:
                 # Fetch constraint metric an additional time. This will lead to two
                 # separate observations for the status quo arm.
-                exp.fetch_data(
-                    metrics=[exp.metrics["branin_map_constraint"]],
-                    combine_with_last_data=True,
-                )
+                exp.fetch_data(metrics=[exp.metrics["branin_map_constraint"]])
             with self.assertNoLogs(logger=logger, level="WARN"), mock.patch(
                 "ax.adapter.base._combine_multiple_status_quo_observations",
                 wraps=_combine_multiple_status_quo_observations,

ax/api/client.py

@@ -495,7 +495,7 @@ def attach_data(
 
         trial = assert_is_instance(self._experiment.trials[trial_index], Trial)
         trial.update_trial_data(
-            raw_data=data_with_progression, combine_with_last_data=True
+            raw_data=data_with_progression,
         )
 
         self._save_or_update_trial_in_db_if_possible(

ax/core/experiment.py

@@ -555,7 +555,7 @@ def get_metrics(self, metric_names: list[str] | None) -> list[Metric]:
     def fetch_data_results(
         self,
         metrics: list[Metric] | None = None,
-        combine_with_last_data: bool = False,
+        combine_with_last_data: bool | None = None,
         overwrite_existing_data: bool = False,
         **kwargs: Any,
     ) -> dict[int, dict[str, MetricFetchResult]]:
@@ -593,7 +593,7 @@ def fetch_trials_data_results(
         self,
         trial_indices: Iterable[int],
         metrics: list[Metric] | None = None,
-        combine_with_last_data: bool = False,
+        combine_with_last_data: bool | None = None,
         overwrite_existing_data: bool = False,
         **kwargs: Any,
     ) -> dict[int, dict[str, MetricFetchResult]]:
@@ -629,7 +629,7 @@ def fetch_data(
         self,
         trial_indices: Iterable[int] | None = None,
         metrics: list[Metric] | None = None,
-        combine_with_last_data: bool = False,
+        combine_with_last_data: bool | None = None,
         overwrite_existing_data: bool = False,
         **kwargs: Any,
     ) -> Data:
@@ -676,7 +676,7 @@ def _lookup_or_fetch_trials_results(
         self,
         trials: list[BaseTrial],
         metrics: Iterable[Metric] | None = None,
-        combine_with_last_data: bool = False,
+        combine_with_last_data: bool | None = None,
         overwrite_existing_data: bool = False,
         **kwargs: Any,
     ) -> dict[int, dict[str, MetricFetchResult]]:
@@ -755,23 +755,27 @@ def _fetch_trial_data(
     def attach_data(
         self,
         data: Data,
-        combine_with_last_data: bool = False,
+        # TODO[bbeckerman]: Deprecate this argument.
+        combine_with_last_data: bool | None = None,
         overwrite_existing_data: bool = False,
     ) -> int:
         """Attach data to experiment. Stores data in `experiment._data_by_trial`,
         to be looked up via `experiment.lookup_data_for_trial`.
 
         Args:
             data: Data object to store.
-            combine_with_last_data: By default, when attaching data, it's identified
+            combine_with_last_data [DEPRECATED]: This argument will be removed in Ax
+                1.3.0. Please leave this as ``None``, in which case it will be assigned
+                as ``not overwrite_existing_data``.
+                By default, when attaching data, it's identified
                 by its timestamp, and `experiment.lookup_data_for_trial` returns
                 data by most recent timestamp. Sometimes, however, we want to combine
                 the data from multiple calls to `attach_data` into one dataframe.
                 This might be because:
                     - We attached data for some metrics at one point and data for
                     the rest of the metrics later on.
                     - We attached data for some fidelity at one point and data for
-                    another fidelity later one.
+                    another fidelity later on.
                 To achieve that goal, set `combine_with_last_data` to `True`.
                 In this case, we will take the most recent previously attached
                 data, append the newly attached data to it, attach a new
@@ -785,10 +789,34 @@ def attach_data(
                 the incoming data contains all the information we need for a given
                 trial, we can replace the existing data for that trial, thereby
                 reducing the amount we need to store in the database.
+                If set to False, we will combine the data in the present call with that
+                from prior calls to `attach_data`, into one dataframe. Reasons to do
+                this may include:
+                    - We attached data for some metrics at one point and data for
+                    the rest of the metrics later on.
+                    - We attached data for some fidelity at one point and data for
+                    another fidelity later on.
+                In this case, we will take the most recent previously attached
+                data, append the newly attached data to it, and attach a new
+                Data object with the merged result and delete the old one.
+                Afterwards, calls to `lookup_data_for_trial` will return this
+                new combined data object. This operation will also validate that the
+                newly added data does not contain observations for metrics that
+                already have observations at the same fidelity in the most recent data.
+
 
         Returns:
             Timestamp of storage in millis.
         """
+        if combine_with_last_data is None:
+            combine_with_last_data = not overwrite_existing_data
+        else:
+            # logger.warning(
+            raise DeprecationWarning(
+                "The `combine_with_last_data` argument is deprecated and will be "
+                "removed soon. Please leave this as None, in which case it will be "
+                "assigned as `not overwrite_existing_data`."
+            )
         if combine_with_last_data and overwrite_existing_data:
             raise UnsupportedError(
                 "Cannot set both combine_with_last_data=True and "
@@ -921,7 +949,7 @@ def _get_last_data_without_similar_rows(
     def attach_fetch_results(
         self,
         results: Mapping[int, Mapping[str, MetricFetchResult]],
-        combine_with_last_data: bool = False,
+        combine_with_last_data: bool | None = None,
         overwrite_existing_data: bool = False,
     ) -> int | None:
         """

ax/core/multi_type_experiment.py

@@ -252,7 +252,7 @@ def fetch_data(
         self,
         trial_indices: Iterable[int] | None = None,
         metrics: list[Metric] | None = None,
-        combine_with_last_data: bool = False,
+        combine_with_last_data: bool | None = None,
         overwrite_existing_data: bool = False,
         **kwargs: Any,
     ) -> Data:
@@ -262,7 +262,12 @@ def fetch_data(
         return self.default_data_constructor.from_multiple_data(
             [
                 (
-                    trial.fetch_data(**kwargs, metrics=metrics)
+                    trial.fetch_data(
+                        **kwargs,
+                        metrics=metrics,
+                        overwrite_existing_data=overwrite_existing_data,
+                        combine_with_last_data=combine_with_last_data,
+                    )
                     if trial.status.expecting_data
                     else Data()
                 )

ax/core/tests/test_experiment.py

@@ -520,7 +520,7 @@ def test_FetchAndStoreData(self) -> None:
                 ]
             )
         )
-        t3 = exp.attach_data(new_data, combine_with_last_data=True)
+        t3 = exp.attach_data(new_data)
         # still 6 data objs, since we combined last one
         self.assertEqual(len(full_dict[0]), 6)
         self.assertIn("z", exp.lookup_data_for_ts(t3).df["metric_name"].tolist())
@@ -557,12 +557,6 @@ def test_OverwriteExistingData(self) -> None:
         # automatically attaches data
         data = exp.fetch_data()
 
-        # can't set both combine_with_last_data and overwrite_existing_data
-        with self.assertRaises(UnsupportedError):
-            exp.attach_data(
-                data, combine_with_last_data=True, overwrite_existing_data=True
-            )
-
         # data exists for two trials
         # data has been attached once for each trial
         self.assertEqual(len(exp._data_by_trial), 2)

ax/core/trial.py

@@ -280,7 +280,7 @@ def update_trial_data(
         raw_data: TEvaluationOutcome,
         metadata: dict[str, str | int] | None = None,
         sample_size: int | None = None,
-        combine_with_last_data: bool = False,
+        combine_with_last_data: bool | None = None,
     ) -> str:
         """Utility method that attaches data to a trial and
         returns an update message.
@@ -295,8 +295,8 @@ def update_trial_data(
             metadata: Additional metadata to track about this run, optional.
             sample_size: Number of samples collected for the underlying arm,
                 optional.
-            combine_with_last_data: Whether to combine the given data with the
-                data that was previously attached to the trial. See
+            combine_with_last_data [DEPRECATED]: Whether to combine the given data
+                with the data that was previously attached to the trial. See
                 `Experiment.attach_data` for a detailed explanation.
 
         Returns:

ax/service/ax_client.py

@@ -743,7 +743,6 @@ def update_running_trial_with_intermediate_data(
             raw_data=raw_data,
             metadata=metadata,
             sample_size=sample_size,
-            combine_with_last_data=True,
         )
         logger.info(f"Updated trial {trial_index} with data: " f"{data_update_repr}.")
 
@@ -794,7 +793,6 @@ def complete_trial(
             metadata=metadata,
             sample_size=sample_size,
             complete_trial=True,
-            combine_with_last_data=True,
         )
         logger.info(f"Completed trial {trial_index} with data: " f"{data_update_repr}.")
 
@@ -839,7 +837,6 @@ def update_trial_data(
             raw_data=raw_data,
             metadata=metadata,
             sample_size=sample_size,
-            combine_with_last_data=True,
         )
         logger.info(f"Added data: {data_update_repr} to trial {trial.index}.")
 
@@ -1558,7 +1555,7 @@ def _update_trial_with_raw_data(
         metadata: dict[str, str | int] | None = None,
         sample_size: int | None = None,
         complete_trial: bool = False,
-        combine_with_last_data: bool = False,
+        combine_with_last_data: bool | None = None,
     ) -> str:
         """Helper method attaches data to a trial, returns a str of update."""
         # Format the data to save.

ax/service/orchestrator.py

@@ -1890,11 +1890,6 @@ def _fetch_and_process_trials_data_results(
             kwargs = deepcopy(self.options.fetch_kwargs)
             for k, v in self.DEFAULT_FETCH_KWARGS.items():
                 kwargs.setdefault(k, v)
-            if kwargs.get("overwrite_existing_data") and kwargs.get(
-                "combine_with_last_data"
-            ):
-                # to avoid error https://fburl.com/code/ilix4okj
-                kwargs["overwrite_existing_data"] = False
             if self.trial_type is not None:
                 metrics = assert_is_instance(
                     self.experiment, MultiTypeExperiment

ax/service/tests/test_orchestrator.py

@@ -2072,9 +2072,6 @@ def test_it_does_not_overwrite_data_with_combine_fetch_kwarg(self) -> None:
             experiment=self.branin_experiment,  # Has runner and metrics.
             generation_strategy=gs,
             options=OrchestratorOptions(
-                fetch_kwargs={
-                    "combine_with_last_data": True,
-                },
                 **self.orchestrator_options_kwargs,
             ),
             db_settings=self.db_settings_if_always_needed,

ax/storage/sqa_store/tests/test_sqa_store.py

@@ -877,9 +877,7 @@ def test_ExperimentUpdateTrial(self) -> None:
         self.assertEqual(self.experiment, loaded_experiment)
 
         # Update a trial by attaching data again
-        self.experiment.attach_data(
-            get_data(trial_index=trial.index), combine_with_last_data=True
-        )
+        self.experiment.attach_data(get_data(trial_index=trial.index))
         save_or_update_trial(experiment=self.experiment, trial=trial)
 
         loaded_experiment = load_experiment(self.experiment.name)