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

Deprecate process_position from the Trainer constructor #8968

Closed
ananthsub opened this issue Aug 18, 2021 · 3 comments · Fixed by #9222
Closed

Deprecate process_position from the Trainer constructor #8968

ananthsub opened this issue Aug 18, 2021 · 3 comments · Fixed by #9222
Assignees
Labels
deprecation Includes a deprecation design Includes a design discussion feature Is an improvement or enhancement good first issue Good for newcomers help wanted Open to be worked on let's do it! approved to implement refactor

Comments

@ananthsub
Copy link
Contributor

ananthsub commented Aug 18, 2021

🚀 Feature

Remove this two arguments from the Trainer constructor

Motivation

We are auditing the Lightning components and APIs to assess opportunities for improvements:

The Trainer today has over 50 arguments to its constructor. This number is growing with each feature release, and makes the trainer cluttered. It also hurts the extensibility of the trainer: a number of arguments passed to the Trainer are to customize other utilities. Plumbing arguments through the trainer creates an undesirable coupling: when the underlying components change, the framework is forced to make breaking API changes in at least two places:

  1. The underlying component
  2. The trainer constructor

Example: #8062

Example: #8780
-log_gpu_memory accepting min_max is hyper-specific to nvidia-smi, and isn't applicable for torch.cuda.memory stats

Upcoming examples:

  • Supporting new types of progress bars feat: Add Rich Progress Bar #8929
  • But our current trainer arguments for progress bars could be specific to the TQDM implementation

Pitch

  1. Deprecate process_position off the Trainer constructor in v1.5

  2. In version 1.7, remove process_positionfrom the Trainer entirely

To customize this, users can still construct the ProgressBar callback object and pass it to the Trainer.

Alternatives

Additional context


If you enjoy Lightning, check out our other projects! ⚡

  • Metrics: Machine learning metrics for distributed, scalable PyTorch applications.

  • Flash: The fastest way to get a Lightning baseline! A collection of tasks for fast prototyping, baselining, finetuning and solving problems with deep learning

  • Bolts: Pretrained SOTA Deep Learning models, callbacks and more for research and production with PyTorch Lightning and PyTorch

  • Lightning Transformers: Flexible interface for high performance research using SOTA Transformers leveraging Pytorch Lightning, Transformers, and Hydra.

@ananthsub ananthsub added feature Is an improvement or enhancement help wanted Open to be worked on design Includes a design discussion refactor labels Aug 18, 2021
@ananthsub
Copy link
Contributor Author

@kaushikb11 would this make integrating new progress bars like Rich more straightforward?

@kaushikb11 kaushikb11 self-assigned this Aug 19, 2021
@awaelchli
Copy link
Contributor

Agree with process_position, nobody uses it and I believe the documentation is slightly misleading.
I am not sure about the refresh rate though. Removing that may introduce a major inconvenience for users.

@awaelchli awaelchli added the deprecation Includes a deprecation label Aug 19, 2021
@ananthsub
Copy link
Contributor Author

ananthsub commented Aug 19, 2021

Agree with process_position, nobody uses it and I believe the documentation is slightly misleading.
I am not sure about the refresh rate though. Removing that may introduce a major inconvenience for users.

Trainer(callbacks=[ProgressBar(refresh_rate=X)])

vs

Trainer(progress_bar_refresh_rate=X)

This doesn't seem inconvenient for me. Especially if users already have other callbacks they use, this is very natural. Wdyt?

Motivation:

  • Keep Trainer constructor specific to what the Trainer actually needs
  • Allow the ProgressBar to add more args without the Trainer needing to be aware or set defaults (and even allow us to test out multiple progress bar implementations without needing to shoehorn this arg into all of them)
  • Consistency with other callbacks: customizations for frameworks provided by the callback like ModelCheckpoint and EarlyStopping are expected to be instantiated this way. Why do we treat ProgressBar differently?
  • Long-term health & expectations for more components: The trainer needing to hold arguments for each of these isn't scaling

@ananthsub ananthsub changed the title Deprecate process_position and progress_bar_refresh_rate from the Trainer constructor Deprecate process_position from the Trainer constructor Aug 28, 2021
@ananthsub ananthsub added the good first issue Good for newcomers label Aug 28, 2021
@tchaton tchaton added the let's do it! approved to implement label Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Includes a deprecation design Includes a design discussion feature Is an improvement or enhancement good first issue Good for newcomers help wanted Open to be worked on let's do it! approved to implement refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants