Skip to content
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.

Sign up for GitHub

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

do logging within run_experiment() #37

Merged
merged 3 commits into from
Sep 16, 2019
Merged

do logging within run_experiment() #37

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2fd7ad2
do logging within run_experiment()
tripl3a Aug 2, 2019
9d7c8c5
Update experiment.py
tripl3a Sep 13, 2019
424888e
Merge branch 'master' into experiment_logging
Timoeller Sep 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion farm/experiment.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the PR.
@Timoeller would you now like to merge this PR despite the possible bug?

from farm.file_utils import read_config, unnestConfig

Expand Down Expand Up @@ -39,6 +39,19 @@ def load_experiments(file):


def run_experiment(args):

logger.info(
"\n***********************************************"
f"\n************* Experiment: {args.name} ************"
"\n************************************************"
)
ml_logger = MLFlowLogger(tracking_uri=args.mlflow_url)
ml_logger.init_experiment(
experiment_name=args.mlflow_experiment,
run_name=args.mlflow_run_name,
nested=args.mlflow_nested
)

validate_args(args)
directory_setup(output_dir=args.output_dir, do_train=args.do_train)
distributed = bool(args.local_rank != -1)
Expand Down
19 changes: 2 additions & 17 deletions run_all_experiments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,7 @@
# limitations under the License.
"""Downstream runner for all experiments in specified config files."""

import logging
from farm.experiment import run_experiment, load_experiments
from farm.utils import MLFlowLogger

logger = logging.getLogger(__name__)


def main():
Expand All @@ -32,19 +28,8 @@ def main():

for conf_file in config_files:
experiments = load_experiments(conf_file)
for args in experiments:
logger.info(
"\n***********************************************"
f"\n************* Experiment: {args.name} ************"
"\n************************************************"
)
ml_logger = MLFlowLogger(tracking_uri=args.mlflow_url)
ml_logger.init_experiment(
experiment_name=args.mlflow_experiment,
run_name=args.mlflow_run_name,
nested=args.mlflow_nested,
)
run_experiment(args)
for experiment in experiments:
run_experiment(experiment)


if __name__ == "__main__":
Expand Down