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

Proposal(CLI): after_instantiate_classes hook #20400

Closed
AlessandroW opened this issue Nov 6, 2024 · 1 comment · Fixed by #20401
Closed

Proposal(CLI): after_instantiate_classes hook #20400

AlessandroW opened this issue Nov 6, 2024 · 1 comment · Fixed by #20401
Labels
design Includes a design discussion feature Is an improvement or enhancement lightningcli pl.cli.LightningCLI

Comments

@AlessandroW
Copy link
Contributor

AlessandroW commented Nov 6, 2024

Description & Motivation

Adds a after_instantiate_classes hook to the Lightning CLI, called after self.instantiate_classes() during the initalization of LightningCLI.

Pitch

While having the Lightning CLI is great, it is not perfect for each use case out-of-the-box. Hence, you included hooks like before_instantiate_classes and describe in the docs how to extend the CLI. Problem is, you cannot extend this feature without hacks or substantial copy-pasta.

I think, to further improve the CLI, without adding any complexity, it makes sense to add a after_instantiate_classes hook, too.

Alternatives

  1. Hacks
  • Extend the Lightning CLI and run the after_instantiate_classes function before the self._run_subcommand function.
  • Problems: it's not intuitive that the function is called there, won't be called if self.subcommand is None
  1. Copy-Pasta
  • Extend the Lightning CLI and replace the original __init__ with the proposed one.
  • Problems: could break with any update, lots of code duplication

Additional context

No response

cc @Borda @tchaton @justusschock @awaelchli @mauvilsa

@AlessandroW AlessandroW added feature Is an improvement or enhancement needs triage Waiting to be triaged by maintainers labels Nov 6, 2024
@lantiga lantiga added design Includes a design discussion lightningcli pl.cli.LightningCLI and removed needs triage Waiting to be triaged by maintainers labels Nov 12, 2024
@AlessandroW
Copy link
Contributor Author

Glad, the proposal was accepted! Thank you for merging it.

Can you add an example for how the new hook will be used?
#20401 (comment)

I used it for logging and data-specific cross-validation. One reason for this approach was the use of MLFlow's parent/child runs for logging without using an additional dependency like Hydra. For refactoring I moved some parts of the logic into before_fit.

Anyway, my situation is very specific and not a good, simple example. The more important part is Lightning's approach of providing hooks to create such specific solutions without having to write everything from scratch. The after_instantiate_classes is just another case where, e.g., run-specific logging that shouldn't be part of the model or the datamodule can be called.

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 lightningcli pl.cli.LightningCLI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants