Dist tests #3515
Dist tests #3515
Conversation
allennlp/run.py
Outdated
| # means that it only runs if allennlp is being run as a binary. | ||
| # Without this, if another library that allennlp uses has set the start method, | ||
| # this line would crash. | ||
| torch.multiprocessing.set_start_method("spawn", force=True) |
There was a problem hiding this comment.
Hmm, this is interesting. What are the implications if another library has set a different start method and we force past that? I'm having trouble finding the docs for force. Pytorch seems to import it from Python which is here: https://github.com/python/cpython/blob/master/Lib/multiprocessing/context.py#L241 But the docs don't mention it and the code doesn't help much either... (Docs : https://docs.python.org/3/library/multiprocessing.html#multiprocessing.set_start_method doesn't mention it.)
There was a problem hiding this comment.
I'm guessing you have some other source that explains why it's necessary?
There was a problem hiding this comment.
It appears that setting this is some kind of global python state, which I don't understand - e.g running this sequentially in two separate python interpreters one after another, closing inbetween, results in it crashing. I'm not super sure about the implications, but using distributed with pytorch does require us to use this spawn method.
There was a problem hiding this comment.
Actually on my linux machine, even opening a python interpreter causes it to already be set.
There was a problem hiding this comment.
Oof, this is nasty. What it sounds like is that some orphaned processes are keeping alive the process that Python creates to spawn the workers (or something in that vein). See https://docs.python.org/3/library/multiprocessing.html#contexts-and-start-methods.
* 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
Add some tests, fix a couple of minor bugs:
torch.multiprocessing.set_start_methodneeds to be called with aforceflag, otherwise in some environments it will raise an error.I tested this on my multi-gpu machine, will run on CI too.