-
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
improve partial Codecov #1172
improve partial Codecov #1172
Conversation
Hello @Borda! Thanks for updating this PR.
Comment last updated at 2020-03-18 20:36:26 UTC |
Codecov Report
@@ Coverage Diff @@
## master #1172 +/- ##
======================================
+ Coverage 89% 91% +2%
======================================
Files 62 62
Lines 3164 3093 -71
======================================
- Hits 2810 2802 -8
+ Misses 354 291 -63 |
@Borda no tensorboardx... keep it pytorch summaryWriter please |
@bmartinn could you help why the Trains tests are failing? |
@Borda sure, I'll take a look. |
@Borda I just pushed a fix as a PR on the Codecov branch |
* Add TrainsLogger.set_credentials to control trains server configuration and authentication from code. Sync trains package version. Fix CI Trains tests
@bmartinn merger you PR here but the failer is the same? |
Hi @Borda, It seems like two Loggers are running simultaneously, is this possible ? Trains is all about logging all the outputs/logs/console/artifacts to the trains-server (usually self hosted). So yes, currently there is no offline mode. That said I could easily add "bypass" argument, making sure the interface works, and it will just bypass sending data to the trains-server. What do you think? |
Hmmm, anyhow it might be safer to have "bypass_mode"... |
@bmartinn that would be great! Thx |
* Add global TrainsLogger set_bypass_mode skips all external communication Co-authored-by: bmartinn <>
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.
awesome! let's keep getting the coverage up :)
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.
Looks good, but at some point we should do an audit of lines that have pragma: no-cover
. IMO some of theses should be included in coverage.
I do agree, we need to work more on tests, but with this kind of commented lines and functional visual codecov UI it is much easier to spot weakness in the package... |
* ignore in setup * show report * abs imports * abstract pass * cover loggers * doctest trains * locals * pass * revert tensorboard * use tensorboardX * revert tensorboardX * fix trains * Add TrainsLogger.set_credentials (Lightning-AI#1179) * Add TrainsLogger.set_credentials to control trains server configuration and authentication from code. Sync trains package version. Fix CI Trains tests * Add global TrainsLogger set_bypass_mode (Lightning-AI#1187) * Add global TrainsLogger set_bypass_mode skips all external communication Co-authored-by: bmartinn <> * rm some no-cov Co-authored-by: Martin.B <51887611+bmartinn@users.noreply.github.com>
What does this PR do?
Partial work on #463 increasing to 91%
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 🙃