pyinstaller / pyinstaller-hooks-contrib

Community maintained hooks for PyInstaller.
Other
96 stars 126 forks source link

hooks: update pydicom hook for compatibility with pydicom v3.0.0 #796

Closed rokm closed 2 months ago

rokm commented 2 months ago

Update the hidden imports for renamed and extended set of plugin modules, and collect the data files that are now required at run-time due to pydicom.examples being imported during pydicom module initialization.

Closes #795.

bersbersbers commented 2 months ago

Works great for me, thanks!

bwoodsend commented 2 months ago

I think we're going to have to make a fuss upstream about those examples. It looks to me like they wouldn't exist at build time in a fresh (e.g. CI/CD) environment so the resultant application would have to redownload everything every time in onefile mode, be shot to pieces by the macOS sandbox and run afoul of permission errors if installed in a read only location. I'll have a proper look (and possible moan) tomorrow when I'm more awake.

rokm commented 2 months ago

I think we're going to have to make a fuss upstream about those examples. It looks to me like they wouldn't exist at build time in a fresh (e.g. CI/CD) environment so the resultant application would have to redownload everything every time in onefile mode, blow up the macOS sandbox and run afoul of permission errors if installed in a read only location. I'll have a proper look (and possible moan) tomorrow when I'm more awake.

On linux, the downloaded files seem to go into ~/.pydicom/data, so this does not violate the immutability of the frozen application bundle. But I agree that it is a bit of a WTF, especially since there is no reason for this import to exist.

bwoodsend commented 2 months ago

I guess that's not quite so bad then. I raised https://github.com/pydicom/pydicom/issues/2131 anyway. We'll see what they think. Is it worth putting an import pydicom into the hook to avoid the examples not packaged because the application was built on a clean CI machine scenario?

rokm commented 2 months ago

Is it worth putting an import pydicom into the hook to avoid the examples not packaged because the application was built on a clean CI machine scenario?

I don't think so - some of those files are part bundled with the package (and we collect those), while some of them are downloaded on first import (and we do not collect those, because they are not part of the package).

bwoodsend commented 2 months ago

I think we can get away without test_files?

bwoodsend commented 2 months ago

Ugh forget it. If I rm -rf my site-packages/pydicom/data/test_files then the next import pydicom doesn't fill ~/.pydicom with anything new. But reading the code seems to imply it should download them as you say.

bwoodsend commented 2 months ago

Hopefully that redundant import pydicom.examples will disappear.

rokm commented 2 months ago

Hmm, you might be right. Because they are not in the urls.json, it doesn't actually try to download them.

So the dictionary ends up like this (for frozen application with test_files patterns removed):

{'ct': None,
 'dicomdir': None,
 'jpeg2k': '/home/rok/.pydicom/data/US1_J2KR.dcm',
 'mr': None,
 'no_meta': None,
 'overlay': '/home/rok/.pydicom/data/MR-SIEMENS-DICOM-WithOverlays.dcm',
 'palette_color': '/home/rok/.pydicom/data/OBXXXX1A.dcm',
 'rgb_color': '/home/rok/.pydicom/data/US1_UNCR.dcm',
 'rt_dose': None,
 'rt_plan': None,
 'rt_ss': None,
 'waveform': None,
 'ybr_color': '/home/rok/.pydicom/data/color3d_jpeg_baseline.dcm'}
bwoodsend commented 2 months ago

Meh, it's up to you. If https://github.com/pydicom/pydicom/issues/2131 goes where I think it'll go then there's a good chance that we won't need any of them.

rokm commented 2 months ago

Let's try getting away without collecting them, then. I've pushed a separate commit so that we have this documented in (the unlikely) event that we end up having to collect them after all.