do logging within run_experiment() #37
Conversation
There was a problem hiding this comment.
Thanks @tripl3a for this PR. It greatly improves upon the functionality and makes things a lot clearer.
I took my time to look into the code and found a little bug. I have added detailed descriptions as a comment in the code.
farm/experiment.py
Outdated
| @@ -11,7 +11,7 @@ | |||
| from farm.data_handler.processor import Processor | |||
| from farm.train import Trainer | |||
| from farm.train import WrappedDataParallel | |||
| from farm.utils import set_all_seeds, initialize_device_settings | |||
| from farm.utils import set_all_seeds, initialize_device_settings, MLFlowLogger | |||
| from farm.utils import MLFlowLogger as MlLogger | |||
There was a problem hiding this comment.
Branden is currently unavailable so I took a deeper look into the code and actually found a potential bug.
First a general comment:
We want to always use "MlLogger" for logging. Every logger class should extend BaseLogger, Baselogger has clearly defined functions for logging. So we can swap MLflow with Tensorboard or any other logging in the future by doing
from farm.utils import XXX as MlLogger
I know this is not yet in the code. Could you adjust to this style in your PR? If you dont have time, I will take over and work on that.
Now to the bug: the MlLogger (not the MLFlowLogger) in experiment.py is actually used in initialize_optimizer(), which is called from a lot of other places. The MlLogger is called without initializing an experiment. The experiment is only initialized for MLFlowLogger. Since MlLogger and MlFlowlogger are the same (for now) it is not a problem, only when they are not it will create an error.
There was a problem hiding this comment.
Thanks for the comprehensive feedback! At the moment I don't have time, but I can take a look at it next week or the week after.
There was a problem hiding this comment.
I updated the PR.
@Timoeller would you now like to merge this PR despite the possible bug?
adjusted as suggested by @Timoeller + compatible with current FARM master branch
|
Thanks for finishing this PR @tripl3a |
So that it will also work for the example Option 1: Run experiment(s) from config.