Fix strategy type validation in connectors (#16693)

Author: awaelchli

src/lightning_fabric/CHANGELOG.md

@@ -19,7 +19,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 ### Fixed
 
--
+- Fixed an attribute error and improved input validation for invalid strategy types being passed to Fabric ([#16693](https://github.com/Lightning-AI/lightning/pull/16693))
 
 
 ## [1.9.1] - 2023-02-10

src/lightning_fabric/connector.py

@@ -187,6 +187,7 @@ def _check_config_and_set_final_flags(
         if strategy is not None and strategy not in self._registered_strategies and not isinstance(strategy, Strategy):
             raise ValueError(
                 f"You selected an invalid strategy name: `strategy={strategy!r}`."
+                " It must be either a string or an instance of `lightning.fabric.strategies.Strategy`."
                 " Example choices: ddp, ddp_spawn, deepspeed, dp, ..."
                 " Find a complete list of options in our documentation at https://lightning.ai"
             )
@@ -436,12 +437,12 @@ def _check_strategy_and_fallback(self) -> None:
 
     def _init_strategy(self) -> None:
         """Instantiate the Strategy given depending on the setting of ``_strategy_flag``."""
+        # The validation of `_strategy_flag` already happened earlier on in the connector
+        assert isinstance(self._strategy_flag, (str, Strategy))
         if isinstance(self._strategy_flag, str):
             self.strategy = STRATEGY_REGISTRY.get(self._strategy_flag)
-        elif isinstance(self._strategy_flag, Strategy):
-            self.strategy = self._strategy_flag
         else:
-            raise RuntimeError(f"{self.strategy} is not valid type: {self.strategy}")
+            self.strategy = self._strategy_flag
 
     def _check_and_init_precision(self) -> Precision:
         self._validate_precision_choice()

src/pytorch_lightning/CHANGELOG.md

@@ -19,7 +19,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 
 ### Fixed
 
--
+- Fixed an attribute error and improved input validation for invalid strategy types being passed to Trainer ([#16693](https://github.com/Lightning-AI/lightning/pull/16693))
 
 
 ## [1.9.1] - 2023-02-10

src/pytorch_lightning/trainer/connectors/accelerator_connector.py

@@ -273,6 +273,14 @@ def _check_config_and_set_final_flags(
                     " you can use `Trainer(strategy='ddp_spawn', accelerator='tpu')` instead."
                 )
 
+        if strategy is not None and strategy not in self._registered_strategies and not isinstance(strategy, Strategy):
+            raise ValueError(
+                f"You selected an invalid strategy name: `strategy={strategy!r}`."
+                " It must be either a string or an instance of `lightning.pytorch.strategies.Strategy`."
+                " Example choices: ddp, ddp_spawn, deepspeed, dp, ..."
+                " Find a complete list of options in our documentation at https://lightning.ai"
+            )
+
         if (
             accelerator is not None
             and accelerator not in self._accelerator_types
@@ -694,13 +702,13 @@ def _init_strategy(self) -> None:
             # handle horovod has to happen before initialize strategy because HorovodStrategy needs hvd.init() first.
             # TODO lazy initialized and setup horovod strategy `global_rank`
             self._handle_horovod()
+        # The validation of `_strategy_flag` already happened earlier on in the connector
+        assert isinstance(self._strategy_flag, (str, Strategy))
         if isinstance(self._strategy_flag, str):
             self.strategy = StrategyRegistry.get(self._strategy_flag)
-        elif isinstance(self._strategy_flag, Strategy):
+        else:
             # TODO(fabric): remove ignore after merging Fabric and PL strategies
             self.strategy = self._strategy_flag  # type: ignore[assignment]
-        else:
-            raise RuntimeError(f"{self.strategy} is not valid type: {self.strategy}")
 
     def _check_and_init_precision(self) -> PrecisionPlugin:
         self._validate_precision_choice()

tests/tests_fabric/test_connector.py

@@ -386,9 +386,10 @@ def test_invalid_accelerator_choice():
         _Connector(accelerator="cocofruit")
 
 
-def test_invalid_strategy_choice():
-    with pytest.raises(ValueError, match="You selected an invalid strategy name: `strategy='cocofruit'`"):
-        _Connector(strategy="cocofruit")
+@pytest.mark.parametrize("invalid_strategy", ["cocofruit", object()])
+def test_invalid_strategy_choice(invalid_strategy):
+    with pytest.raises(ValueError, match="You selected an invalid strategy name:"):
+        _Connector(strategy=invalid_strategy)
 
 
 @pytest.mark.parametrize(

tests/tests_pytorch/trainer/connectors/test_accelerator_connector.py

@@ -64,6 +64,12 @@ def test_accelerator_invalid_choice():
         Trainer(accelerator="invalid")
 
 
+@pytest.mark.parametrize("invalid_strategy", ["cocofruit", object()])
+def test_invalid_strategy_choice(invalid_strategy):
+    with pytest.raises(ValueError, match="You selected an invalid strategy name:"):
+        AcceleratorConnector(strategy=invalid_strategy)
+
+
 @RunIf(skip_windows=True, standalone=True)
 def test_strategy_choice_ddp_on_cpu(tmpdir):
     """Test that selecting DDPStrategy on CPU works."""
@@ -373,13 +379,6 @@ def test_unsupported_strategy_types_on_cpu_and_fallback():
     assert isinstance(trainer.strategy, DDPStrategy)
 
 
-def test_exception_invalid_strategy():
-    with pytest.raises(MisconfigurationException, match=r"strategy='ddp_cpu'\)` is not a valid"):
-        Trainer(strategy="ddp_cpu")
-    with pytest.raises(MisconfigurationException, match=r"strategy='tpu_spawn'\)` is not a valid"):
-        Trainer(strategy="tpu_spawn")
-
-
 @pytest.mark.parametrize(
     ["strategy", "strategy_class"],
     (
@@ -442,7 +441,7 @@ def test_strategy_choice_cpu_instance(strategy_class):
         pytest.param("deepspeed", DeepSpeedStrategy, marks=RunIf(deepspeed=True)),
     ],
 )
-def test_strategy_choice_gpu_str(strategy, strategy_class):
+def test_strategy_choice_gpu_str(strategy, strategy_class, cuda_count_2, mps_count_0):
     if "sharded" in strategy:
         with pytest.deprecated_call(match="FairScale has been deprecated in v1.9.0"):
             trainer = Trainer(strategy=strategy, accelerator="gpu", devices=2)