-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Refactor codebase to use the loggers property instead of logger #11731
Closed
akashkw
wants to merge
89
commits into
Lightning-AI:master
from
akashkw:refactor/switch-to-loggers-property
Closed
Changes from all commits
Commits
Show all changes
89 commits
Select commit
Hold shift + click to select a range
2883fb2
Implement logger property, replace self.logger
akashkw 3ae0a6e
Fix small bugs
akashkw 6d917a4
Fixed initalization bug
akashkw 30d3a57
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 62c9335
Fix for case where creating a LoggerCollection of size 1
akashkw 99a4d98
Better fix for the LoggerCollection of size 1 issue
akashkw 01287ba
Change trainer.loggers from a property to an instance variable
akashkw 41cbab0
Revert all instances of trainer.loggers being used
akashkw d78d98f
Use logger param to initialize trainer.loggers
akashkw 7f809ff
Remove unneeded newlines
akashkw 179f3d4
Implement unit test for loggers property
akashkw 1ad542a
make trainer.loggers by default an empty list
akashkw c5df0ad
Update changelog
akashkw 9d564d8
fix unit test according to suggestions
akashkw 9d7c1bf
Update CHANGELOG.md
akashkw d263659
Remove unnecessary Trainer params
akashkw befad11
Remove tmpdir parameter for unit test
akashkw 4773c26
Write setters for logger and loggers
akashkw 8871c36
Unit test for setters
akashkw 65ae649
Fix bug where logger setter is called twice
akashkw df992de
Fix initialization bug with trainer test
akashkw 5e47ef4
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] d55f626
Get rid of extra DummyLogger assignment
akashkw 3debee7
Merge branch 'refactor/create-loggers-property' of github.com:akashkw…
akashkw 29778b1
flake and mypy fixes
akashkw 506e5fd
Flake fix did not commit properly
akashkw 144169b
Small changes based on suggestions
akashkw bdcbcfb
Shorten setters and update unit test
akashkw d42ce90
Move unit test to a new file
akashkw 4001efc
flake and mypy fixes
akashkw 9401fb7
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] e2be787
Refactor setter to handle special case of size 1 LoggerCollection
akashkw 1e71c40
Remove DummyLogger changes
akashkw 4418f10
Merge branch 'master' into refactor/create-loggers-property
akashkw c96534b
Commit suggestion to change None to []
akashkw 6a6e2ef
Merge branch 'master' into refactor/create-loggers-property
akashkw 7fef6fd
Decouple both setters for readability
akashkw f2a598f
Fix merge conflicts
akashkw a669a98
Fix tiny bug in trainer.loggers setter
akashkw b6d6fba
Merge branch 'master' into refactor/create-loggers-property
akashkw 8714857
Add loggers property to lightning.py
akashkw a7a47f1
update changelog
akashkw 15fa585
Add logger property to docs
akashkw 3f195be
Fix typo
akashkw c944f9a
update trainer.rst
akashkw e3312d5
correct spacing
akashkw 88b3c24
remove typing for now
akashkw 89f5037
Merge branch 'master' into refactor/create-loggers-property
akashkw 85256e7
Fix jit unused issue
akashkw 7ab3683
Fix underlines in docs
akashkw e390c0d
convert some unit tests
akashkw aeaa448
Modify more tests
akashkw c51f336
Updates based on suggestions
akashkw 820ce32
Merge branch 'refactor/create-loggers-property' into refactor/switch-…
akashkw b2bd5a1
refactor two more tests
akashkw ec99acd
More updates to docs based on suggestions
akashkw ba09e27
Create unit test for lightningmodule loggers property
akashkw bc6fd72
Replace Mock with Trainer
akashkw 17895f3
Merge branch 'refactor/create-loggers-property' into refactor/switch-…
akashkw 8278519
update all the logger tests with loggers property
akashkw 40c3c73
update the last of the unit tests to use loggers
akashkw d23cac8
Bugfixes for unit tests
akashkw f5b492d
Update types
akashkw 8800ec8
Remove list cast
akashkw 15b2644
Merge branch 'refactor/create-loggers-property' into refactor/switch-…
akashkw 5852ee2
Remove unit test for unsupported behavior
akashkw 9a29e3f
Merge branch 'refactor/create-loggers-property' into refactor/switch-…
akashkw 701d4fa
Add TODO for tests that can't be easily adapted
akashkw ac84709
Change TODO messages
akashkw 0752fb5
Handle special case for setter
akashkw 46b5ae1
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] 39a85da
Merge branch 'refactor/create-loggers-property' into refactor/switch-…
akashkw 15ae1f6
Merge branch 'refactor/create-loggers-property' into refactor/switch-…
akashkw 023802f
refactor logger_connector, batch_size_scaling, and lr_finder
akashkw 7b8a79f
update signal_connector and trainer.py
akashkw b97724c
small bugfix
akashkw d186a7f
Fix handling of special case
akashkw 68eaf65
refactor all logger files
akashkw be12180
Modify more files
akashkw 83339b2
Update docs
akashkw ffbf225
Fix flake8 error
akashkw 07707a6
Restore broken file
akashkw 70d2461
Add TODO to broken file
akashkw 5c5d87c
Review and resolve a few TODO's
akashkw d30bb02
Sanity check on a few more files
akashkw dd32d6a
rewrite log_dir
akashkw b5fa1d8
Revise unit test
akashkw 87a8eb0
flake fixes
akashkw 9b3fa48
resolve merge conflict
akashkw File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -385,8 +385,9 @@ def save_checkpoint(self, trainer: "pl.Trainer") -> None: | |
self._save_last_checkpoint(trainer, monitor_candidates) | ||
|
||
# notify loggers | ||
if trainer.is_global_zero and trainer.logger: | ||
trainer.logger.after_save_checkpoint(proxy(self)) | ||
if trainer.is_global_zero: | ||
for logger in trainer.loggers: | ||
logger.after_save_checkpoint(proxy(self)) | ||
|
||
def _should_skip_saving_checkpoint(self, trainer: "pl.Trainer") -> bool: | ||
from pytorch_lightning.trainer.states import TrainerFn | ||
|
@@ -580,18 +581,22 @@ def __resolve_ckpt_dir(self, trainer: "pl.Trainer") -> None: | |
if self.dirpath is not None: | ||
return # short circuit | ||
|
||
if trainer.logger is not None: | ||
if trainer.loggers: | ||
if trainer.weights_save_path != trainer.default_root_dir: | ||
# the user has changed weights_save_path, it overrides anything | ||
save_dir = trainer.weights_save_path | ||
else: | ||
save_dir = trainer.logger.save_dir or trainer.default_root_dir | ||
|
||
if len(trainer.loggers) == 1 and trainer.loggers[0].save_dir: | ||
save_dir = trainer.loggers[0].save_dir | ||
else: | ||
save_dir = trainer.default_root_dir | ||
# TODO: Find out we handle trainer.logger.version without LoggerCollection | ||
version = ( | ||
trainer.logger.version | ||
if isinstance(trainer.logger.version, str) | ||
else f"version_{trainer.logger.version}" | ||
) | ||
# TODO: Find out we handle trainer.logger.name without LoggerCollection | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
ckpt_path = os.path.join(save_dir, str(trainer.logger.name), version, "checkpoints") | ||
else: | ||
ckpt_path = os.path.join(trainer.weights_save_path, "checkpoints") | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -213,6 +213,7 @@ def get_standard_metrics(trainer: "pl.Trainer", pl_module: "pl.LightningModule") | |
if pl_module.truncated_bptt_steps > 0: | ||
items_dict["split_idx"] = trainer.fit_loop.split_idx | ||
|
||
# TODO: Find out we handle trainer.logger.version without LoggerCollection | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
if trainer.logger is not None and trainer.logger.version is not None: | ||
version = trainer.logger.version | ||
if isinstance(version, str): | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should we handle trainer.logger.version and trainer.logger.name here? LoggerCollection will concatenate the values together from each logger, but with trainer.loggers we no longer have that logic and that logic might not be the best approach anyways.