Fix TrainsLogger doctest failing (switch to bypass mode in GitHub CI) #1379

bmartinn · 2020-04-04T18:52:59Z

Fix TrainsLogger CI doctests failing
See PR #1352 and allegroai/clearml#119

TrainsLogger run in offline (bypass-mode) automatically on GitHub CI doctest.

codecov · 2020-04-04T19:12:15Z

Codecov Report

Merging #1379 into master will decrease coverage by 1%.
The diff coverage is 50%.

@@          Coverage Diff           @@
##           master   #1379   +/-   ##
======================================
- Coverage      92%     91%   -1%     
======================================
  Files          63      63           
  Lines        3336    3321   -15     
======================================
- Hits         3061    3025   -36     
- Misses        275     296   +21

Borda

this fix is just for the two CI, but what about other users running test offline?

Borda · 2020-04-04T20:19:53Z

pytorch_lightning/loggers/trains.py

            self._trains = None
+            print('TRAINS Task: running in bypass mode')


use rather a logger from pytorch_lightning import _logger as log

I'm using "print" to match the behavior of Trains, otherwise the doctest will not match the expected behavior outside of CI testing, don't you think?

@Borda what do you think? If I'll use logging it will break the doctest again, unless we change the documentation (which I'm okay with) ...

Borda · 2020-04-04T20:20:56Z

pytorch_lightning/loggers/trains.py

+
+        :return: If True, all outside communication is skipped
+        """
+        return cls._bypass if cls._bypass is not None else \


but this will still fail if a contributor will run it locally without connection, right?

Yes working on CI failing
I'm using "print" to match the behavior of Trains, otherwise the doctest will not match the expected behavior outside of CI testing, don't you think?

Is it necessary to doctest the print statements? Perhaps it is enough to just test the instantiation? Anyway I think having the doctest is already very nice :)

@awaelchli the print is to match the expected behavior, so users know what to excepted when everything is working correctly. That said the actual text is printed (stdout) inside Trains package, so my goal was that CI process imitates the Trains prints.
@Borda what do you think? If I'll use logging it will break the doctest again, unless we change the documentation (which I'm okay with) ...

p.s.
CI finally passing, all in bypass mode

oh right, these prints are the package itself. ok sry :)

@Borda @williamFalcon again my apologies for not realizing the doctest are executed as part of the CI (after seeing it, I can definitely relate to how annoying tests failing is).
Also, while debugging the CI pipelines I found that the same doctests are executed by three different pipelines: GitHub workflow, Circle, and Drone. Not sure if it is on purpose, but just fyi...

but this will still fail if a contributor will run it locally without connection, right?

If users use the TrainsLogger without a connection the Trains package will output an error (it has to have a connection)
That said bypass mode is still supported with:

TrainsLogger.set_bypass_mode(True)

bmartinn · 2020-04-04T21:21:09Z

this fix is just for the two CI, but what about other users running test offline?

This is still supported with:

TrainsLogger.set_bypass_mode(True)

williamFalcon · 2020-04-05T15:06:34Z

This PR drops coverage though. Can we fix that?

williamFalcon

Mind making sure this doesn't drop coverage?

bmartinn · 2020-04-05T17:14:13Z

Guys, the "Build-Docs" started failing, yesterday it passed on the exact same code base.
Did something change in the CI ?

awaelchli · 2020-04-05T17:44:33Z

It looks like sphinx got a new release but one of the extensions is not ready for that.
A temporary fix is to change sphinx>=2.0 to sphinx<3.0 in docs/requirements.txt
@Borda what do you think?

jeremyjordan · 2020-04-05T17:58:48Z

@awaelchli yes i think as a general rule we should pin versions to prevent upgrading major releases without explicit review

sphinx>=2.0,<3.0

williamFalcon · 2020-04-06T12:14:26Z

@Borda how do you run the codecov bot again?

Borda · 2020-04-06T12:55:03Z

@Borda how do you run the codecov bot again?

you need to re-run Drone CI because only this one is sending the report

bmartinn · 2020-04-06T21:48:43Z

@Borda How do I do that (re-run Drone CI) ?
edit:
I pushed the final #1382 merge, maybe it will trigger it?!
edit 2:
Seems to have no effect, @Borda do you think it has something to do with the fact that when I click on the codecov PR link I get a forbidden page and a request to login (that was not there yesterday...)

bmartinn · 2020-04-07T15:51:11Z

Guys, I still can't detect any change in Codecov, is it just me? (The decrease in coverage was due to the CI bypass mode, I think I manage to tell Codecov to ignore it, but I'm uncertain...)

Edit:
@Borda , seems like the doc build fell on connection issue, I imagine rerunning it will solve it.

bmartinn · 2020-04-07T23:46:43Z

@williamFalcon I can't get the Codecov bot to update the new coverage but on the drone log it seems to stay 92% (i.e. no change)
See testing section log here

…Lightning-AI#1379) * Fix TrainsLogger doctest failing (switch to bypass mode in GitHub CI) * fix * test ci * debug * debug CI * Fix CircleCI * Fix Any CI environment switch to bypass mode * Removed debug prints * Improve code coverage * Improve code coverage * Reverted * Improve code coverage * Test CI * test codecov * Codecov fix * remove pragma Co-authored-by: bmartinn <>

mergify bot requested a review from a team April 4, 2020 18:53

Borda requested changes Apr 4, 2020

View reviewed changes

Borda added the ci Continuous Integration label Apr 4, 2020

williamFalcon requested changes Apr 5, 2020

View reviewed changes

This was referenced Apr 5, 2020

Tensorboard logger check if lightning_logs directory exists #1377

Merged

Pin sphinx version (fix for current docs build errors) #1382

Merged

bmartinn added 12 commits April 7, 2020 15:07

Fix TrainsLogger doctest failing (switch to bypass mode in GitHub CI)

df45a87

fix

9ac2a6b

test ci

94834df

debug

94f15d0

debug CI

caecf4d

Fix CircleCI

41a353e

Fix Any CI environment switch to bypass mode

5267a0b

Removed debug prints

c857ae5

Improve code coverage

497fe91

Improve code coverage

e2b1206

Reverted

801a96f

Improve code coverage

5ba689c

Borda force-pushed the fix_doctest_trainslogger branch from 4b40eef to 5ba689c Compare April 7, 2020 13:07

Test CI

0bcc4d0

bmartinn added 3 commits April 8, 2020 00:24

test codecov

d8f96ad

Codecov fix

5fc9dec

remove pragma

6517ca8

bmartinn requested a review from Borda April 7, 2020 23:45

bmartinn requested a review from williamFalcon April 7, 2020 23:47

williamFalcon merged commit fb8d085 into Lightning-AI:master Apr 8, 2020

bmartinn deleted the fix_doctest_trainslogger branch April 8, 2020 23:10

bmartinn mentioned this pull request Apr 8, 2020

pkg messages breaks doctests allegroai/clearml#119

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TrainsLogger doctest failing (switch to bypass mode in GitHub CI) #1379

Fix TrainsLogger doctest failing (switch to bypass mode in GitHub CI) #1379

bmartinn commented Apr 4, 2020

codecov bot commented Apr 4, 2020 •

edited

Loading

Borda left a comment

Borda Apr 4, 2020

bmartinn Apr 4, 2020 •

edited

Loading

Borda Apr 4, 2020

bmartinn Apr 4, 2020 •

edited

Loading

awaelchli Apr 4, 2020

bmartinn Apr 4, 2020

awaelchli Apr 4, 2020

bmartinn Apr 4, 2020

bmartinn Apr 4, 2020

bmartinn commented Apr 4, 2020

williamFalcon commented Apr 5, 2020

williamFalcon left a comment

bmartinn commented Apr 5, 2020

awaelchli commented Apr 5, 2020 •

edited

Loading

jeremyjordan commented Apr 5, 2020

williamFalcon commented Apr 6, 2020

Borda commented Apr 6, 2020

bmartinn commented Apr 6, 2020 •

edited

Loading

bmartinn commented Apr 7, 2020 •

edited

Loading

bmartinn commented Apr 7, 2020

		self._trains = None
		print('TRAINS Task: running in bypass mode')

Fix TrainsLogger doctest failing (switch to bypass mode in GitHub CI) #1379

Fix TrainsLogger doctest failing (switch to bypass mode in GitHub CI) #1379

Conversation

bmartinn commented Apr 4, 2020

codecov bot commented Apr 4, 2020 • edited Loading

Codecov Report

Borda left a comment

Choose a reason for hiding this comment

Borda Apr 4, 2020

Choose a reason for hiding this comment

bmartinn Apr 4, 2020 • edited Loading

Choose a reason for hiding this comment

Borda Apr 4, 2020

Choose a reason for hiding this comment

bmartinn Apr 4, 2020 • edited Loading

Choose a reason for hiding this comment

awaelchli Apr 4, 2020

Choose a reason for hiding this comment

bmartinn Apr 4, 2020

Choose a reason for hiding this comment

awaelchli Apr 4, 2020

Choose a reason for hiding this comment

bmartinn Apr 4, 2020

Choose a reason for hiding this comment

bmartinn Apr 4, 2020

Choose a reason for hiding this comment

bmartinn commented Apr 4, 2020

williamFalcon commented Apr 5, 2020

williamFalcon left a comment

Choose a reason for hiding this comment

bmartinn commented Apr 5, 2020

awaelchli commented Apr 5, 2020 • edited Loading

jeremyjordan commented Apr 5, 2020

williamFalcon commented Apr 6, 2020

Borda commented Apr 6, 2020

bmartinn commented Apr 6, 2020 • edited Loading

bmartinn commented Apr 7, 2020 • edited Loading

bmartinn commented Apr 7, 2020

codecov bot commented Apr 4, 2020 •

edited

Loading

bmartinn Apr 4, 2020 •

edited

Loading

bmartinn Apr 4, 2020 •

edited

Loading

awaelchli commented Apr 5, 2020 •

edited

Loading

bmartinn commented Apr 6, 2020 •

edited

Loading

bmartinn commented Apr 7, 2020 •

edited

Loading