-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
fix flushing loggers #1459
fix flushing loggers #1459
Conversation
I will add a tests to cover this usecases over all loggers.... |
Codecov Report
@@ Coverage Diff @@
## master #1459 +/- ##
======================================
Coverage 91% 91%
======================================
Files 67 67
Lines 3742 3756 +14
======================================
+ Hits 3400 3415 +15
+ Misses 342 341 -1 |
Hello @Borda! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-04-14 17:19:52 UTC |
I have added parametrized test for "fit + test" and "pickle" for all loggers which are running with default |
any idea why unrelated test benchmark fails:
|
@rank_zero_only | ||
def finalize(self, status: str = None) -> None: | ||
# super().finalize(status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this intended to be commented? If so, why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added this to be the same as other loggers but then it is crashing because the accumulator is missing...
I don't feel like debugging this magic tool, there are already several other things I don't like much...
It have not been even properly tested yet
opened issue here - https://github.saobby.my.eu.orgmunity/t5/GitHub-Actions/Hanging-workflow-even-with-successful-tests/m-p/53850#M8988 UPDATE: it was the |
68dab4a
to
ba59000
Compare
83a0c1c
to
a868d50
Compare
Hi @Borda, it seems that you turned offline_mode on in NeptuneLogger by default. Was this intentional? If not, how would you like to proceed with restoring the previous default - should I throw a PR or would you like to do it? |
@Borda let’s turn the default back on |
* flushing loggers * flushing loggers * flushing loggers * flushing loggers * changelog * typo * fix trains * optimize imports * add logger test all * add logger test pickle * flake8 * fix benchmark * hanging loggers * try * del * all * cleaning
Before submitting
What does this PR do?
Fixes #1447 and #1460.
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 🙃