Conversation
Sorted import statements, minor change in log message.
The wrong version of the updated PLOT operation was accidentally committed. This one should be OK.
Removed the now obsolete `multiproceManager` class. It has been replaced by `multiprocessing.Pool`
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
Resolved merge conflict.
Two scripts that help in debugging/comparing the output of recently changed operations against a reference set. Both sets need to be generated. First run `benchmark_all.sh` on the reference (e.g. the current `master`). Next run it on the changed operations in a different directory. Finally run `compare.sh` to compare the contents of the two directories. The parset files contain inputs for each of the LoSoTo operations under test. NOTE: This directory can be removed once everything works as expected. TODO: Investigate how these tests can somehow be converted into regression tests (ran by `pytest`).
|
Note for the reviewer(s): the |
The file `compare.out` contains the difference between two runs. Note that the run times may vary with a few seconds between runs. The differences are not something to be worried about, though for very short processes (e.g. in the PLOT operation) you'll notice that the new implementation has a bit more overhead.
For fun I tried to run debug/benchmark_all.sh for myself but I don't have the required |
| return multiprocessing.cpu_count() | ||
|
|
||
|
|
||
| class multiprocManager(object): |
| ncpu = ncpu if ncpu > 0 else nproc() # use all available CPUs if ncpu is not set | ||
| logging.debug("Using %s CPU(s) for operation FLAG.", ncpu) | ||
| with multiprocessing.Pool(ncpu) as pool: | ||
| results = pool.starmap(_flag, args) |
There was a problem hiding this comment.
You mentioned in slack that the new pool implementation was a bit slower and you wondered if it was related to overhead of running the Pool. While that can be the case, I wonder if the difference was in the original queue returning results as soon as they were ready whereas the pool.starmap will return them in order once all are finished. We can test this by using imap_unordered instead of starmap.
How big were the performance differences you observed?
There was a problem hiding this comment.
You mentioned in slack that the new pool implementation was a bit slower and you wondered if it was related to overhead of running the Pool. While that can be the case, I wonder if the difference was in the original queue returning results as soon as they were ready whereas the pool.starmap will return them in order once all are finished. We can test this by using imap_unordered instead of starmap.
How big were the performance differences you observed?
Not that large. Have a look at debug/compare.out. And they can differ by a few seconds between runs. But overall the Pool seems to be slightly slower. I like your suggestion to use imap_unordered, I will try that.
There was a problem hiding this comment.
Just realized I cannot use imap_unordered as a drop-in replacement, because I need the starmap functionality. So, I would need something like starmap_unordered, which unfortunately doesn't exist. Work-around would be to write that wrapper myself.
There was a problem hiding this comment.
Fair, let's stick with the starmap. The performance loss is small and it both simplifies and fixes the potential for haning queues so still pretty good all things considered.
The input file is almost 1 GB in size. I can put it somewhere where you can download it, if you want. But this is indeed the problematic part of reproducing the results. |
|
Looks good to me! Out of curiosity, does the new implementation indeed solve the deadlock issues mentioned in #180? (I'm assuming it does, just wondering if you had a chance to check it.) |
Yes it solves the dead-lock issue. You now get a clear error message from the child process if it dies due to an exception. |
Removed the debug directory. It is no longer necessary and litters the project.
3 participants
This pull request implements the changes suggested in #180.