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

Deprecated LightningLoggerBase in favor of Logger and deprecated loggers/base.py in favor of loggers/logger.py with backward compatibility #12014

Merged

Conversation

MohammedAlkhrashi
Copy link
Contributor

@MohammedAlkhrashi MohammedAlkhrashi commented Feb 19, 2022

What does this PR do?

Fixes #11971

  • Changed loggers.logger.LightningLoggerBase to loggers.logger.Logger
  • Then renamed loggers/base.py to loggers/logger.py.
  • Created a new loggers/base.py, which contains LightningLoggerBase with a deprecation warning and subclasses from Logger to support backward compatibility.
  • Changed all internal imports to support the refactoring.
  • Changed loggers/test_base.py to loggers/test_logger.py
  • Created a new loggers/test_base.py to test the deprecation warning from loggers.base.LightningLoggerBase

I still haven't updated the documentation, I first wanted to make sure that I am on the right track and if there are any necessary changes needed.

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

  • Was this discussed/approved via a GitHub issue? (not for typos and docs)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure your PR does only one thing, instead of bundling different changes together?
  • Did you make sure to update the documentation with your changes? (if necessary)
  • Did you write any new necessary tests? (not for typos and docs)
  • Did you verify new and existing tests pass locally with your changes?
  • Did you list all the breaking changes introduced by this pull request?
  • Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

  • Is this pull request ready for review? (if not, please submit in draft mode)
  • Check that all items from Before submitting are resolved
  • Make sure the title is self-explanatory and the description concisely explains the PR
  • Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

  • Had fun? 😄

@MohammedAlkhrashi MohammedAlkhrashi changed the title [WIP] Rename loggers/base.py to loggers/logger.py and changed LightningLoggerBase to Logger Rename loggers/base.py to loggers/logger.py and changed LightningLoggerBase to Logger Mar 6, 2022
@daniellepintz
Copy link
Contributor

@MohammedAlkhrashi Thanks so much for your PR! Mind fixing the merge conflicts and failing tests so we can get this merged?

@MohammedAlkhrashi
Copy link
Contributor Author

@MohammedAlkhrashi Thanks so much for your PR! Mind fixing the merge conflicts and failing tests so we can get this merged?

Alright 👍 , I will try to fix the conflicts and the failing tests by next week if that's okay.

@daniellepintz
Copy link
Contributor

Awesome, thanks!

pytorch_lightning/loggers/__init__.py Outdated Show resolved Hide resolved
pytorch_lightning/loggers/base.py Outdated Show resolved Hide resolved
Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also update occurrences in the docs?

$ grep -rnI LightningLoggerBase docs
docs/source/extensions/logging.rst:207:You can implement your own logger by writing a class that inherits from :class:`~pytorch_lightning.loggers.base.LightningLoggerBase`.
docs/source/extensions/logging.rst:212:    from pytorch_lightning.loggers.base import LightningLoggerBase, rank_zero_experiment
docs/source/extensions/logging.rst:216:    class MyLogger(LightningLoggerBase):
docs/source/common/lightning_module.rst:996:        # List of LightningLoggerBase objects
docs/source/common/trainer.rst:1755:    # List of LightningLoggerBase objects

pytorch_lightning/loggers/base.py Outdated Show resolved Hide resolved
@MohammedAlkhrashi
Copy link
Contributor Author

MohammedAlkhrashi commented Mar 31, 2022

Hi everyone, I've fixed the merge conflicts, failed tests, and updated the docs accordingly. However, I was wondering if we should also implement wrappers for other functionality in current loggers/base.py for backward compatibility? Such as DummyLogger, or LoggerCollection. Currently, only LightiningLoggerBase is backward compatible if imported directly from 'pl.loggers.base'.

Additionally, I am not sure about the deprecation test procedure exactly, should I move my deprecation tests to tests/deprecated_api? If so, I am assuming it's moved to the 1.8 files, is that correct?

@awaelchli awaelchli added logger Related to the Loggers deprecation Includes a deprecation labels Apr 2, 2022
@awaelchli awaelchli added this to the 1.7 milestone Apr 2, 2022
tests/loggers/test_base.py Outdated Show resolved Hide resolved
@awaelchli
Copy link
Contributor

Great work!

I was wondering if we should also implement wrappers for other functionality in current loggers/base.py for backward compatibility? Such as DummyLogger, or LoggerCollection.

Yes exactly, we should. The exception is LoggerCollection where you would need to just keep the existing deprecation message (because it gets removed in 1.8).

Additionally, I am not sure about the deprecation test procedure exactly, should I move my deprecation tests to tests/deprecated_api? If so, I am assuming it's moved to the 1.8 files, is that correct?

Yes, see my latest commit and the addition in test_remove_1-9.py. The same would be done for the other classes that had to move.

@MohammedAlkhrashi MohammedAlkhrashi force-pushed the rename_to_lightning_logger branch from d6d1e5e to 73c094d Compare April 3, 2022 17:34
Copy link
Contributor

@akihironitta akihironitta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution!

CHANGELOG.md Outdated Show resolved Hide resolved
@mergify mergify bot added the ready PRs ready to be merged label Apr 4, 2022
@akihironitta akihironitta enabled auto-merge (squash) April 7, 2022 12:45
@mergify mergify bot added has conflicts and removed ready PRs ready to be merged labels Apr 7, 2022
auto-merge was automatically disabled April 8, 2022 16:06

Head branch was pushed to by a user without write access

@mergify mergify bot added ready PRs ready to be merged and removed has conflicts ready PRs ready to be merged labels Apr 8, 2022
@akihironitta akihironitta enabled auto-merge (squash) April 8, 2022 17:55
@akihironitta akihironitta merged commit f3e7e91 into Lightning-AI:master Apr 8, 2022
@MohammedAlkhrashi MohammedAlkhrashi changed the title Rename loggers/base.py to loggers/logger.py and changed LightningLoggerBase to Logger Deprecated LightningLoggerBase in favor of Logger and deprecated loggers/base.py in favor of loggers/logger.py with backward compatibility Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deprecation Includes a deprecation logger Related to the Loggers ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename LightningLoggerBase to Logger
7 participants