FIX: Lingering temporary directories

[mgxd]

This is a spin-off of #466 and a continuation of #485

After first confirming the problem with a failing test, this PR:

• Removes custom temporary directory object from various function signatures
• Replaces temporary directory creation with context managers (where possible)
• Simplifies and moves TempDirs object into a dedicated module

[yarikoptic]

the fail

=================================== FAILURES ===================================
____________________________ test_smoke_convertall _____________________________
heudiconv/tests/test_heuristics.py:42: in test_smoke_convertall
    assert not list(Path(tempfile.gettempdir()).glob('heudiconv*'))
E   AssertionError: assert not [PosixPath('/tmp/heudiconvDCM42wzdx58'), PosixPath('/tmp/heudiconvDCMqb920gwc'), PosixPath('/tmp/heudiconvDCM3w5cw9bq')]
E    +  where [PosixPath('/tmp/heudiconvDCM42wzdx58'), PosixPath('/tmp/heudiconvDCMqb920gwc'), PosixPath('/tmp/heudiconvDCM3w5cw9bq')] = list(<generator object Path.glob at 0x7fa918d4df20>)
E    +    where <generator object Path.glob at 0x7fa918d4df20> = <bound method Path.glob of PosixPath('/tmp')>('heudiconv*')
E    +      where <bound method Path.glob of PosixPath('/tmp')> = PosixPath('/tmp').glob
E    +        where PosixPath('/tmp') = Path('/tmp')
E    +          where '/tmp' = <function gettempdir at 0x7fa924f2e040>()
E    +            where <function gettempdir at 0x7fa924f2e040> = tempfile.gettempdir
------------------------------ Captured log call -------------------------------
main.py                    262 INFO     Running heudiconv version 0.9.0 latest 0.9.0
main.py                    262 INFO     Running heudiconv version 0.9.0 latest 0.9.0
main.py                    291 INFO     Need to process 0 study sessions
=============== 1 failed, 55 passed, 4 skipped in 203.21 seconds ===============
The command "coverage run `which py.test` -s -v heudiconv" exited with 1.```

[codecov]

Codecov Report

Merging #512 (6e95e14) into master (f44c099) will increase coverage by 0.05%.
The diff coverage is 89.87%.

❗ Current head 6e95e14 differs from pull request most recent head f78fdef. Consider uploading reports for the commit f78fdef to get more accurate results

@@            Coverage Diff             @@
##           master     #512      +/-   ##
==========================================
+ Coverage   77.28%   77.34%   +0.05%     
==========================================
  Files          41       42       +1     
  Lines        3108     3098      -10     
==========================================
- Hits         2402     2396       -6     
+ Misses        706      702       -4     

Impacted Files	Coverage Δ
heudiconv/utils.py	90.56% <ø> (+0.36%)	⬆️
heudiconv/dicoms.py	82.11% <82.05%> (ø)
heudiconv/convert.py	86.98% <88.88%> (-0.16%)	⬇️
heudiconv/_tmpdirs.py	100.00% <100.00%> (ø)
heudiconv/main.py	77.09% <100.00%> (+0.53%)	⬆️
heudiconv/parser.py	92.63% <100.00%> (-0.08%)	⬇️
heudiconv/tests/test_heuristics.py	100.00% <100.00%> (ø)
heudiconv/tests/test_tarballs.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f44c099...f78fdef. Read the comment docs.

[yarikoptic]

all lgr. invocations were removed. I think they should be brought back at debug level

[yarikoptic]

but it seems that their use was completely replaced with tempfile. constructs, then TempDirs isn't used at all, or what am I missing?

Although I am all for mitigating lingering temp dirs, and like that we would no longer pass them in functions (not sure why that was done originally anyways), I am not sure if switching to tempfile constructs wouldn't have detrimental DX effects:

• Absent easy to trigger logging to discover details on what is going on
• Absent centralization making it impossible to disable cleanup e.g. for the purpose of debugging

so eventually the centralized control might come back but would be improved to also provide context-manager style of invocation support . So why not to do that right here and make it possible to do

with tmpdirs(prefix='dcm2niix') as tmpdir:

which would take care about logging and cleaning it up right away (unless centrally disabled)?

[mgxd]

In my ideal world, we would purge the hacky TempDirs (which is just a remnant of the old single script days) and wrap all necessary code with tempfile.TemporaryDirectory context managers. Then but doing so requires restructuring / rewriting some of the logic (it all stems from get_extracted_dicoms), and I am not up for that task.

Absent centralization making it impossible to disable cleanup e.g. for the purpose of debugging

Are we doing this now? This feels like it should be addressed in a separate PR.

[yarikoptic]

note: wouldn't be safe (fail) in case of parallel testing, not that we are doing it now but someone could with e.g. py.test xdist

[yarikoptic]

and on the other hand - since we do not use heudiconv prefix consistently across all uses - it might miss some temp dir to not be cleaned up.

I will push adding consistent heudiconv prefix (possibly changing names) to all temp dirs invocations. Not that it how it was, but I think it would be good to have all temp dirs with consistent prefix.

[mgxd]

good point - do you have any recommendations on a cleaner way to test for linger tmpdirs?

[yarikoptic]

patch TMPDIR env var for this run, point to some directory... at little fragile since AFAIK we would not know if underlying code actually uses it, so would need an extra check (would be a useful test on its own IMHO) to test that tmpdirs.add_tmpdir and tempfile.TemporaryDirectory(prefix='heudiconv-') create a directory there.

also note -- runner AFAIK runs in the same process, doesn't it? so atexit wouldn't work out, so I guess we would need manual .cleanup here?

[yarikoptic]

I will just close this one for now since haven't seen activity for awhile and IIRC @mgxd moved on to other adventures. But feel welcome to reopen or otherwise reincarnate