Miscellaneous updates

[ZviBaratz]

Getting a little acquainted with the codebase and proposing a few fixes and updates.

[ZviBaratz]

4c1d755 resolves #899.

[pep8speaks]

Hello @ZviBaratz, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. 🍻 To test for issues locally, pip install flake8 and then run flake8 niworkflows.

Comment last updated at 2021-02-26 10:11:22 UTC

[ZviBaratz]

@oesteban @effigies I'm taking a lot of liberty introducing new development dependencies and tidying up whatever I can. There are quite a few "aesthetic" changes I'm proposing (e.g. using isort, creating a messages module to store all the long strings to reduce clutter, standardizing all the caption symbols in .rst files, etc.,), I genuinely hope I'm not imposing my ideas on your project and these changes do in fact constitute a contribution and not the opposite.

[oesteban]

Thanks much for this effort. I left a few comments, but this is very much needed.

[oesteban]

Suggested change

	from mriqc.utils.bids import collect_bids_data
	from mriqc.utils.misc import reorder_csv
	from .bids import collect_bids_data
	from .misc import reorder_csv

Why absolute imports here?

[effigies]

PEP 8:

Absolute imports are recommended, as they are usually more readable and tend to be better behaved (or at least give better error messages) if the import system is incorrectly configured (such as when a directory inside a package ends up on sys.path):

import mypkg.sibling
from mypkg import sibling
from mypkg.sibling import example

However, explicit relative imports are an acceptable alternative to absolute imports, especially when dealing with complex package layouts where using absolute imports would be unnecessarily verbose:

from . import sibling
from .sibling import example

Standard library code should avoid complex package layouts and always use absolute imports.

Implicit relative imports should never be used and have been removed in Python 3.

On this advice, I've generally been moving toward absolute imports when I update code.

[oesteban]

Hmmm, that would mandate to run unit tests after installing or explicitly manipulating the PYTHONPATH, correct?

I'm not convinced it is more readable and given that the recommendation is pretty soft, I honestly would rather keep the relative imports.

On the plus side, relative imports help black not to reorder the imports wrongly, alphabetically -- which is another PEP8 recommendation.

[ZviBaratz]

Hmmm, that would mandate to run unit tests after installing or explicitly manipulating the PYTHONPATH, correct?

If I understand correctly then no, it shouldn't require that, but in any case let me know whatever you decide, of course I have no problem changing it back if you think it's better.

[ZviBaratz]

autodoc might require adding sys.path.insert(0, os.path.abspath("../../")), but considering the documentation seems to be building successfully, it doesn't seem to be an issue.

[oesteban]

why absolute imports?

[oesteban]

absolute imports

[oesteban]

absolute import

[ZviBaratz]

Another question regarding import statements - there are many scattered lazy imports. Again, to the best of my understanding, except for some particular edge-cases and/or custom plug-in functionality, this is bad practice. Should I refactor them to appear at the top of the modules?

[oesteban]

Nope, those lazy imports respond to a design problem of Nipype and the Linux compilation of cPython.

1. The problem of cPython: Python never ever clears up memory on Linux, you might reduce a bit with large objects that are removed from the heap (those are garbage-collected) but loaded modules stay on the stack (in VM). In general, this decision is not problematic at all - however in HPC systems and others where the overcommitting policies and/or cgroups impose some limitations to memory utilization then this becomes important. If you run a multiprocessing code on Linux, you'll easily use 850 MB per process (with a very early and clean implementation, maybe you narrow it down to 550MB). With long applications spinning up many processes, the base memory footprint builds up quite a little (even using forkserver and killing workers after running one task, as fMRIPrep and MRIQC do).
2. Nipype: we run nipype in multiprocessing mode (so we do have the problem above), and nipype loads a humongous amount of modules which build up pretty quickly. In Linux, all those imports make for a big footprint, and with thousands of nodes in the compute graph that have to fork (bc in Linux you generally don't spawn) well, you definitely reach memory limitations (typically virtual memory, so at least you don't get trapped into an infinite swapping).

So, although they are annoying to read, our tendency is going more and more towards these lazy imports.

[ZviBaratz]

@oesteban Thank you! I really appreciate the clear explanation. No problem, lazy imports it is.

Also, it seems I managed to break something already - mriqc_wf in the CLI's run module raises a ModuleNotFoundError for the common module I refactored into two separate interface modules. It seems to have something to do with nipype's caching mechanism trying to load some pickled file, is either of you familiar with this?

[ZviBaratz]

Looking at the logs, the execution of niworkflows.interfaces.bids.ReadSidecarJSON is failing because it tries to read the cached results but can't due to the changes in the general structure of the files (common module having been refactored). Is there any easy way to run the tests (once?) without nipype's caching? Could changing the hash_method in nipype's configuration file help?
I realize refactoring the common module is probably not the highest priority at the moment, however, the more I understand how to introduce changes and manage the tests (etc), the more I will be able to contribute in maintaining this repo.

[ZviBaratz]

I recreated the interfaces.common module as a directory, which seems to solve the issue with the pickled results. If either of you has any thoughts regarding the exception that was raised it would still be good to know, since I'm assuming we'll encounter it again if things are moved around.

[ZviBaratz]

Let me know if there's any verdict regarding absolute imports - I think once that's resolved this PR could be merged, unless there's anything else in this "general scope" of updates that you think would best be included here.

[effigies]

Hi @ZviBaratz, is there any chance this could be split into separate PRs? With 98 files touched, it's going to be hard to find time to review this in a single go. For instance, reformatted docs would be an easy PR to review in isolation.

I don't know if @oesteban has been able to follow along more closely, and if so, I'm happy to let him finish this review off.

[ZviBaratz]

@effigies other than separating the documentation changes from the rest of the updates (which should be trivial), isolating other changes might be somewhat challenging due to the project-wide actions made to fix linting issues and standardize code formatting. I realize this makes for a somewhat troublesome review, however, if we can get this one massive PR out of the way, in the future I promise to behave.