add tensorboard logger test and fix callback typo

[bw4sz]

Closes the #559 and #558 , there was a typo in callbacks only for the create_trainer(logger=), function. I added a test, but it really should have been caught somewhere else. This is high priority.

[ethanwhite]

Everything looks good. Will approve once the tests pass.

[bw4sz]

I think there is still something hapenning with the tensorboard logger and the trainer object, I have a meeting until 1pm, but i'll have look this afternoon.

[ethanwhite]

Sounds good. I'll leave this for now. Ping me when you're ready for a review.

[bw4sz]

This turned out to be a large issue and definitely warranting of 1.3.0. Our tests did not cover the logger, we had discontinued it because of the annoyances of mocking the comet_ml experiment. This turns out to be tangential to the goal, but hid the actual problem, which was quite huge. The problem is upstream of any given logger, and with pytorch lightning trainer.predict methods.

In f71eef4, I changed the predict_file methods to user trainer.predict. The virtue of this was that it made prediction much faster. We used to have to do alot of annoying workarounds, like not run evaluation that often because that function was so slow. What we didn't see was that when using a logger, like comet_ml or tensorboard, you cannot mix trainer.predict inside of an evaluation_hook. I have documented this on pytorch lightning repo. See lots of relevant conversation.

Lightning-AI/pytorch-lightning#19101

The bad news means that training with a logger has been dead for three weeks and we didn't know.
The good news is that with every error, we find ways to make the whole API better. I have refactored the evaluate routine to include a new wrapper that directly uses the predictions made during pytorch lightnings hooks, instead of re-running them separately. This will be faster. It also allowed me to move code out of main.py and into other modules, which is a long term goal to making the API more flexible, main.py should really just have wrappers and not custom code. During this I was then able to remove quite a bit of duplicate code. This is the same reason that the typo noted in #558 was able to get through the tests. No test was running this because it related to a logger during a callback. This is corrected now.

[bw4sz]

i'm tempted to force push this, but I suppose it can wait until Monday. Once @ethanwhite merges I will bumpversion minor and push to pypi.

[ethanwhite]

Looks really good. My one meaningful thing is that I found the fact that predict.predict_file() doesn't work directly on a file confusing. It sent me on a couple of tangents while reviewing which might suggest it could lead to confusing maintenance. Other than potentially renaming that there are a few docstring issues to cleanup and I think this should be ready to go it.

[bw4sz]

@ethanwhite I pushed the docstring changes, renamed the predict.predict_file to predict._predict_a_dataloader_ method to reflect it predicts a dataloader and separated predict._predict_image_ from main.predict_image.

[ethanwhite]

A couple more small cleanups and then I'll get this merged.

[ethanwhite]

Ah, that makes sense. I like the solution.