Skip to content
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

[RFC] Create explicit setup and teardown hooks for each stage on the Lightning and DataModules #6420

Closed
ananthsub opened this issue Mar 8, 2021 · 3 comments
Labels
design Includes a design discussion feature Is an improvement or enhancement won't fix This will not be worked on

Comments

@ananthsub
Copy link
Contributor

ananthsub commented Mar 8, 2021

🚀 Feature

LightningModules and DataModules currently support a setup API which takes an optional stage argument.
#6386 addresses some issues in the setup/teardown lifecycle, so I was wondering if we should take this further (#6401)

Motivation

Pros of making the separate hooks for each stage:

  • Clarity in the API that helps forwards compatibility: In the current scheme, the Lightning trainer can pass an arbitrary value for stage that user code might not handle. With the explicit hooks, new stages becomes opt-in for users, as users must implement the corresponding hook in their lightning/data module
  • Consistency in the API: this matches the pattern already established for Lightning/data modules which have train/validation/test/predict defined as separate hooks
  • On the Lightning internals, we can remove the base datamodule wrapper class, and remove the has_setup_{stage} attributes since it'll be obvious when the hooks are called

Cons:

  • This requires a deprecation process and can cause thrash for users
  • Users now have to implement more hooks. However, a mitigation is that the refactoring should be straightforward as users can easily share code with a helper function in the lightning/data module.

Pitch

We add the following hooks to the DataHooks base:

  • on_{stage}_prepare_data
  • on_{stage}_setup
  • on_{stage}_teardown

for the existing values of stage: fit, test, validate, predict

Similarly, we add corresponding hooks to the Callback base:

  • on_{stage}_setup
  • on_{stage}_teardown

During the migration, in the trainer, if the Lightning(Data)Module has this hook implemented, then we call it. Otherwise, we fallback to calling the existing setup/teardown hooks. We do the same for the callback hooks.

We could set a longer deprecation timeline for this given how prevalent these hooks are. For example, we don't deprecate prepare_data, setup, or teardown until version 1.7+.

Additionally, we should move the trainer argument prepare_data_per_node to the DataHooks base, similar to how automatic_optimization is a property of the LightningModule. This point is separate from the overall hooks discussion and could happen faster to slightly simplify the trainer API.

Alternatives

Keep the existing hooks

Additional context

@ananthsub ananthsub added feature Is an improvement or enhancement help wanted Open to be worked on design Includes a design discussion and removed help wanted Open to be worked on labels Mar 8, 2021
@carmocca
Copy link
Contributor

carmocca commented Mar 29, 2021

One tricky issue about this is that setup is suggested for layer initialization (https://pytorch-lightning.readthedocs.io/en/latest/starter/introduction_guide.html#models-defined-by-data).

So implementing this change would need to consider this case. This also is problematic given the current direction the model parallel hook is taking: #6679 (comment)

cc: @SeanNaren

@stale
Copy link

stale bot commented Apr 29, 2021

This issue has been automatically marked as stale because it hasn't had any recent activity. This issue will be closed in 7 days if no further activity occurs. Thank you for your contributions, Pytorch Lightning Team!

@stale stale bot added the won't fix This will not be worked on label Apr 29, 2021
@kaushikb11 kaushikb11 removed the won't fix This will not be worked on label Apr 29, 2021
@edenlightning edenlightning added this to the v1.4 milestone May 9, 2021
@edenlightning edenlightning modified the milestones: v1.4, v1.5 Jul 1, 2021
@ananthsub ananthsub removed this from the v1.5 milestone Oct 7, 2021
@stale
Copy link

stale bot commented Nov 7, 2021

This issue has been automatically marked as stale because it hasn't had any recent activity. This issue will be closed in 7 days if no further activity occurs. Thank you for your contributions, Pytorch Lightning Team!

@stale stale bot added the won't fix This will not be worked on label Nov 7, 2021
@stale stale bot closed this as completed Nov 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design Includes a design discussion feature Is an improvement or enhancement won't fix This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants