move Trains logger from PL

[Borda]

What does this PR do?

Moving Trains logger from PL to Bolts Lightning-AI/pytorch-lightning#2384

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 🙃

[codecov-commenter]

Codecov Report

Merging #86 into master will decrease coverage by 0.61%.
The diff coverage is 71.79%.

@@            Coverage Diff             @@
##           master      #86      +/-   ##
==========================================
- Coverage   92.52%   91.91%   -0.62%     
==========================================
  Files          75       77       +2     
  Lines        3827     3944     +117     
==========================================
+ Hits         3541     3625      +84     
- Misses        286      319      +33     

Flag	Coverage Δ
#unittests	91.91% <71.79%> (-0.62%)	⬇️

Impacted Files	Coverage Δ
pl_bolts/loggers/trains.py	70.79% <70.79%> (ø)
pl_bolts/loggers/__init__.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1fdbac7...b8f40aa. Read the comment docs.

[williamFalcon]

@Borda hold off on adding trains until the fix the abc issue

[jkhenning]

Hi @williamFalcon,

If by abc issue you're referring to clearml/clearml#156 and clearml/clearml#155, please note we're waiting for your feedback on both.
To recap, we're not sure the issue is with Trains - the error you've reported is caused by graphql which is not used or required by Trains.

[bmartinn]

@Borda Any reason you guys are moving the Loggers from the main repo to the auxiliary one?

[Borda]

Borda Any reason you guys are moving the Loggers from the main repo to the auxiliary one?

@williamFalcon ^^

[williamFalcon]

we’re not moving all of them, just the ones that need to be more stable. The TRAINs logger causes all sorts of build issues and has deprecated python usage that causes warnings for our users (tje abc thing). Bolts is meant as a type of proof of concept before we bring main things into lightning.

as soon as Trains fixes these issues, then we’re happy to bring it back to lightning.

[mergify]

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

[bmartinn]

The TRAINs logger causes all sorts of build issues and has deprecated python usage that causes warnings for our users (tje abc thing).

Nothing in the Trains Issues or the Python-Ignite issues that I can find...
Also regrading the "abc thing", I suspect you are mixing packages here, it seems to stem from graphql and not the trains package (and anyhow we do not import graphql or have it in the requirements), you can see the response here & here, both waiting for @williamFalcon

I'm not aware of any other instability issues, not in the past few months at least.
Would you care to share them? I will be more than happy to fix.

[williamFalcon]

sure, @Borda can you elaborate?

The abc issues went away after we removed TRAINs. But again, users can still use it just under bolts until we fix the issues and bring the library back in.

Please install trains on python 3.7+ to see the issues

[bmartinn]

The abc issues went away after we removed TRAINs. But again, users can still use it just under bolts until we fix the issues and bring the library back in.

@williamFalcon
There was an abc warning issue (on py3.7+) but it was solved about a couple of months ago (probably 4 to 5 versions ago)...
Any chance you witnessed it on an old Trains version ?

EDIT:

Please install trains on python 3.7+ to see the issues

Tested Trains 0.15.1 on python 3.8 no warning shows :)

[NumesSanguis]

What's the status of this pull request? We're using Trains and we just moved to PyTorch Lightning (from vanilla PyTorch). We can't integrate the 2 now, because this logger has been removed from pytorch-lightning, but not yet merged here.

Due to a bug in older versions of PyTorch Lightning, we also can't downgrade to a version where Trains was still included.

[Borda]

I'm not aware of any other instability issues, not in the past few months at least.
Would you care to share them? I will be more than happy to fix.

There were a few corrections on online/offline, we have also quite a long discussion on fixing compatibility...
I have not checked the abc recently but it was traced back to Trains as you were probably required such lib
I am not much aware of graphql issue so cannot say anything about it... 🐰

[Borda]

@williamFalcon it is ready to merge it just seems that it is/was not completely tested...

[williamFalcon]

Ok, let's merge. but we need to fix the abc issue @bmartinn

[bmartinn]

@williamFalcon , it seems that I keep failing to explain :(
There is no abc issue, it was fixed 3 months ago see here
The funny thing is your PR actually reverts the fix, see here

In python 3.8 and above trying to import Iterable directly from collections will result in a warning,
This is why you first have to try to import from collections.abc and only if that fails (i.e older python version) then you import directly from collection

Make sense?