Revert "Revert "[train+tune] Make some deprecations for 2.10" (#42899)…

…" (#42909)

This PR hard-deprecates/removes APIs that were scheduled for removal: `TuneConfig(chdir_to_trial_dir)` and certain `SyncConfig` parameters. This PR also resolves some other miscellaneous TODOs.

This PR fixes some RLlib tests that were failing previously from #42812

---------

Signed-off-by: Justin Yu <justinvyu@anyscale.com>

doc/source/tune/examples/BUILD

@@ -21,9 +21,7 @@ py_test_run_all_notebooks(
         "pbt_ppo_example.ipynb",
         "tune-xgboost.ipynb",
         "pbt_transformers.ipynb",  # Transformers uses legacy Tune APIs.
-        # TODO(justinvyu): tune_sklearn uses removed experiment analysis method
-        "tune-sklearn.ipynb",
-    ],
+   ],
     tags = [
         "exclusive",
         "team:ml",

python/ray/train/_internal/data_config.py

@@ -6,9 +6,6 @@
 from ray.data import DataIterator, Dataset, ExecutionOptions, NodeIdStr
 from ray.data._internal.execution.interfaces.execution_options import ExecutionResources
 from ray.data.preprocessor import Preprocessor
-
-# TODO(justinvyu): Fix the circular import error
-from ray.train.constants import TRAIN_DATASET_KEY  # noqa
 from ray.util.annotations import DeveloperAPI, PublicAPI
 
 

python/ray/train/_internal/syncer.py

@@ -3,14 +3,12 @@
 import threading
 import time
 import traceback
-import warnings
 from dataclasses import dataclass
 from typing import Any, Callable, Dict, List, Optional, Tuple, Union
 
 from ray._private.thirdparty.tabulate.tabulate import tabulate
 from ray.train.constants import _DEPRECATED_VALUE
-from ray.util import log_once
-from ray.util.annotations import PublicAPI
+from ray.util.annotations import DeveloperAPI, PublicAPI
 from ray.widgets import Template
 
 logger = logging.getLogger(__name__)
@@ -70,24 +68,26 @@ class SyncConfig:
     syncer: Optional[Union[str, "Syncer"]] = _DEPRECATED_VALUE
     sync_on_checkpoint: bool = _DEPRECATED_VALUE
 
+    # TODO(justinvyu): [Deprecated] Remove in 2.11.
     def _deprecation_warning(self, attr_name: str, extra_msg: str):
         if getattr(self, attr_name) != _DEPRECATED_VALUE:
-            if log_once(f"sync_config_param_deprecation_{attr_name}"):
-                warnings.warn(
-                    f"`SyncConfig({attr_name})` is a deprecated configuration "
-                    "and will be ignored. Please remove it from your `SyncConfig`, "
-                    "as this will raise an error in a future version of Ray."
-                    f"{extra_msg}"
-                )
+            raise DeprecationWarning(
+                f"`SyncConfig({attr_name})` is a deprecated configuration "
+                "Please remove it from your `SyncConfig`. "
+                f"{extra_msg}"
+            )
 
     def __post_init__(self):
-        for (attr_name, extra_msg) in [
-            ("upload_dir", "\nPlease specify `train.RunConfig(storage_path)` instead."),
+        for attr_name, extra_msg in [
+            (
+                "upload_dir",
+                "\nPlease specify `ray.train.RunConfig(storage_path)` instead.",
+            ),
             (
                 "syncer",
                 "\nPlease implement custom syncing logic with a custom "
                 "`pyarrow.fs.FileSystem` instead, and pass it into "
-                "`train.RunConfig(storage_filesystem)`. "
+                "`ray.train.RunConfig(storage_filesystem)`. "
                 "See here: https://docs.ray.io/en/latest/train/user-guides/persistent-storage.html#custom-storage",  # noqa: E501
             ),
             ("sync_on_checkpoint", ""),
@@ -178,6 +178,7 @@ def wait(self, timeout: Optional[float] = None) -> Any:
         return result
 
 
+@DeveloperAPI
 class Syncer(abc.ABC):
     """Syncer class for synchronizing data between Ray nodes and remote (cloud) storage.
 

python/ray/tune/analysis/experiment_analysis.py

@@ -702,36 +702,3 @@ def make_stub_if_needed(trial: Trial) -> Trial:
 
         state["trials"] = [make_stub_if_needed(t) for t in state["trials"]]
         return state
-
-    # TODO(ml-team): [Deprecated] Remove in 2.8
-    @property
-    def best_logdir(self) -> str:
-        raise DeprecationWarning(
-            "`best_logdir` is deprecated. Use `best_trial.local_path` instead."
-        )
-
-    def get_best_logdir(
-        self,
-        metric: Optional[str] = None,
-        mode: Optional[str] = None,
-        scope: str = "last",
-    ) -> Optional[str]:
-        raise DeprecationWarning(
-            "`get_best_logdir` is deprecated. "
-            "Use `get_best_trial(...).local_path` instead."
-        )
-
-    def get_trial_checkpoints_paths(
-        self, trial: Trial, metric: Optional[str] = None
-    ) -> List[Tuple[str, Number]]:
-        raise DeprecationWarning(
-            "`get_trial_checkpoints_paths` is deprecated. "
-            "Use `get_best_checkpoint` or wrap this `ExperimentAnalysis` in a "
-            "`ResultGrid` and use `Result.best_checkpoints` instead."
-        )
-
-    def fetch_trial_dataframes(self) -> Dict[str, DataFrame]:
-        raise DeprecationWarning(
-            "`fetch_trial_dataframes` is deprecated. "
-            "Access the `trial_dataframes` property instead."
-        )

python/ray/tune/experiment/config_parser.py

@@ -78,15 +78,6 @@ def _make_parser(parser_creator=None, **kwargs):
         action="store_true",
         help="Whether to checkpoint at the end of the experiment. Default is False.",
     )
-    parser.add_argument(
-        "--sync-on-checkpoint",
-        action="store_true",
-        help="Enable sync-down of trial checkpoint to guarantee "
-        "recoverability. If unset, checkpoint syncing from worker "
-        "to driver is asynchronous, so unset this only if synchronous "
-        "checkpointing is too slow and trial restoration failures "
-        "can be tolerated.",
-    )
     parser.add_argument(
         "--keep-checkpoints-num",
         default=None,

python/ray/tune/experiment/experiment.py

@@ -370,8 +370,8 @@ def local_path(self) -> Optional[str]:
     @property
     @Deprecated("Replaced by `local_path`")
     def local_dir(self):
-        # Deprecate: Raise in 2.5, Remove in 2.6
-        return self.local_path
+        # TODO(justinvyu): [Deprecated] Remove in 2.11.
+        raise DeprecationWarning("Use `local_path` instead of `local_dir`.")
 
     @property
     def remote_path(self) -> Optional[str]:
@@ -386,11 +386,10 @@ def checkpoint_config(self):
         return self.spec.get("checkpoint_config")
 
     @property
-    @Deprecated("Replaced by `checkpoint_dir`")
+    @Deprecated("Replaced by `local_path`")
     def checkpoint_dir(self):
-        # Deprecate: Raise in 2.5, Remove in 2.6
-        # Provided when initializing Experiment, if so, return directly.
-        return self.local_path
+        # TODO(justinvyu): [Deprecated] Remove in 2.11.
+        raise DeprecationWarning("Use `local_path` instead of `checkpoint_dir`.")
 
     @property
     def run_identifier(self):

python/ray/tune/experiment/trial.py

@@ -541,8 +541,8 @@ def local_experiment_path(self) -> str:
     @property
     @Deprecated("Replaced by `local_path`")
     def logdir(self) -> Optional[str]:
-        # Deprecate: Raise in 2.5, Remove in 2.6
-        return self.local_path
+        # TODO(justinvyu): [Deprecated] Remove in 2.11.
+        raise DeprecationWarning("Use `local_path` instead of `logdir`.")
 
     @property
     def local_path(self) -> Optional[str]:
@@ -627,8 +627,8 @@ def reset(self):
 
     @Deprecated("Replaced by `init_local_path()`")
     def init_logdir(self):
-        # Deprecate: Raise in 2.5, Remove in 2.6
-        self.init_local_path()
+        # TODO(justinvyu): [Deprecated] Remove in 2.11.
+        raise DeprecationWarning("Use `init_local_path` instead of `init_logdir`.")
 
     def init_local_path(self):
         """Init logdir."""

python/ray/tune/syncer.py

@@ -1,32 +1,16 @@
 import logging
-import warnings
 
 from ray.train._internal.syncer import SyncConfig as TrainSyncConfig
 from ray.util.annotations import Deprecated
-from ray.util.debug import log_once
 
 
 logger = logging.getLogger(__name__)
 
 
 @Deprecated
 class SyncConfig(TrainSyncConfig):
-    def __new__(cls: type, *args, **kwargs):
-        if log_once("sync_config_moved"):
-            warnings.warn(
-                "`tune.SyncConfig` has been moved to `train.SyncConfig`. "
-                "Please update your code to use `train.SyncConfig`, "
-                "as this will raise an error in a future Ray version.",
-                stacklevel=2,
-            )
-        return super(SyncConfig, cls).__new__(cls, *args, **kwargs)
-
-
-@Deprecated
-class Syncer:
     def __new__(cls: type, *args, **kwargs):
         raise DeprecationWarning(
-            "`tune.syncer.Syncer` has been deprecated. "
-            "Please implement custom syncing logic via a custom "
-            "`train.RunConfig(storage_filesystem)` instead."
+            "`ray.tune.SyncConfig` has been moved to `ray.train.SyncConfig`. "
+            "Please update your code to use `ray.train.SyncConfig`."
         )

python/ray/tune/tests/test_tuner.py

@@ -12,7 +12,6 @@
 import ray
 from ray import train, tune
 from ray.train import CheckpointConfig, RunConfig, ScalingConfig
-from ray.train.constants import RAY_CHDIR_TO_TRIAL_DIR
 from ray.train.examples.pytorch.torch_linear_example import (
     train_func as linear_train_func,
 )
@@ -420,11 +419,8 @@ def train_func(config):
 
 def test_tuner_no_chdir_to_trial_dir_deprecated(shutdown_only, chdir_tmpdir):
     """Test the deprecated `chdir_to_trial_dir` config."""
-    _test_no_chdir("tuner", {}, use_deprecated_config=True)
-
-    # The deprecated config will fallback ot setting the environment variable.
-    # Reset it for the following tests
-    os.environ.pop(RAY_CHDIR_TO_TRIAL_DIR, None)
+    with pytest.raises(DeprecationWarning):
+        _test_no_chdir("tuner", {}, use_deprecated_config=True)
 
 
 @pytest.mark.parametrize("runtime_env", [{}, {"working_dir": "."}])

python/ray/tune/tune.py

@@ -396,14 +396,7 @@ def run(
             unique identifier (such as `Trial.trial_id`) is used in each trial's
             directory name. Otherwise, trials could overwrite artifacts and checkpoints
             of other trials. The return value cannot be a path.
-        chdir_to_trial_dir: Deprecated. Use `RAY_CHDIR_TO_TRIAL_DIR=0` instead.
-            Whether to change the working directory of each worker
-            to its corresponding trial directory. Defaults to `True` to prevent
-            contention between workers saving trial-level outputs.
-            If set to `False`, files are accessible with paths relative to the
-            original working directory. However, all workers on the same node now
-            share the same working directory, so be sure to use
-            `ray.train.get_context().get_trial_dir()` as the path to save any outputs.
+        chdir_to_trial_dir: Deprecated. Set the `RAY_CHDIR_TO_TRIAL_DIR` env var instead
         sync_config: Configuration object for syncing. See train.SyncConfig.
         export_formats: List of formats that exported at the end of
             the experiment. Default is None.
@@ -666,16 +659,15 @@ class and registered trainables.
         )
         checkpoint_config.checkpoint_at_end = checkpoint_at_end
 
+    # TODO(justinvyu): [Deprecated] Remove in 2.11.
     if chdir_to_trial_dir != _DEPRECATED_VALUE:
-        warnings.warn(
-            "`chdir_to_trial_dir` is deprecated and will be removed. "
+        raise DeprecationWarning(
+            "`chdir_to_trial_dir` is deprecated. "
             f"Use the {RAY_CHDIR_TO_TRIAL_DIR} environment variable instead. "
             "Set it to 0 to disable the default behavior of changing the "
             "working directory.",
             DeprecationWarning,
         )
-        if chdir_to_trial_dir is False:
-            os.environ[RAY_CHDIR_TO_TRIAL_DIR] = "0"
 
     if num_samples == -1:
         num_samples = sys.maxsize

python/ray/tune/tune_config.py

@@ -58,15 +58,7 @@ class TuneConfig:
             directory name. Otherwise, trials could overwrite artifacts and checkpoints
             of other trials. The return value cannot be a path.
             NOTE: This API is in alpha and subject to change.
-        chdir_to_trial_dir: Deprecated. Use the `RAY_CHDIR_TO_TRIAL_DIR=0`
-            environment variable instead.
-            Whether to change the working directory of each worker
-            to its corresponding trial directory. Defaults to `True` to prevent
-            contention between workers saving trial-level outputs.
-            If set to `False`, files are accessible with paths relative to the
-            original working directory. However, all workers on the same node now
-            share the same working directory, so be sure to use
-            `ray.train.get_context().get_trial_dir()` as the path to save any outputs.
+        chdir_to_trial_dir: Deprecated. Set the `RAY_CHDIR_TO_TRIAL_DIR` env var instead
     """
 
     # Currently this is not at feature parity with `tune.run`, nor should it be.

rllib/common.py

@@ -8,8 +8,8 @@
 import requests
 
 
+from ray.train.constants import _DEPRECATED_VALUE
 from ray.tune.experiment.config_parser import _make_parser
-from ray.tune.result import DEFAULT_RESULTS_DIR
 
 
 class FrameworkEnum(str, Enum):
@@ -73,7 +73,6 @@ def download_example_file(
     """
     temp_file = None
     if not os.path.exists(example_file):
-
         example_url = base_url + example_file if base_url else example_file
         print(f">>> Attempting to download example file {example_url}...")
 
@@ -133,8 +132,12 @@ def get_help(key: str) -> str:
     v="Whether to use INFO level logging.",
     vv="Whether to use DEBUG level logging.",
     resume="Whether to attempt to resume from previous experiments.",
-    local_dir=f"Local dir to save training results to. "
-    f"Defaults to '{DEFAULT_RESULTS_DIR}'.",
+    storage_path=(
+        "Directory to save training results and checkpoints to. "
+        "Can be either a local directory or a cloud storage path "
+        "(e.g. '/tmp/ray_results', 's3://bucket-name'). Defaults to '~/ray_results'."
+    ),
+    local_dir="Deprecated. Use `storage_path` instead.",
     local_mode="Run Ray in local mode for easier debugging.",
     ray_address="Connect to an existing Ray cluster at this address instead "
     "of starting a new one.",
@@ -143,7 +146,7 @@ def get_help(key: str) -> str:
     ray_num_gpus="The '--num-gpus' argument to use if starting a new cluster.",
     ray_num_nodes="Emulate multiple cluster nodes for debugging.",
     ray_object_store_memory="--object-store-memory to use if starting a new cluster.",
-    upload_dir="Optional URI to sync training results to (e.g. s3://bucket).",
+    upload_dir="Deprecated. Use `storage_path` instead.",
     trace="Whether to attempt to enable eager-tracing for framework=tf2.",
     torch="Whether to use PyTorch (instead of tf) as the DL framework. "
     "This argument is deprecated, please use --framework to select 'torch'"
@@ -228,15 +231,16 @@ class CLIArguments:
     NumSamples = typer.Option(1, help=get_help("num_samples"))
     CheckpointFreq = typer.Option(0, help=get_help("checkpoint_freq"))
     CheckpointAtEnd = typer.Option(True, help=get_help("checkpoint_at_end"))
-    LocalDir = typer.Option(DEFAULT_RESULTS_DIR, help=train_help.get("local_dir"))
+    StoragePath = typer.Option(None, help=train_help.get("storage_path"))
+    LocalDir = typer.Option(_DEPRECATED_VALUE, help=train_help.get("local_dir"))
     Restore = typer.Option(None, help=get_help("restore"))
     Framework = typer.Option(None, help=train_help.get("framework"))
     ResourcesPerTrial = typer.Option(None, help=get_help("resources_per_trial"))
     KeepCheckpointsNum = typer.Option(None, help=get_help("keep_checkpoints_num"))
     CheckpointScoreAttr = typer.Option(
-        "training_iteration", help=get_help("sync_on_checkpoint")
+        "training_iteration", help=get_help("checkpoint_score_attr")
     )
-    UploadDir = typer.Option("", help=train_help.get("upload_dir"))
+    UploadDir = typer.Option(_DEPRECATED_VALUE, help=train_help.get("upload_dir"))
     Trace = typer.Option(False, help=train_help.get("trace"))
     LocalMode = typer.Option(False, help=train_help.get("local_mode"))
     Scheduler = typer.Option("FIFO", help=get_help("scheduler"))

rllib/tests/test_rllib_train_and_evaluate.py

@@ -32,7 +32,7 @@ def evaluate_test(algo, env="CartPole-v1", test_episode_rollout=False):
 
         print("RLlib dir = {}\nexists={}".format(rllib_dir, os.path.exists(rllib_dir)))
         os.system(
-            "TEST_TMPDIR='{}' python {}/train.py --local-dir={} --run={} "
+            "TEST_TMPDIR='{}' python {}/train.py --storage-path={} --run={} "
             "--checkpoint-freq=1 ".format(tmp_dir, rllib_dir, tmp_dir, algo)
             + "--config='{"
             + '"num_workers": 1, "num_gpus": 0{}{}'.format(fw_, extra_config)
@@ -103,7 +103,7 @@ def learn_test_plus_evaluate(algo: str, env="CartPole-v1"):
 
         print("RLlib dir = {}\nexists={}".format(rllib_dir, os.path.exists(rllib_dir)))
         os.system(
-            "TEST_TMPDIR='{}' python {}/train.py --local-dir={} --run={} "
+            "TEST_TMPDIR='{}' python {}/train.py --storage-path={} --run={} "
             "--checkpoint-freq=1 --checkpoint-at-end ".format(
                 tmp_dir, rllib_dir, tmp_dir, algo
             )