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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable NeptuneLogger to work with distributed_backend=ddp #1753

Conversation

pitercl
Copy link
Contributor

@pitercl pitercl commented May 7, 2020

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 #1683 (issue).

The main change in this PR is that Neptune experiment is created eagerly on NeptuneLogger instantiation. After it's created it can be retrieved in spawned Trainer process. This enables the logger to work in distrubuted trainings.

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 馃檭

@pep8speaks
Copy link

pep8speaks commented May 7, 2020

Hello @pitercl! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 馃嵒

Comment last updated at 2020-05-08 14:34:24 UTC

@mergify mergify bot requested a review from a team May 7, 2020 09:56
@codecov
Copy link

codecov bot commented May 7, 2020

Codecov Report

Merging #1753 into master will decrease coverage by 0%.
The diff coverage is 94%.

@@          Coverage Diff           @@
##           master   #1753   +/-   ##
======================================
- Coverage      88%     88%   -0%     
======================================
  Files          69      69           
  Lines        4158    4166    +8     
======================================
+ Hits         3668    3675    +7     
- Misses        490     491    +1     

@pitercl pitercl marked this pull request as ready for review May 7, 2020 12:08
@mergify
Copy link
Contributor

mergify bot commented May 7, 2020

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

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 馃

@mergify mergify bot requested a review from a team May 7, 2020 22:19
@pitercl
Copy link
Contributor Author

pitercl commented May 8, 2020

Hi @Borda, thanks for the review. Since I'm conflicting with master, what is the workflow now? Should I rebase my branch onto master? Or maybe wait for more reviews before rebasing? (from what I see I need 3 approvals)

@Borda
Copy link
Member

Borda commented May 8, 2020

Think you can rebase now =)

@pitercl pitercl force-pushed the bugfix/1683-neptune-logger-ddp-fix branch from a3d1fd4 to 9a501ff Compare May 8, 2020 14:17
@pitercl pitercl force-pushed the bugfix/1683-neptune-logger-ddp-fix branch from 9a501ff to a880802 Compare May 8, 2020 14:34
@Borda Borda added the feature Is an improvement or enhancement label May 9, 2020
@Borda Borda added this to the 0.7.6 milestone May 9, 2020
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.

pls allow defining offline folder

pytorch_lightning/loggers/neptune.py Show resolved Hide resolved
@mergify mergify bot requested a review from a team May 9, 2020 14:49
@Borda
Copy link
Member

Borda commented May 10, 2020

@pitercl pls allow edit PR...

@Borda Borda added the waiting on author Waiting on user action, correction, or update label May 10, 2020
@pitercl
Copy link
Contributor Author

pitercl commented May 10, 2020

@pitercl pls allow edit PR...

I would, but I don't know how.

I don't have the option described here. Maybe because the fork is in neptune-ai organization and not in my private account.

@mergify mergify bot requested a review from a team May 10, 2020 16:35
@Borda
Copy link
Member

Borda commented May 10, 2020

just pls add a note to changelog :]

@Borda Borda added ready PRs ready to be merged and removed waiting on author Waiting on user action, correction, or update labels May 10, 2020
@williamFalcon williamFalcon merged commit 0cb6767 into Lightning-AI:master May 10, 2020
@pitercl pitercl deleted the bugfix/1683-neptune-logger-ddp-fix branch May 10, 2020 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NeptuneLogger doesn't work with distributed_backend='ddp'
4 participants