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

Replace Wandb callback's finalize with no-op #1193

Merged
merged 8 commits into from
Mar 30, 2020
Merged

Replace Wandb callback's finalize with no-op #1193

merged 8 commits into from
Mar 30, 2020

Conversation

bilal2vec
Copy link
Contributor

@bilal2vec bilal2vec commented Mar 19, 2020 •
edited by Borda
Loading

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 #1116 and #1220

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 🙃

@Borda Borda changed the title Replace Wandb callback's finalize with no-op and fix #1116 Replace Wandb callback's finalize with no-op Mar 19, 2020
@Borda Borda added the bug Something isn't working label Mar 19, 2020
@Borda Borda added this to the 0.7.2 milestone Mar 19, 2020
@mergify
Copy link
Contributor

mergify bot commented Mar 24, 2020

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

@codecov
Copy link

codecov bot commented Mar 25, 2020 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (master@2ccc745). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             master   #1193   +/-   ##
========================================
  Coverage          ?     92%           
========================================
  Files             ?      62           
  Lines             ?    3179           
  Branches          ?       0           
========================================
  Hits              ?    2911           
  Misses            ?     268           
  Partials          ?       0

@Borda
Copy link
Member

Borda commented Mar 25, 2020

@bkkaggle could you pls add a note to changelog?

@Borda Borda mentioned this pull request Mar 25, 2020
Closed
@Borda Borda linked an issue Mar 25, 2020 that may be closed by this pull request
WandbLogger does not log test results #1220
Closed
@borisdayma
Copy link
Contributor

The change to the callback looks good to me.

@Borda Borda added the ready PRs ready to be merged label Mar 25, 2020
@jeremyjordan
Copy link
Contributor

@bkkaggle can you update the corresponding tests? can't merge with failing tests

@Borda Borda removed the ready PRs ready to be merged label Mar 26, 2020
@Borda
Copy link
Member

Borda commented Mar 26, 2020

@calclavia @borisdayma can the failing platform-dependent?
it seems that this fix passes on Win but fails on Linux/Unix

@borisdayma
Copy link
Contributor

I'm actually surprised. Right now wandb.join is not called anymore and finalize does not do anything.
The test actually just needs to be removed (or we need to check the return value is just 1).

@Borda
Copy link
Member

Borda commented Mar 26, 2020

@bkkaggle could you pls implement #1193 (comment) suggestion
@borisdayma what do you recommend remove or check return value?

@borisdayma
Copy link
Contributor

I would just remove finalize method (not sure if pytorch-lightning checks the return value).

If @bkkaggle doesn't have the time I can update the PR.

@borisdayma
Copy link
Contributor

For info, here are the changes I suggest: master...borisdayma:fix-wandb

@borisdayma
Copy link
Contributor

I am currently testing proposed changes with a W&B sweep and it seems to be working well: Sweep run

I also had to change the watch method as it could not be called at the moment.

I adapted the semantic segmentation example to customize unet, gradient accumulation, etc. Still discovering pytorch-lightning features. Really impressive!
Only thing I cannot test right now is multiple GPU's…

@Borda
Copy link
Member

Borda commented Mar 28, 2020 •
edited
Loading

@borisdayma sounds cool, mind send update PR for semantic Segmentation?

@borisdayma
Copy link
Contributor

Looks good to me! I'll make separate one to update the watch method and avoid conflicts.

@bilal2vec
Copy link
Contributor Author

@Borda I removed the finalize() method and its tests and updated the changelog.

Borda
Borda approved these changes Mar 30, 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
Copy link
Member

Borda commented Mar 30, 2020

it seems that there is issue with examples, could you please rebase master

@mergify
Copy link
Contributor

mergify bot commented Mar 30, 2020

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

@mergify mergify bot requested a review from a team March 30, 2020 22:33
@williamFalcon williamFalcon merged commit a707d4b into Lightning-AI:master Mar 30, 2020
@borisdayma borisdayma mentioned this pull request Mar 30, 2020
Closed
4 tasks
alexeykarnachev pushed a commit to alexeykarnachev/pytorch-lightning that referenced this pull request Apr 3, 2020
@bilal2vec @Borda @williamFalcon
Replace Wandb callback's finalize with no-op (Lightning-AI#1193)
fa1f5b5 
* Replace Wandb callback's finalize with no-op

* Update pytorch_lightning/loggers/wandb.py

* Update wandb.py

* remove wandb logger's finalize and update tests

* update changelog

Co-authored-by: Jirka Borovec <[email protected]>
Co-authored-by: William Falcon <[email protected]>
@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

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

Successfully merging this pull request may close these issues.

WandbLogger does not log test results Wandb logger doesn't upload saved model checkpoint for final epoch
5 participants
@bilal2vec @Borda @borisdayma @jeremyjordan @williamFalcon