Logging and metrics changes for distributed training #3372
Conversation
There was a problem hiding this comment.
@scarecrow1123 looking good, I caught a few things (not creating the output dir is why like 95% of the tests are failing, i'm sure)
|
Also, when you're working on PRs to this It would be good if you could revert the changes in 7afe971, because that will be in master when we rebase. It's fine this time because we created it after you started working, but e.g you can see that 7afe971 snuck in, and it will make rebasing/merging against master easier if we don't pull in diffs from there as we go. Thanks! |
|
Also, as you're going to be submitting several PRs, here are a couple of development tips for allennlp which I find useful:
https://github.com/allenai/allennlp/blob/master/doc/api/allennlp.common.tee_logger.rst Hopefully adding/removing them is self-explanatory, but getting them to build correctly can be a pain (sphinx is very fiddly). If you ever have trouble with that, let us know, because there's lots of sphinx rules i've internalised inside my head haha. |
|
One clarification, if we literally
That will ensure |
* `cleanup_logging()` is replaced with stdlib's `logging.shutdown()` * Remove `TeeLogger` and use standard log handlers * Remove `replace_cr_with_newline` and use the standard logging practice of using `logging.Filter` * Introduce `rank` and `world_size` optional attributes to support distributed workers
`logging.shutdown` is called by the logging module by default during exit which makes it unnecessary to be called from `train_model`
Apologies for the goof up. I've modified the tree to be clean thanks to @brendan-ai2 's suggestions. |
scarecrow1123
changed the title
|
@DeNeutoy Let me know if there is anything else to be changed here and if this is good to go into the branch. The changes for the next PR is underway. |
Followup PR to #3390 and #3372 to bring in distributed training support. Following are the major changes done: * Workers are spawned using `mp.spawn` and each worker creates its own `Trainer` instance * `Trainer.__init__` wraps up `self.model` with `DistributedDataParallel` * Logging and metric aggregation are already done in the previous PRs * `Vocabulary` creation in case of distributed training is done before spawning the workers and creating `Trainer` class To run distributed training, the trainer needs to have the following flag to be enabled: ```jsonnet { "trainer": { "distributed": true, // ... } } ``` TODO: * Try to reproduce comparable results and share extensive results for existing/selected models * Check if other commands like `evaluate`, `predict`, `fine-tune` works well with the new changes * Should all the callbacks need to be called from every worker in case callback based training? * Should the current dataset readers be changed to support distributed training as well?(to selectively yield data based on their rank) * Write tests - _would be happy to get some suggestions on how to write tests for this_
* Logging and metrics changes for distributed training (#3372) * Refactor logging setup to support distributed attrs * `cleanup_logging()` is replaced with stdlib's `logging.shutdown()` * Remove `TeeLogger` and use standard log handlers * Remove `replace_cr_with_newline` and use the standard logging practice of using `logging.Filter` * Introduce `rank` and `world_size` optional attributes to support distributed workers * Support for distributed training in `get_metrics` * Remove bad import * Fix duplicate log messages in stdout * Remove preemptive `logging.shutdown` `logging.shutdown` is called by the logging module by default during exit which makes it unnecessary to be called from `train_model` * Fix black formatting issues * Remove `tee_logger` references in API doc * Set log level from `ALLENNLP_DEBUG` env * Changes to `train_model` for distributed training support (#3390) * High level API changes to support distributed training * Fix flake8 error * Fix mypy error * Add docstring and misc fixes * Fix flake tests * `Trainer` changes for distributed training (#3414) Followup PR to #3390 and #3372 to bring in distributed training support. Following are the major changes done: * Workers are spawned using `mp.spawn` and each worker creates its own `Trainer` instance * `Trainer.__init__` wraps up `self.model` with `DistributedDataParallel` * Logging and metric aggregation are already done in the previous PRs * `Vocabulary` creation in case of distributed training is done before spawning the workers and creating `Trainer` class To run distributed training, the trainer needs to have the following flag to be enabled: ```jsonnet { "trainer": { "distributed": true, // ... } } ``` TODO: * Try to reproduce comparable results and share extensive results for existing/selected models * Check if other commands like `evaluate`, `predict`, `fine-tune` works well with the new changes * Should all the callbacks need to be called from every worker in case callback based training? * Should the current dataset readers be changed to support distributed training as well?(to selectively yield data based on their rank) * Write tests - _would be happy to get some suggestions on how to write tests for this_ * Dist tests (#3515) * add some tests * another test, fix incorrect type annotations * torch mp uses it's own context, no need to set default * lint * strip out old DP stuff, ensure multiple cuda devices raises err… (#3516) * strip out old DP stuff, ensure multiple cuda devices raises errors * lint * remove unused attribute * remove _cuda_devices everywhere * fixes * move distributed config up to top level * lint * clean up * rename occurences of batch_group * remove hack from find_learning_rate * fix last tests * black * use a top level distributed config * correct error for int * change up parse_cuda_devices to raise good error and be strongly typed * fix merge
As discussed at #2536 , this is the first PR to bring in
DistributedDataParallelsupport. To start with this contains refactored implementations ofprepare_global_loggingandtraining_util.get_metricsmethods.Both changes involve adding
rankandworld_sizeas optional attributes.Wrt logging, in the distributed case, each worker will log to their respective files with names
stdout_{rank}.log/stderr_{rank}.log.Logs to stdout/stderr are only from the master(rank=0) process.
tqdmgets updated with metrics aggregated across all workersWith these changes to logging,
TeeLoggerbecomes unnecessary IMO and hence I took the liberty to remove it.get_metricsdoes adist.all_reduceto aggregate all the provided metricsP.S: pytest failures are yet to be fixed