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.

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

Tensorboard logger check if lightning_logs directory exists #1377

Merged
merged 4 commits into from Apr 7, 2020
Merged

Tensorboard logger check if lightning_logs directory exists #1377

merged 4 commits into from Apr 7, 2020

Conversation

areshytko
Copy link
Contributor

@areshytko areshytko commented Apr 4, 2020 •
edited

Before submitting

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contributor guideline, Pull Request section?
  • Did you make sure to update the docs?
  • Did you write any new necessary tests?
  • If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

Fixes #1375 .

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

@areshytko areshytko changed the title Tensorboard logger check if lightning_logs directory exists (fixes 1375) Tensorboard logger check if lightning_logs directory exists Apr 4, 2020
@mergify
Copy link
Contributor

mergify bot commented Apr 4, 2020

This pull request is now in conflict... :(

@mergify mergify bot requested a review from a team April 4, 2020 16:36
Borda
Borda requested changes Apr 4, 2020
View reviewed changes
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

add warning

pytorch_lightning/loggers/tensorboard.py Show resolved Hide resolved
pytorch_lightning/loggers/tensorboard.py Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@Borda Borda added the bug Something isn't working label Apr 4, 2020
@Borda Borda added this to the 0.7.2 milestone Apr 4, 2020
@Borda Borda requested a review from a team April 4, 2020 18:14
@codecov
Copy link

codecov bot commented Apr 4, 2020 •
edited

Codecov Report

Merging #1377 into master will decrease coverage by 0%.
The diff coverage is 50%.

@@          Coverage Diff           @@
##           master   #1377   +/-   ##
======================================
- Coverage      92%     92%   -0%     
======================================
  Files          63      63           
  Lines        3332    3336    +4     
======================================
+ Hits         3059    3061    +2     
- Misses        273     275    +2

Borda
Borda reviewed Apr 5, 2020
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@Borda Borda added the ready PRs ready to be merged label Apr 5, 2020
@mergify mergify bot requested a review from a team April 5, 2020 07:12
Borda
Borda approved these changes Apr 5, 2020
View reviewed changes
Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@Borda Borda requested a review from a team April 5, 2020 13:57
@mergify
Copy link
Contributor

mergify bot commented Apr 5, 2020

This pull request is now in conflict... :(

@Borda
Copy link
Member

Borda commented Apr 5, 2020

@awaelchli any idea why the docs fail with

/usr/local/lib/python3.7/site-packages/sphinx_paramlinks/sphinx_paramlinks.py:27: RemovedInSphinx40Warning: env.indexentries() is deprecated. Please use IndexDomain instead.

@awaelchli
Copy link
Member

Sphinx got new release 3.0, and the extensions is not compatible with that. See comment here #1379

@awaelchli awaelchli mentioned this pull request Apr 5, 2020
Merged
5 tasks
@Borda
Copy link
Member

Borda commented Apr 5, 2020

Sphinx got new release 3.0, and the extensions is not compatible with that. See comment here #1379

yeah just noted, preparing fix...

@williamFalcon
Copy link
Contributor

@Borda failing docs as well

@awaelchli
Copy link
Member

The fix for this doc fail is now in master #1382 . A rebase will do:)

@Borda Borda force-pushed the tensorboard-logger-ddp-fix--1375 branch from 2b49800 to 529065a Compare April 6, 2020 13:00
@mergify
Copy link
Contributor

mergify bot commented Apr 7, 2020

This pull request is now in conflict... :(

@williamFalcon williamFalcon merged commit 495ffbd into Lightning-AI:master Apr 7, 2020
tullie pushed a commit to tullie/pytorch-lightning that referenced this pull request Jun 7, 2020
@areshytko @williamFalcon @tullie
Tensorboard logger check if lightning_logs directory exists (Lightnin…
fd0ffb7 
…g-AI#1377)

* tensorboard logger version if root_dir not exist

* update changelog

* resolve comments

Co-authored-by: Alexander Reshytko <areshytko@Alexanders-MacBook-Pro.local>
Co-authored-by: William Falcon <waf2107@columbia.edu>
@Borda Borda modified the milestones: v0.7., v0.7.x Apr 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Borda Borda Borda approved these changes

@justusschock justusschock justusschock approved these changes

@jeffling jeffling Awaiting requested review from jeffling

@MattPainter01 MattPainter01 Awaiting requested review from MattPainter01

Assignees
No one assigned
Labels
bug Something isn't working ready PRs ready to be merged
Projects
None yet
Milestone
pl:0.7.x
Development

Successfully merging this pull request may close these issues.

Tensorboard logger error: lightning_logs directory not exists in multi-node DDP on nodes with rank != 0
5 participants
@areshytko @Borda @awaelchli @williamFalcon @justusschock