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

Rename ProgressBarBase to ProgressBar #11908

Closed
daniellepintz opened this issue Feb 14, 2022 · 8 comments · Fixed by #17058
Closed

Rename ProgressBarBase to ProgressBar #11908

daniellepintz opened this issue Feb 14, 2022 · 8 comments · Fixed by #17058
Labels
code quality deprecation Includes a deprecation good first issue Good for newcomers
Milestone

Comments

@daniellepintz
Copy link
Contributor

daniellepintz commented Feb 14, 2022

Proposed refactor

Class Names (conform to standard of Callback, Strategy, Accelerator)

  • Rename LightningLoggerBase to Logger
  • Rename AbstractProfiler and BaseProfiler to Profiler
  • Rename ProgressBarBase to ProgressBar

Directory Names (conform to standard of callbacks/, strategies/, accelerators/)

  • Rename profiler/ directory to profilers/

File Names (conform to standard of accelerators/accelerator.py, strategies/strategy.py)

  • Rename loggers/base.py to loggers/logger.py
  • Rename profiler/base.py to profiler/profiler.py
  • Rename callbacks/base.py to callbacks/callback.py
  • Rename loops/base.py to loops/loop.py

Motivation

Consistency in naming is important to give users and developers the cleanest and simplest experience when interacting with the code. It also makes it easier to find things in the codebase when everything conforms to the same standard.

cc @Borda @tchaton @rohitgr7 @akihironitta @justusschock @awaelchli

@daniellepintz daniellepintz changed the title Fix several inconsistencies in Lightning class names, directory names, and file names [RFC] Fix several inconsistencies in Lightning class names, directory names, and file names Feb 14, 2022
@carmocca carmocca added code quality deprecation Includes a deprecation and removed refactor labels Feb 17, 2022
@awaelchli
Copy link
Contributor

I am in favor of this. I propose to go with a longer deprecation than usual as this potentially impacts many import statements in user code. Lightning 2.0?

@adi-kmt
Copy link

adi-kmt commented Feb 27, 2022

Hi would like to work on this issue. I mean the parts that have not been done yet

@daniellepintz daniellepintz self-assigned this Mar 2, 2022
@daniellepintz daniellepintz moved this to Accepted in Lightning RFCs Mar 2, 2022
@awaelchli awaelchli added this to the 1.7 milestone Apr 25, 2022
@carmocca carmocca changed the title [RFC] Fix several inconsistencies in Lightning class names, directory names, and file names Rename ProgressBarBase to ProgressBar Jul 19, 2022
@carmocca carmocca added the good first issue Good for newcomers label Jul 19, 2022
@carmocca carmocca modified the milestones: pl:1.7, pl:future Jul 19, 2022
@haesungpyun
Copy link

Hello, I'm a newbie to open source contribution. And I'm interested in this issue. If no one has been assigned, can I work on this issue?

@akihironitta
Copy link
Contributor

Hi @haesungpyun! Thank you for showing your interest! Feel free to start working on this issue! (please note that it may take some time to merge your PR because the next 1.7 release is getting very close.)

@haesungpyun
Copy link

haesungpyun commented Jul 27, 2022

Hello, I have an problem while renaming ProgressBarBase to ProgressBar.
There's a conflict in src/pytorch_lightning/callbacks/progress/rich_progress.py in 23 line and 33 line if ProgressBarBase renamed to ProgressBar.
The code use ProgressBar from rich package progress_bar.py. (The part where the error occurred was marked with *)
How should I handle this conflict?
Thank you!

https://github.com/Lightning-AI/lightning/blob/master/src/pytorch_lightning/callbacks/progress/rich_progress.py

import pytorch_lightning as pl
from pytorch_lightning.callbacks.progress.base import ***ProgressBarBase
from pytorch_lightning.utilities.imports import _package_available

_RICH_AVAILABLE: bool = _package_available("rich") and _compare_version("rich", operator.ge, "10.2.2")

Task, Style = None, None
if _RICH_AVAILABLE:
    from rich import get_console, reconfigure
    from rich.console import RenderableType
    from rich.progress import BarColumn, Progress, ProgressColumn, Task, TaskID, TextColumn
    from rich.progress_bar import ***ProgressBar
    from rich.style import Style
    from rich.text import Text

@sparkyniner
Copy link

Hello, can I work on this issue?

@carmocca carmocca modified the milestones: pl:future, pl:1.8 Aug 19, 2022
@carmocca carmocca modified the milestones: v1.8, future Oct 13, 2022
@carmocca carmocca modified the milestones: future, 2.0 Mar 13, 2023
@carmocca
Copy link
Contributor

If this doesn't land for 2.0, it will likely not happen. Is anybody interested in gettting this done quickly?

@tshu-w
Copy link
Contributor

tshu-w commented Mar 13, 2023

@carmocca Hi, I make a quick replacement in #17058, I will check the test and doc tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality deprecation Includes a deprecation good first issue Good for newcomers
Projects
No open projects
Status: Accepted
Development

Successfully merging a pull request may close this issue.

8 participants