-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Connect the model to the training type plugin at the start of run #8536
Changes from all commits
c02621e
0b3f14b
942b836
5beb5c9
0adb3a7
89eb948
c42c084
014a03c
7b51b8a
1a2c581
79edc9f
5a13861
a1149ef
75fb2f4
04b2a3b
6dad76d
b5ab1b5
589d101
48d5718
cd4c738
65ede1b
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 |
---|---|---|
|
@@ -16,7 +16,6 @@ | |
|
||
import torch | ||
from torch import Tensor | ||
from torch.nn import Module | ||
|
||
from pytorch_lightning.plugins.environments.cluster_environment import ClusterEnvironment | ||
from pytorch_lightning.plugins.training_type.ddp import DDPPlugin | ||
|
@@ -138,9 +137,11 @@ def wrap_policy(*args, **kwargs): | |
): | ||
yield | ||
|
||
def connect(self, model: Module) -> None: | ||
super().connect(model) | ||
model_call_configure_sharded_model_hook = getattr(model, "call_configure_sharded_model_hook", False) | ||
def setup_environment(self) -> None: | ||
super().setup_environment() | ||
model_call_configure_sharded_model_hook = getattr( | ||
self.lightning_module, "call_configure_sharded_model_hook", False | ||
) | ||
Comment on lines
+140
to
+144
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. Documenting the explanation for this change: Without this, the test That test calls def setup(self, stage: str) -> None:
self.call_configure_sharded_model_hook = False
...
def on_load_checkpoint(self, checkpoint: Dict[str, Any]) -> None:
self.setup("fit") This means, that before this change, the Some pseudocode to illustrate: Order before this PR load_checkpoint_weights
model.on_load_checkpoint (in the test)
model.setup -> this sets model.call_configure_sharded
acc.connect -> this checks model.call_configure_sharded
acc.setup_env
model.setup
configure_sharded_model -> this sets model.call_configure_sharded
acc.setup Order after this PR without this change: acc.connect -> this checks model.call_configure_sharded
load_checkpoint_weights
model.on_load_checkpoint (in the test)
model.setup -> this sets model.call_configure_sharded **BUG** as we already checked it
acc.setup_env
model.setup
configure_sharded_model -> this sets model.call_configure_sharded
acc.setup Order after this PR with this change: acc.connect
load_checkpoint_weights
model.on_load_checkpoint (in the test)
model.setup -> this sets model.call_configure_sharded
acc.setup_env -> this checks model.call_configure_sharded
model.setup
configure_sharded_model -> this sets model.call_configure_sharded
acc.setup |
||
if not model_call_configure_sharded_model_hook: | ||
# if model has not called configure sharded model, we reset | ||
# the training type plugin's call_configure_sharded_model_hook | ||
|
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.
small note: This could be an issue if we ever decide to expose the Accelerator API.
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.
Why an issue? There's the
connect
hook to connect the model already.self.model
should be available for the plugin whensetup
is calledThere 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.
@carmocca is this an invariant? should the accelerator assert that the model is available before calling setup training type plugin?
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.
@ananthsub We could