Float and sort all imports

[ndevenish]

This patch floats all inline imports in dxtbx to the top of the file, then sorts them with isort. The imports are sorted with a custom ordering such that all cctbx_project imports are clearly separated into their own block. This has been configured in the root pyproject.toml with the aim of integrating isort into the pre-commit hook in the future. isort now runs cleanly on the repository.

The imports were floated with a custom script (in the commit "Sort and Float Imports") which ignores imports that are commented, inside an if block or inside a try block. The cases that didn't get automatically floated were then manually inspected and moved if appropriate.

There's a few highly circular imports in dxtbx that would be more complicated to remove, mainly with the ImageSet and Format modules, so I've manually reinserted them for now, with an explicit note why they are inline. There's also a couple of other cases that I manually fixed and made robust to e.g. handling missing imports like lz4, bitshuffle which aren't present in conda.

Tests run and pass on python 2 and python3 installs.

[Anthchirp]

looks alright, apart from the change in dxtbx/ext.py which I take it you are reverting right now

Good effort.

[ndevenish]

Yes, ext.py reverted in 93962b8 because it's not the right place to fiddle about with that

[phyy-nx]

This pull request increases the number of imports required to read a file (in this case a full cbf) from 407 to 439:

$ cd dials_regression/image_examples/LCLS_cspad_nexus
$ libtbx.python -v -c "import dxtbx; dxtbx.load('idx-20130301060858401.cbf')" 2> tmp1
$ grep import tmp1 | sed 's/\#.*$//' | sort -u > t1
$ wc -l t1
407 t1
$ <checkout this branch>
$ libtbx.python -v -c "import dxtbx; dxtbx.load('idx-20130301060858401.cbf')" 2> tmp2
$ grep import tmp2 | sed 's/\#.*$//' | sort -u > t2
$ wc -l t2
439 t2

Given how important reducing the number of imports on large computing clusters is, I'd like to know if there is a way we can satisfy pep8 without increasing the number of imports.

For completeness, here is the set of new imports:

$ diff --changed-group-format='%>' --unchanged-group-format='' t1 t2
import cbflib_adaptbx
import cbflib_ext
import dxtbx.sweep_filenames
import gettext
import iotbx
import iotbx.detectors
import iotbx.detectors.adsc
import iotbx.detectors.adsc_module
import iotbx.detectors.bruker
import iotbx_detectors_bruker_ext
import iotbx.detectors.cbf
import iotbx.detectors.detectorbase
import iotbx.detectors.dtrek
import iotbx.detectors.edf
import iotbx.detectors.eiger
import iotbx.detectors.eiger_minicbf
import iotbx_detectors_ext
import iotbx.detectors.hamamatsu
import iotbx.detectors.macscience
import iotbx.detectors.mar
import iotbx.detectors.marIP
import iotbx.detectors.noir
import iotbx.detectors.pilatus_minicbf
import iotbx.detectors.pilatus_slice
import iotbx.detectors.raxis
import iotbx.detectors.raxisbase
import iotbx.detectors.raxis_nonsquare
import iotbx.detectors.saturn
import libtbx.option_parser
import libtbx.test_utils
import optparse
import Queue

[graeme-winter]

Do we know that this has anything more than a trivial increase in time taken to access the data? It is noteworthy that this is not a usual way of loading data, though it is a neat way of demonstrating imports.

This also highlights to me we carry a lot of "historical baggage" around namely the iotbx.detectorbase which I don't think we really use or have ever used in DIALS. This has frequently caused us issues when dealing with new instruments.

I would finally ask if we can demonstrate that these imports are a significant fraction of the run-time of performing the full analysis of a real data set i.e. we don't (to my knowledge) scatter many thousands of jobs independently spinning up to load a single frame as your example demonstrates - we usually do more substantial analysis across multiple frames, in which case I would suggest the presence or absence of imports becomes a trivial contribution to the overall time.

Looking at this more thoroughly by performing an actual task (in this case, find_spots) we see the list of differences to be smaller:

> dxtbx.format.FormatCBFMultiTile
170a172
> dxtbx.serialize.imageset
> iotbx.detectors
> iotbx.detectors.adsc
> iotbx.detectors.adsc_minicbf
> iotbx.detectors.adsc_module
> iotbx.detectors.bruker
> iotbx_detectors_bruker_ext
> iotbx.detectors.cbf
> iotbx.detectors.detectorbase
> iotbx.detectors.dtrek
> iotbx.detectors.edf
> iotbx.detectors.eiger
> iotbx.detectors.eiger_minicbf
> iotbx_detectors_ext
> iotbx.detectors.hamamatsu
> iotbx.detectors.macscience
> iotbx.detectors.mar
> iotbx.detectors.marIP
> iotbx.detectors.noir
> iotbx.detectors.pilatus_minicbf
> iotbx.detectors.pilatus_slice
> iotbx.detectors.raxis
> iotbx.detectors.raxisbase
> iotbx.detectors.raxis_nonsquare
> iotbx.detectors.saturn
> iotbx.xds
> iotbx.xds.xds_inp
> iotbx.xds.xparm
> rstbx.cftbx.coordinate_frame_converter

and dominated by iotbx imports , and - when it comes down to it - the key point you are getting at is about timing, and it appears that the real effect on the wall clock is very modest indeed:

Old:
real    0m2.509s
user    0m1.230s
sys    0m0.481s
New:
real    0m2.452s
user    0m1.248s
sys    0m0.479s

doing

echo 3 | sudo /usr/bin/tee /proc/sys/vm/drop_caches

before running the find spots a couple of times to ensure no compilation time recorded.

So - while the number of files inspected has gone up I would assert that the real-life effect of this is close to unmeasurable when compared with performing any analysis at all, and the benefits to clearly written code outweigh these. Thanks to @Anthchirp in preparing this response.

[phyy-nx]

It's probably fine. At small scale it probably doesn't matter. Just FYI, we have had issues with imports at the large scale (thousands of cores, huge amounts of data). We've even had live processing killed by this issue, as all the cores tried to hit the python files at once. To be fair, probably no amount of slimming down the number of imports would have helped (we had to eventually distribute the filesystem metadata to the compute nodes). But because of that experience, I tend to inline my imports as much as possible.

[ndevenish]

I mean without being able to analyse the specific situation it's a guess, but that sort of thing is probably better dealt with by other methods to lighten the central load, like random delays to startup for flood control so that not every job hits at once, or zipped packages, or - as you say you solved the problem with - architecting the cluster to match the use case so you can forget the problem. Large parts of the code are pretty interdependent, so without large active efforts to disentangle modules I'm not convinced that there's much to be bought on that front.

Faster startup time for very short-lived, or user facing jobs that need to turn around and give error feedback quickly is IMO a reasonable reason to care about this, but it's probably easier to actually do that closer to the root e.g. directly in the entry module - it's a lot harder to cut down a tree if you start by pulling off every leaf.

[graeme-winter]

It’s certainly true that there are things we could do to help the job start up problem. Actually sorting the build system to produce a small number of library files containing all the object code would help, which I think would also simplify conda distribution. Thinking along the lines of libdials.so etc. Having zip files full of the python would also help, so that the overall number of file system operations required to start any program would be massively reduced. We started looking at this here then found that absolute imports helped enough that we could move on. While both of these would help in massively parallel environments fixing them up is a fair chunk of work and a long game. Possibly one worth considering but will require a long hard look at our build system at least! Anyhow it’s good we understand the sensitivities behind the problem. Sounds like this is not such a problem today though.

[phyy-nx]

👍

…

On Wed, Sep 11, 2019 at 10:34 AM Graeme Winter ***@***.***> wrote: It’s certainly true that there are things we could do to help the job start up problem. Actually sorting the build system to produce a small number of library files containing all the object code would help, which I think would also simplify conda distribution. Thinking along the lines of libdials.so etc. Having zip files full of the python would also help, so that the overall number of file system operations required to start any program would be massively reduced. We started looking at this here then found that absolute imports helped enough that we could move on. While both of these would help in massively parallel environments fixing them up is a fair chunk of work and a long game. Possibly one worth considering but will require a long hard look at our build system at least! Anyhow it’s good we understand the sensitivities behind the problem. Sounds like this is not such a problem today though. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#84?email_source=notifications&email_token=ADGY5SSUBKDN2WJAYQJ3YMTQJETXZA5CNFSM4IVUKGW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6PIZ7A#issuecomment-530484476>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADGY5SWY3ERWMNJ7U6RCOSDQJETXZANCNFSM4IVUKGWQ> .

[Anthchirp]

Minor note on import time and file operations in parallel environments:
If you are in a multiprocessing environment then (assuming a posix model) you will want to get your necessary imports done as early as possible. The reason for this is that imports before the fork() simply persist, but imports after fork() have to be done separately in each process.

[phyy-nx]

Sure, that makes sense. However most of our big work, where I think imports are an issue, is on MPI. I don't think that applies. I think every MPI rank loads its own imports.

…

On Wed, Sep 11, 2019 at 2:53 PM Markus Gerstel ***@***.***> wrote: Minor note on import time and file operations in parallel environments: If you are in a multiprocessing environment then (assuming a posix model) you will want to get your necessary imports done as early as possible. The reason for this is that imports before the fork() simply persist, but imports after fork() have to be done separately in each process. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#84?email_source=notifications&email_token=ADGY5SQRVSFGUTV66D73GFDQJFSFJA5CNFSM4IVUKGW2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6QAOAI#issuecomment-530581249>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADGY5SR6EI6PDAB3PV3SR33QJFSFJANCNFSM4IVUKGWQ> .