fix `dirpath` in `log_dir` for CSVLogger #16401

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.

Sign up for GitHub

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

Jump to bottom

Merged

Borda merged 15 commits into master from fix/legacy

Jan 17, 2023

Member

Borda commented Jan 17, 2023 •

edited by github-actions bot

Loading

What does this PR do?

replace the rather more robust way of obtaining log_dir
see failing legacy testing: https://github.com/Lightning-AI/lightning/actions/runs/3938658456/jobs/6737646117

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 minor internal changes/refactors)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the 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?

Make sure you had fun coding 🙃

cc @awaelchli @Borda @Blaizzy @tchaton


          fix dirpath in log_dir

b08918a

Borda added bug logger priority: 1 labels

Borda requested review from williamFalcon, tchaton, awaelchli and carmocca as code owners

January 17, 2023 13:34

github-actions bot added the pl label

Borda requested review from lantiga, akihironitta and a team

January 17, 2023 13:34

Borda enabled auto-merge (squash)

January 17, 2023 13:35

Contributor

github-actions bot commented Jan 17, 2023 •

edited

Loading

⚡ Required checks status: All passing 🟢

Groups summary

🟢 pytorch_lightning: Tests workflow

Check ID	Status
pl-cpu (macOS-11, pytorch, 3.8, 1.11)	success	✅
pl-cpu (macOS-11, pytorch, 3.9, 1.12)	success	✅
pl-cpu (macOS-11, pytorch, 3.10, 1.13)	success	✅
pl-cpu (macOS-11, pytorch, 3.8, 1.10, oldest)	success	✅
pl-cpu (ubuntu-20.04, pytorch, 3.8, 1.10)	success	✅
pl-cpu (ubuntu-20.04, pytorch, 3.9, 1.11)	success	✅
pl-cpu (ubuntu-20.04, pytorch, 3.10, 1.12)	success	✅
pl-cpu (ubuntu-20.04, pytorch, 3.10, 1.13)	success	✅
pl-cpu (ubuntu-20.04, pytorch, 3.7, 1.10, oldest)	success	✅
pl-cpu (windows-2022, pytorch, 3.9, 1.11)	success	✅
pl-cpu (windows-2022, pytorch, 3.10, 1.12)	success	✅
pl-cpu (windows-2022, pytorch, 3.10, 1.13)	success	✅
pl-cpu (windows-2022, pytorch, 3.7, 1.10, oldest)	success	✅
pl-cpu (slow, macOS-11, pytorch, 3.7, 1.11)	success	✅
pl-cpu (slow, ubuntu-20.04, pytorch, 3.7, 1.11)	success	✅
pl-cpu (slow, windows-2022, pytorch, 3.7, 1.11)	success	✅
pl-cpu (macOS-11, lightning, 3.8, 1.13)	success	✅
pl-cpu (ubuntu-20.04, lightning, 3.8, 1.13)	success	✅
pl-cpu (windows-2022, lightning, 3.8, 1.13)	success	✅

These checks are required after the changes to .github/workflows/ci-tests-pytorch.yml, src/pytorch_lightning/callbacks/model_checkpoint.py, src/pytorch_lightning/loggers/csv_logs.py, src/pytorch_lightning/trainer/trainer.py.

🟢 pytorch_lightning: Azure GPU

Check ID	Status
pytorch-lightning (GPUs)	success	✅

These checks are required after the changes to src/pytorch_lightning/callbacks/model_checkpoint.py, src/pytorch_lightning/loggers/csv_logs.py, src/pytorch_lightning/trainer/trainer.py.

🟢 pytorch_lightning: Azure HPU

Check ID	Status
pytorch-lightning (HPUs)	success	✅

These checks are required after the changes to src/pytorch_lightning/callbacks/model_checkpoint.py, src/pytorch_lightning/loggers/csv_logs.py, src/pytorch_lightning/trainer/trainer.py.

🟢 pytorch_lightning: Azure IPU

Check ID	Status
pytorch-lightning (IPUs)	success	✅

These checks are required after the changes to src/pytorch_lightning/callbacks/model_checkpoint.py, src/pytorch_lightning/loggers/csv_logs.py, src/pytorch_lightning/trainer/trainer.py.

🟢 pytorch_lightning: Docs

Check ID	Status
make-doctest (pytorch)	success	✅
make-html (pytorch)	success	✅

These checks are required after the changes to src/pytorch_lightning/callbacks/model_checkpoint.py, src/pytorch_lightning/loggers/csv_logs.py, src/pytorch_lightning/trainer/trainer.py.

🟢 mypy

Check ID	Status
mypy	success	✅

These checks are required after the changes to src/pytorch_lightning/callbacks/model_checkpoint.py, src/pytorch_lightning/loggers/csv_logs.py, src/pytorch_lightning/trainer/trainer.py.

🟢 install

Check ID	Status
install-pkg (ubuntu-22.04, app, 3.7)	success	✅
install-pkg (ubuntu-22.04, app, 3.10)	success	✅
install-pkg (ubuntu-22.04, fabric, 3.7)	success	✅
install-pkg (ubuntu-22.04, fabric, 3.10)	success	✅
install-pkg (ubuntu-22.04, pytorch, 3.7)	success	✅
install-pkg (ubuntu-22.04, pytorch, 3.10)	success	✅
install-pkg (ubuntu-22.04, lightning, 3.7)	success	✅
install-pkg (ubuntu-22.04, lightning, 3.10)	success	✅
install-pkg (ubuntu-22.04, notset, 3.7)	success	✅
install-pkg (ubuntu-22.04, notset, 3.10)	success	✅
install-pkg (macOS-12, app, 3.7)	success	✅
install-pkg (macOS-12, app, 3.10)	success	✅
install-pkg (macOS-12, fabric, 3.7)	success	✅
install-pkg (macOS-12, fabric, 3.10)	success	✅
install-pkg (macOS-12, pytorch, 3.7)	success	✅
install-pkg (macOS-12, pytorch, 3.10)	success	✅
install-pkg (macOS-12, lightning, 3.7)	success	✅
install-pkg (macOS-12, lightning, 3.10)	success	✅
install-pkg (macOS-12, notset, 3.7)	success	✅
install-pkg (macOS-12, notset, 3.10)	success	✅
install-pkg (windows-2022, app, 3.7)	success	✅
install-pkg (windows-2022, app, 3.10)	success	✅
install-pkg (windows-2022, fabric, 3.7)	success	✅
install-pkg (windows-2022, fabric, 3.10)	success	✅
install-pkg (windows-2022, pytorch, 3.7)	success	✅
install-pkg (windows-2022, pytorch, 3.10)	success	✅
install-pkg (windows-2022, lightning, 3.7)	success	✅
install-pkg (windows-2022, lightning, 3.10)	success	✅
install-pkg (windows-2022, notset, 3.7)	success	✅
install-pkg (windows-2022, notset, 3.10)	success	✅

These checks are required after the changes to src/pytorch_lightning/callbacks/model_checkpoint.py, src/pytorch_lightning/loggers/csv_logs.py, src/pytorch_lightning/trainer/trainer.py.

Thank you for your contribution! 💜

Note
This comment is automatically generated and updates for 60 minutes every 180 seconds. If you have any other questions, contact carmocca for help.

lantiga approved these changes

View reviewed changes

Collaborator

lantiga left a comment

Great catch @Borda!


          another save_dir

6a88e1f

Borda requested a review from justusschock as a code owner

January 17, 2023 13:42

carmocca approved these changes

View reviewed changes

Contributor

carmocca left a comment

LGTM. I'd like to get @awaelchli's stamp too though

mergify bot added the ready label

carmocca and others added 2 commits

January 17, 2023 15:12

fix

6ae537a


          attrib

3a5dede

carmocca disabled auto-merge

January 17, 2023 14:13


          Merge branch 'fix/legacy' of https://github.com/PyTorchLightning/pyto…

2ebdabd

…rch-lightning into fix/legacy

carmocca reviewed

View reviewed changes

src/pytorch_lightning/callbacks/model_checkpoint.py Outdated Show resolved Hide resolved

Borda added 2 commits

January 17, 2023 15:16


          none


          import

d0e0367

Borda changed the title ~~fix dirpath in log_dir~~ fix dirpath in log_dir for CSVLogger

Borda enabled auto-merge (squash)

January 17, 2023 14:36

awaelchli approved these changes

View reviewed changes

Contributor

awaelchli left a comment

I don't understand why you get an attribute error. The attribute/property exists on the base logger and returns None by default. So this shouldn't happen at all.

Note: Underlying inconsistency will get fixed with #14188, but I haven't had the time to complete the work.

src/pytorch_lightning/trainer/trainer.py Outdated

-                          else:
-                              dirpath = self.loggers[0].log_dir
+                          logger_ = self.loggers[0]
+                          dirpath = getattr(logger_, "log_dir", getattr(logger_, "save_dir", None))

Contributor

awaelchli Jan 17, 2023

Why has log_dir precedence over save_dir? It should be the other way around to be consistent with the way it was before.

Member Author

Borda Jan 17, 2023 •

edited

Loading

👍 sorry for the confusion :)

Contributor

awaelchli Jan 17, 2023

What I'm saying is: dirpath = getattr(logger_, "save_dir", getattr(logger_, "log_dir", None))
(the order in which we access save_dir and log_dir)

because that's how it's done in the if-else above.

or

576a881

Borda added 2 commits

January 17, 2023 15:48


          reverse

4b61606

ci

91fa593

Member Author

Borda commented Jan 17, 2023

Underlying inconsistency will get fixed with #14188, but I haven't had the time to complete the work.

this would be great! it comes from creating legacy checkpoints which have not been part of normal CI so adding it here too 🦦

Borda added 5 commits

January 17, 2023 16:03


          path

ed10d8a


          export

12ea2c9


          later

875319e


          rev back

8b0b8a2


          drop

a9a86c2

Borda merged commit d46091c into master

Borda deleted the fix/legacy branch

January 17, 2023 16:31

carmocca added a commit that referenced this pull request


          Revert "fix dirpath in log_dir for CSVLogger (#16401)"

957d884

This reverts commit d46091c.

Borda added a commit that referenced this pull request


          Revert #16401 and user proper CSVLogger (#16405)

fc195b9

Co-authored-by: Jirka <[email protected]>

Borda added this to the v1.9.x milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lantiga lantiga approved these changes

williamFalcon Awaiting requested review from williamFalcon

tchaton Awaiting requested review from tchaton tchaton is a code owner

akihironitta Awaiting requested review from akihironitta

justusschock Awaiting requested review from justusschock justusschock is a code owner

+2 more reviewers

awaelchli awaelchli approved these changes

carmocca carmocca approved these changes

Labels

bug logger pl priority: 1 ready