Move is_interactive_compatible to Strategy #11970

four4fish · 2022-02-18T01:10:55Z

What does this PR do?

Fixes #11449 follow up item "Move _IS_INTERACTIVE check to strategy"

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

Was this discussed/approved via a GitHub issue? (not for typos and docs)
Did you read the contributor guideline, Pull Request section?
Did you make sure your PR does only one thing, instead of bundling different changes together?
Did you make sure to update the documentation with your changes? (if necessary)
Did you write any new necessary tests? (not for typos and docs)
Did you verify new and existing tests pass locally with your changes?
Did you list all the breaking changes introduced by this pull request?
Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

Is this pull request ready for review? (if not, please submit in draft mode)
Check that all items from Before submitting are resolved
Make sure the title is self-explanatory and the description concisely explains the PR
Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

ananthsub · 2022-02-18T01:17:33Z

pytorch_lightning/trainer/connectors/accelerator_connector.py

+        is_interactive_compatible = (
+            self.strategy.is_interactive_compatible if hasattr(self.strategy, "is_interactive_compatible") else False
+        )
+        if _IS_INTERACTIVE and not is_interactive_compatible:


do the _IS_INTERACTIVE check first. you can return early if it's not interactive. then you only need to check the strategy's property afterward instead of always checking it.

We need to have strategy object to get the is_interactive_compatible property.

I think Ananth means:

if _IS_INTERACTIVE and hasattr(self.strategy, "is_interactive_compatible") and not self.strategy.is_interactive_compatible: raise ...

if not _IS_INTERACTIVE: return if hasattr(self.strategy, "is_interactive_compatible") and not self.strategy.is_interactive_compatible: raise

ananthsub · 2022-02-18T01:24:48Z

pytorch_lightning/trainer/connectors/accelerator_connector.py

+        from pytorch_lightning.utilities import _IS_INTERACTIVE
+
+        is_interactive_compatible = (
+            self.strategy.is_interactive_compatible if hasattr(self.strategy, "is_interactive_compatible") else False


should these specific hasattr checks could be formalized as traits or mixins? someone has to go deep into the code to realize these specific properties need to be added to their strategy for this to work. we had the same discussion for backward sync

carmocca · 2022-02-18T01:25:17Z

pytorch_lightning/trainer/connectors/accelerator_connector.py

+        from pytorch_lightning.utilities import _IS_INTERACTIVE
+
+        is_interactive_compatible = (
+            self.strategy.is_interactive_compatible if hasattr(self.strategy, "is_interactive_compatible") else False


Why not add it to the base class and have it default to False?

carmocca · 2022-02-18T01:26:33Z

pytorch_lightning/trainer/connectors/accelerator_connector.py

+        is_interactive_compatible = (
+            self.strategy.is_interactive_compatible if hasattr(self.strategy, "is_interactive_compatible") else False
+        )
+        if _IS_INTERACTIVE and not is_interactive_compatible:


I think Ananth means:

if _IS_INTERACTIVE and hasattr(self.strategy, "is_interactive_compatible") and not self.strategy.is_interactive_compatible: raise ...

awaelchli · 2022-02-19T03:23:12Z

pytorch_lightning/strategies/ddp_spawn.py

@@ -109,6 +109,10 @@ def _is_single_process_single_device(self):
    def _configure_launcher(self):
        self._launcher = _SpawnLauncher(self)

+    @property
+    def is_interactive_compatible(self) -> bool:
+        return True


well, unfortunately no longer #7550 :(((

awaelchli · 2022-02-19T03:23:44Z

pytorch_lightning/strategies/ddp_spawn.py

@@ -109,6 +109,10 @@ def _is_single_process_single_device(self):
    def _configure_launcher(self):
        self._launcher = _SpawnLauncher(self)

+    @property


can we move the property up to join the others?

awaelchli · 2022-02-19T03:25:15Z

pytorch_lightning/trainer/connectors/accelerator_connector.py

+                f"`Trainer(strategy={self.strategy.strategy_name!r})` or"
+                f" `Trainer(accelerator={self.strategy.strategy_name!r})` is not compatible with an interactive"
+                " environment. Run your code as a script, or choose one of the compatible backends, for example:"
+                "dp, ddp_spawn, ddp_shard_spawn or tpu_spawn"


Suggested change

"dp, ddp_spawn, ddp_shard_spawn or tpu_spawn"

" dp, ddp_sharded_spawn or tpu_spawn."

awaelchli · 2022-02-19T03:33:16Z

Can we consider adding this to the launcher? it would be much less to think about when implementing a new strategy and the Jupyter environment compatibility is really just about the multiprocess launching so to me the process launchers are the better place to introduce this property.

What do people think?

EDIT: Found that interactive support is broken on master. See #12008

four4fish · 2022-02-22T23:27:31Z

Abandon this PR in favor of #12008

awaelchli · 2022-02-23T00:31:07Z

Thanks anyway for looking into this ☺️

Move is_interactive_compatible to Strategy

9d416d7

four4fish requested review from Borda, tchaton, SeanNaren, carmocca, awaelchli, justusschock and kaushikb11 as code owners February 18, 2022 01:10

ananthsub reviewed Feb 18, 2022

View reviewed changes

carmocca approved these changes Feb 18, 2022

View reviewed changes

awaelchli reviewed Feb 19, 2022

View reviewed changes

awaelchli mentioned this pull request Feb 19, 2022

Fix is_interactive_compatible logic after AcceleratorConnector rewrite #12008

Merged

11 tasks

mergify bot added the has conflicts label Feb 22, 2022

four4fish closed this Feb 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move is_interactive_compatible to Strategy #11970

Move is_interactive_compatible to Strategy #11970

four4fish commented Feb 18, 2022

ananthsub Feb 18, 2022

four4fish Feb 18, 2022

carmocca Feb 18, 2022 •

edited

Loading

ananthsub Feb 18, 2022

ananthsub Feb 18, 2022

carmocca Feb 18, 2022

carmocca Feb 18, 2022 •

edited

Loading

awaelchli Feb 19, 2022

awaelchli Feb 19, 2022

awaelchli Feb 19, 2022

awaelchli commented Feb 19, 2022 •

edited

Loading

four4fish commented Feb 22, 2022

awaelchli commented Feb 23, 2022

	"dp, ddp_spawn, ddp_shard_spawn or tpu_spawn"
	" dp, ddp_sharded_spawn or tpu_spawn."

Move is_interactive_compatible to Strategy #11970

Move is_interactive_compatible to Strategy #11970

Conversation

four4fish commented Feb 18, 2022

What does this PR do?

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

PR review

Did you have fun?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carmocca Feb 18, 2022 • edited Loading

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

carmocca Feb 18, 2022 • edited Loading

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

awaelchli commented Feb 19, 2022 • edited Loading

four4fish commented Feb 22, 2022

awaelchli commented Feb 23, 2022

carmocca Feb 18, 2022 •

edited

Loading

carmocca Feb 18, 2022 •

edited

Loading

awaelchli commented Feb 19, 2022 •

edited

Loading