ENH: Support extracting DICOMs from ZIP files (and possibly other archives) by switching to use shutil.unpack_archive instead of tarfile module functionality #471

HippocampusGirl · 2020-10-27T16:29:12Z

Dear heudiconv team,

I was recently working on converting a DICOM dataset to BIDS with heudiconv. Our imaging facility sends us the DICOM files as ZIP archives.

While I could extract the ZIP files manually before running heudiconv, I thought it may be useful to have on-the-fly extraction just like for tarballs. I implemented this via the built-in Python module "zipfile".

codecov · 2020-10-27T18:00:51Z

Codecov Report

Patch coverage: 93.46% and project coverage change: +5.39 🎉

Comparison is base (d855f64) 76.13% compared to head (99b0bf6) 81.52%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #471      +/-   ##
==========================================
+ Coverage   76.13%   81.52%   +5.39%     
==========================================
  Files          37       43       +6     
  Lines        2962     3930     +968     
==========================================
+ Hits         2255     3204     +949     
- Misses        707      726      +19

Impacted Files	Coverage Δ
heudiconv/cli/monitor.py	`31.52% <ø> (ø)`
heudiconv/cli/run.py	`91.48% <ø> (ø)`
heudiconv/external/dlad.py	`97.67% <ø> (ø)`
heudiconv/heuristics/multires_7Tbold.py	`15.90% <ø> (ø)`
heudiconv/heuristics/test_reproin.py	`100.00% <ø> (ø)`
heudiconv/tests/test_queue.py	`100.00% <ø> (ø)`
heudiconv/tests/anonymize_script.py	`54.54% <33.33%> (ø)`
heudiconv/due.py	`48.00% <48.00%> (ø)`
heudiconv/tests/test_dicoms.py	`85.88% <75.00%> (-14.12%)`	⬇️
heudiconv/heuristics/reproin.py	`84.29% <77.50%> (+3.18%)`	⬆️
... and 21 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

HippocampusGirl · 2020-10-27T21:08:48Z

I just discovered that this works even without the change I uploaded. Sorry, my bad :-)

yarikoptic · 2021-02-06T17:59:06Z

interesting -- stock heudiconv doesn't work for me:

$> heudiconv -f reproin --bids -o /tmp/h --files bids_anat-T1w.zip 
INFO: Running heudiconv version 0.9.0 latest 0.9.0
INFO: Analyzing 1 dicoms
INFO: Filtering out 0 dicoms based on their filename
WARNING: Ignoring bids_anat-T1w.zip since not quite a "normal" DICOM: 'FileDataset' object has no attribute 'SeriesNumber'
INFO: Generated sequence info for 0 studies with 0 entries total
INFO: Need to process 0 study sessions

so may be there is still a need in this/such PR. Do you remember with which version it has worked?

Haven't reviewed changes but tried with this PR and above worked... so we still need it seems to me.

yarikoptic · 2021-02-06T23:10:22Z

From cursory review looks ok but needs a unittest to ensure that it works. Test could reuse existing files, zip them, and do conversion.

HippocampusGirl · 2021-02-08T17:44:25Z

Interesting, maybe some zip files work with tarfile and some do not. Or maybe I made a mistake and got things mixed up, who knows ;-)

I agree that a unit test would be important. I am currently quite busy, but I will have time to write one next week, so please stay tuned.

yarikoptic

Not sure yet how it could have worked on zip, most likely some fluke (e.g. .tar.gz was named with .zip extension or alike). So I think this enhancement has value so I will keep it live.

It will also need a unittest (produce .zip in the test code, give to function, see that does the right thing).

Meanwhile I will convert to draft. please advise if you are going to work on it further @HippocampusGirl . Cheers

heudiconv/parser.py

HippocampusGirl · 2023-02-17T15:57:31Z

Hi @yarikoptic, I think I got confused when I closed the pull request. I also agree that this is a valuable feature and have been using my changes for multiple projects.

I want to work on this some more, but cannot prioritize this right now. If you or anyone else is willing to step in, then things may be faster.

psadil · 2023-03-23T16:04:53Z

I want to work on this some more, but cannot prioritize this right now. If you or anyone else is willing to step in, then things may be faster.

Hello @HippocampusGirl, is it still the case that this feature is on hold? I would be interested in the ability to work with zip files and would be happy to bring this pull request over the finish line.

HippocampusGirl · 2023-03-23T19:33:42Z

Hi @psadil, That would be wonderful!

psadil · 2023-03-23T21:15:43Z

Hi @psadil, That would be wonderful!

Great!

I wonder what's going to be a good way to contribute code. I have this potential solution that builds on the pull request's initial commit, d076a07. Do I submit a pull request into HippocampusGirl:add-zipfile-support so that it shows up in this #471 request? Can I directly add on to #471? Should I open a new pull request for the main nipy/heudiconv repo?

@yarikoptic perhaps you have guidance?

Thank you

HippocampusGirl · 2023-03-23T22:23:55Z

Hi @psadil, I have given you write access to my repository. You should be able to push into my branch now, so that it shows up as part of the pull request :-)

Enh/add zipfile support

psadil · 2023-03-29T00:25:07Z

@yarikoptic (tagging you as the original reviewer) - I think this may be ready to go. As you requested in the original review, the unpacking is now done with the shutil module, and several new tests of get_extracted_dicoms have been added.

yarikoptic

major concern -- seems to be a change in behavior in case of a mixed list of files/archives. I would prefer to keep original behavior unless there is really a strong need
some minor comments on styling etc.

heudiconv/parser.py

yarikoptic · 2023-03-29T14:04:08Z

heudiconv/parser.py

-import tarfile
-from tempfile import mkdtemp
+import shutil
+import typing


I think frequent typing. makes code too cluttered. I am ok with imports of the constructs we use as we already do elsewhere

heudiconv/dicoms.py:from typing import List, Optional

note -- ATM we do not check typing information at all. Filed #653

Sounds good. Looking forward to newer versions that allow the use of builtins like list or | :)

heudiconv/parser.py

heudiconv/tests/test_archives.py

yarikoptic · 2023-03-29T14:42:32Z

heudiconv/parser.py

+
+    Returns
+    -------
+    ItemsView[int | None, list[str]]


Unfortunately on this aspect there even were not discussion so I initiated one numpy/numpydoc#196 (comment) . I guess we are doomed to keep it as this for now here.

heudiconv/parser.py

yarikoptic · 2023-03-29T14:50:23Z

heudiconv/parser.py

+        input_list_of_unpacked or
+        all(t.endswith(_UNPACK_FORMATS) for t in fl)
+        ):
+        raise ValueError("Some but not all input files are archives.")


I think prior code was handling that just fine -- and add non-archive to None session. Is there a reason to introduce this breaking behavior? I think it might have really bad manifestations (didn't check) e.g. by pointing to a folder with some tarballs and some files to just be ignored which we ignore if not dicom later (?) in the code, etc.

yarikoptic · 2023-03-29T14:51:31Z

heudiconv/parser.py

+        typing.List[str]
+        ] = defaultdict(list)
+
+    if not isinstance(fl, list):
        fl = list(fl)


do we even need this "casting"?

I'll check. There was a similar cast before

heudiconv/heudiconv/parser.py

Lines 85 to 86 in c83e06c

if not isinstance(fl, (list, tuple)):

fl = list(fl)

but perhaps that can be dropped altogether

yeah -- that is what I wondered on why it was needed before. I wish smarter earlier us had left a comment ;)

yarikoptic · 2023-03-29T14:52:47Z

heudiconv/parser.py

+            # contained within each archive. So, files are temporarily
+            # extracted into unique tempdirs
+            # cannot use TempDirs since will trigger cleanup with __del__
+            tmpdir = tempdirs(prefix="heudiconvDCM")


make that tmpdir 0o700 right away here so no files extracted would have any moment in time when they are accessible to non-authorized user (i.e. until chmoding below)?

Good point! This seems especially relevant considering that the files are unpacked into a generic tmp directory, which could be a surprising risk to privacy.

psadil · 2023-03-29T22:11:27Z

major concern -- seems to be a change in behavior in case of a mixed list of files/archives. I would prefer to keep original behavior unless there is really a strong need

Ah, sorry. I thought that I was making something explicit that was previously only implicit. I didn't have a strong need. I'll update it to match the current behavior

some minor comments on styling etc.

Great! I'm looking through these and will revise accordingly

Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>

psadil · 2023-03-30T02:02:08Z

Okay, now revised

psadil · 2023-04-06T00:52:51Z

Checking in on this -- the code is ready for another round of review

yarikoptic

code styling is a bit odd to me, but without black I cannot insist really ;)

Otherwise I think it looks great and let's give it a try! Thank you @HippocampusGirl and @psadil !!!

github-actions · 2023-05-08T14:41:27Z

🚀 PR was released in v0.13.0 🚀

ENH: Support parsing DICOM datasets from zip files

d076a07

HippocampusGirl closed this Oct 27, 2020

yarikoptic reopened this Feb 6, 2021

yarikoptic requested changes Feb 17, 2023

View reviewed changes

heudiconv/parser.py Outdated Show resolved Hide resolved

yarikoptic marked this pull request as draft February 17, 2023 15:31

psadil added 2 commits March 22, 2023 21:35

update to latest

600ef48

allow input of zipped dicoms

6a1b449

psadil added 2 commits March 23, 2023 13:53

ensure no tests fail

d508eb0

add test for result of empty input

a296554

Merge pull request #1 from psadil/enh/add-zipfile-support

351d57a

Enh/add zipfile support

yarikoptic requested changes Mar 29, 2023

View reviewed changes

psadil and others added 7 commits March 29, 2023 18:16

simplify aggregation of unpack formats

fed0cfb

Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>

consolidate fixtures

1c21dbc

Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>

tidying

fd15602

Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>

being more consistent

bca7986

Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>

remove duplicated typing information

4743650

Co-authored-by: Yaroslav Halchenko <debian@onerussian.com>

restore ability to mix unarchived & archived files

b8c37bf

parameterize test for archive format

254dce7

ensure test does not short-circuit

99b0bf6

yarikoptic changed the title ~~ENH: Support extracting DICOMs from ZIP files~~ ENH: Support extracting DICOMs from ZIP files (and possibly other archives) by switching to use shutil.unpack_archive instead of tarfile module functionality Apr 6, 2023

yarikoptic approved these changes Apr 6, 2023

View reviewed changes

yarikoptic added the minor Increment the minor version when merged label Apr 6, 2023

yarikoptic marked this pull request as ready for review April 6, 2023 02:39

yarikoptic merged commit 3940813 into nipy:master Apr 6, 2023

github-actions bot added the released label May 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Support extracting DICOMs from ZIP files (and possibly other archives) by switching to use shutil.unpack_archive instead of tarfile module functionality #471

ENH: Support extracting DICOMs from ZIP files (and possibly other archives) by switching to use shutil.unpack_archive instead of tarfile module functionality #471

HippocampusGirl commented Oct 27, 2020

codecov bot commented Oct 27, 2020 •

edited

Loading

HippocampusGirl commented Oct 27, 2020

yarikoptic commented Feb 6, 2021

yarikoptic commented Feb 6, 2021

HippocampusGirl commented Feb 8, 2021

yarikoptic left a comment

HippocampusGirl commented Feb 17, 2023

psadil commented Mar 23, 2023

HippocampusGirl commented Mar 23, 2023

psadil commented Mar 23, 2023

HippocampusGirl commented Mar 23, 2023

psadil commented Mar 29, 2023

yarikoptic left a comment

yarikoptic Mar 29, 2023

yarikoptic Mar 29, 2023

psadil Mar 30, 2023

yarikoptic Mar 29, 2023

yarikoptic Mar 29, 2023

yarikoptic Mar 29, 2023

psadil Mar 29, 2023

yarikoptic Mar 29, 2023

yarikoptic Mar 29, 2023

psadil Mar 30, 2023

psadil commented Mar 29, 2023

psadil commented Mar 30, 2023

psadil commented Apr 6, 2023

yarikoptic left a comment

github-actions bot commented May 8, 2023

ENH: Support extracting DICOMs from ZIP files (and possibly other archives) by switching to use shutil.unpack_archive instead of tarfile module functionality #471

ENH: Support extracting DICOMs from ZIP files (and possibly other archives) by switching to use shutil.unpack_archive instead of tarfile module functionality #471

Conversation

HippocampusGirl commented Oct 27, 2020

codecov bot commented Oct 27, 2020 • edited Loading

Codecov Report

HippocampusGirl commented Oct 27, 2020

yarikoptic commented Feb 6, 2021

yarikoptic commented Feb 6, 2021

HippocampusGirl commented Feb 8, 2021

yarikoptic left a comment

Choose a reason for hiding this comment

HippocampusGirl commented Feb 17, 2023

psadil commented Mar 23, 2023

HippocampusGirl commented Mar 23, 2023

psadil commented Mar 23, 2023

HippocampusGirl commented Mar 23, 2023

psadil commented Mar 29, 2023

yarikoptic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psadil commented Mar 29, 2023

psadil commented Mar 30, 2023

psadil commented Apr 6, 2023

yarikoptic left a comment

Choose a reason for hiding this comment

github-actions bot commented May 8, 2023

codecov bot commented Oct 27, 2020 •

edited

Loading