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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement finalize for WandbLogger #4341

Merged
merged 7 commits into from
Oct 26, 2020
Merged

Implement finalize for WandbLogger #4341

merged 7 commits into from
Oct 26, 2020

Conversation

awaelchli
Copy link
Contributor

@awaelchli awaelchli commented Oct 25, 2020

What does this PR do?

Fixes #3002

The checkpoint artifacts will be uploaded at the end of the run:
image

@awaelchli awaelchli added bug Something isn't working logger Related to the Loggers labels Oct 25, 2020
@awaelchli awaelchli added this to the 1.0.x milestone Oct 25, 2020
@pep8speaks
Copy link

pep8speaks commented Oct 25, 2020

Hello @awaelchli! Thanks for updating this PR.

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

Comment last updated at 2020-10-26 10:54:55 UTC

@awaelchli awaelchli marked this pull request as ready for review October 25, 2020 05:59
@mergify mergify bot requested a review from a team October 25, 2020 05:59
@codecov
Copy link

codecov bot commented Oct 25, 2020

Codecov Report

Merging #4341 into master will decrease coverage by 0%.
The diff coverage is 67%.

@@          Coverage Diff           @@
##           master   #4341   +/-   ##
======================================
- Coverage      93%     93%   -0%     
======================================
  Files         111     111           
  Lines        8046    8049    +3     
======================================
+ Hits         7467    7469    +2     
- Misses        579     580    +1     

@Borda Borda changed the title Implement finalize for WandbLogger [skip ci] Implement finalize for WandbLogger Oct 25, 2020
@Borda Borda added the ready PRs ready to be merged label Oct 25, 2020
@mergify mergify bot requested a review from a team October 25, 2020 09:54
@mergify mergify bot requested a review from a team October 25, 2020 10:23
@SeanNaren SeanNaren merged commit 376268f into master Oct 26, 2020
@SeanNaren SeanNaren deleted the bugfix/wandb-finish branch October 26, 2020 11:22
@borisdayma
Copy link
Contributor

borisdayma commented Oct 26, 2020

Thanks @awaelchli

I see from the commit history you probably intended to call self.logger.experiment.finish().

The reason I didn't add it in the logger is I was scared it would affect users who manually upload other charts (interpretation, etc) or files to their W&B run once training is complete.

I think we will be able to make the model upload cleaner through W&B artifacts.
I plan to look at it and propose something in the coming weeks.

@awaelchli
Copy link
Contributor Author

@borisdayma Great to have you look into it more!
I tried self.experiment.finish() but this run object didn't have the finish method. The reason I ended up with wandb.save is because I saw it in the docs and it seemed to be a good way. For now, it is guarded by self._log_model so people can keep it off if there is an issue with it and we can look into your proposal :)

@borisdayma
Copy link
Contributor

@awaelchli Thanks for the feedback. It would be easy add finish to self.experiment but I don't think it's a good idea.
For now let's keep your implementation and we'll try to find a better way with artifacts.
Thanks a lot for the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working logger Related to the Loggers ready PRs ready to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Checkpoint files location when WandbLogger
8 participants