-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Move is_interactive_compatible to Strategy #11970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. well, unfortunately no longer #7550 :((( |
||
|
|
||
| def setup(self, trainer: "pl.Trainer") -> None: | ||
| os.environ["MASTER_PORT"] = str(self.cluster_environment.main_port) | ||
| super().setup(trainer) | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -593,6 +593,21 @@ def _init_strategy(self) -> None: | |||||
| else: | ||||||
| raise RuntimeError(f"{self.strategy} is not valid type: {self.strategy}") | ||||||
|
|
||||||
| from pytorch_lightning.utilities import _IS_INTERACTIVE | ||||||
|
|
||||||
| is_interactive_compatible = ( | ||||||
| self.strategy.is_interactive_compatible if hasattr(self.strategy, "is_interactive_compatible") else False | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not add it to the base class and have it default to False? |
||||||
| ) | ||||||
| if _IS_INTERACTIVE and not is_interactive_compatible: | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to have strategy object to get the is_interactive_compatible property.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think Ananth means: if _IS_INTERACTIVE and hasattr(self.strategy, "is_interactive_compatible") and not self.strategy.is_interactive_compatible:
raise ...
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if not _IS_INTERACTIVE:
return
if hasattr(self.strategy, "is_interactive_compatible") and not self.strategy.is_interactive_compatible:
raise |
||||||
| raise MisconfigurationException( | ||||||
| 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" | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| " In case you are spawning processes yourself, make sure to include the Trainer" | ||||||
| " creation inside the worker function." | ||||||
| ) | ||||||
|
|
||||||
| def _check_and_init_precision(self) -> PrecisionPlugin: | ||||||
| self._validate_precision_choice() | ||||||
| if isinstance(self._precision_plugin_flag, PrecisionPlugin): | ||||||
|
|
@@ -713,25 +728,6 @@ def _lazy_init_strategy(self) -> None: | |||||
| self.strategy.set_world_ranks() | ||||||
| self.strategy._configure_launcher() | ||||||
|
|
||||||
| from pytorch_lightning.utilities import _IS_INTERACTIVE | ||||||
|
|
||||||
| # TODO move is_compatible logic to strategy API | ||||||
| interactive_compatible_strategy = ( | ||||||
| DataParallelStrategy.strategy_name, | ||||||
| DDPSpawnStrategy.strategy_name, | ||||||
| DDPSpawnShardedStrategy.strategy_name, | ||||||
| TPUSpawnStrategy.strategy_name, | ||||||
| ) | ||||||
| if _IS_INTERACTIVE and self.strategy.strategy_name not in interactive_compatible_strategy: | ||||||
| raise MisconfigurationException( | ||||||
| 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:" | ||||||
| f" {', '.join(interactive_compatible_strategy)}." | ||||||
| " In case you are spawning processes yourself, make sure to include the Trainer" | ||||||
| " creation inside the worker function." | ||||||
| ) | ||||||
|
|
||||||
| # TODO: should be moved to _check_strategy_and_fallback(). | ||||||
| # Current test check precision first, so keep this check here to meet error order | ||||||
| if isinstance(self.accelerator, TPUAccelerator) and not isinstance( | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we move the property up to join the others?