io factory registration python

[thewtex]

Load IO and FFT factories at runtime based on the itk-module.cmake
FACTORY_NAMES metadata, as is done with C++ static binary
initialization.

This enables run-time discovery of all Python IO or FFT implementations.
Some may be added with remote module Python packages.

This also addresses Slicer's use case where Slicer is performing
initialization of IO factories with static initialization. Only
one static binary initialization is intended to be supported per
application. Do not perform static binary initialization in the Python
modules.

FFTImageFilter consolidation was finalized, and the required
corresponding VnlFFTImageFilterFactory and FFTWFFTImageFilterFactory
classes created and wrapped.

Add the missing required TransformIO wrapping.

[thewtex]

Closes #2233

[tbirdso]

Thanks for putting this together @thewtex . A couple of questions on the FFT factory changes. I have not tried running Python changes but adding factory information to module info looks sound.

[dzenanz]

Mostly looks good, but I have the same questions as Tom.

[tbirdso]

Note that Python wrapping is passing locally on Linux but I am seeing errors when trying to read an image:

>>> import itk
>>> itk.auto_progress(2)
>>> itk.imread('/home/tom/repos/spectra-pixel.mhd',pixel_type=itk.VariableLengthVector[itk.F])
Loading ITKPyBase... done
Loading ITKCommon... done
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/tom/builds/ITK-build-dev/Wrapping/Generators/Python/itk/support/extras.py", line 970, in imread
    load_factories('ImageIO')
  File "/home/tom/builds/ITK-build-dev/Wrapping/Generators/Python/itk/support/base.py", line 331, in load_factories
    module = importlib.import_module(f"itk.{module_name}Python")
  File "/home/tom/anaconda3/envs/venv-itkdev/lib/python3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1014, in _gcd_import
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 671, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 783, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/tom/builds/ITK-build-dev/Wrapping/Generators/Python/itk/ITKIOBMPPython.py", line 13, in <module>
    from . import _ITKIOBMPPython
ImportError: PyCapsule_Import could not import module "_ITKCommonPython"

Doing an itk.force_load() before imread allows image reading to succeed.

>>> itk.force_load()
...
Loading ITKIOTransformInsightLegacy... done
Loading ITKVTK... done
>>> itk.imread('/home/tom/repos/spectra-pixel.mhd',pixel_type=itk.VariableLengthVector[itk.F])
Running itkImageFileReaderVIF2... done
<itk.itkVectorImagePython.itkVectorImageF2; proxy of <Swig Object of type 'itkVectorImageF2 *' at 0x7faf4dae2a80> >

[thewtex]

/home/tom/anaconda3/envs/venv-itkdev

Do we still get the error with a fresh venv?

[blowekamp]

Sorry to chime in here if this is not the right place. In SimpleITK I have had problems with the "magic" static being removed in some cases by the linker when that particular compilation unit is not directly used. So I needed to add the magic statics in many places... anding bloat. I think a manual and explicit registration would be better for the SimpleITK situation.

[tbirdso]

Hi @blowekamp, this patch is intended to remove magic static / static initialization mechanisms in favor of explicit registration at module load time in ITK Python. C++ projects consuming ITK are unaffected and continue to rely on initialization with magic statics called via FactoryRegistrationManager configurable headers. I would favor tracking any changes to the latter in a separate PR after this PR is completed.

[tbirdso]

@thewtex

Do we still get the error with a fresh venv?

Yes, I created a new environment and copied over WrapITK.pth, and am seeing the same error:

>>> itk.imread(r'spectra-pixel-image.mhd',pixel_type=itk.VariableLengthVector[itk.F])
Loading ITKPyBase... done
Loading ITKCommon... done
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/tom/builds/ITK-build-dev/Wrapping/Generators/Python/itk/support/extras.py", line 970, in imread
    load_factories('ImageIO')
  File "/home/tom/builds/ITK-build-dev/Wrapping/Generators/Python/itk/support/base.py", line 331, in load_factories
    module = importlib.import_module(f"itk.{module_name}Python")
  File "/home/tom/anaconda3/envs/venv-itktest/lib/python3.8/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1014, in _gcd_import
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 671, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 843, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/tom/builds/ITK-build-dev/Wrapping/Generators/Python/itk/ITKIOBMPPython.py", line 13, in <module>
    from . import _ITKIOBMPPython
ImportError: PyCapsule_Import could not import module "_ITKCommonPython"

Does importlib.import_module handle recursive module dependencies? Perhaps we need itk_load_swig_module here instead?

[thewtex]

Yes, I created a new environment and copied over WrapITK.pth, and am seeing the same error:

This is odd because the test suite passed for me locally, and even here ITKCommon appears to load successfully before the traceback.

Loading ITKCommon... done

ImportError: PyCapsule_Import could not import module "_ITKCommonPython"

[tbirdso]

This is odd because the test suite passed for me locally

I've seen the same issue in both Linux and Windows local environment so far, unsure of the root cause. The Python test suite is failing on Linux but it looks like only FFT factory registrations are found to have issues there. I assume that imread and imwrite are covered under CI for Python builds but have not confirmed yet.

and even here ITKCommon appears to load successfully before the traceback.

Agreed, which is why I'm uncertain if importlib.import_module is the right method here or not. Maybe an issue with what namespace we are trying to load from/into?

EDIT: @thewtex Would you please try running the minimum reproducible example to load an image with imread? Would be interested to see the output on your system with itk.auto_progress on.

[tbirdso]

Some additional notes from investigation so far:

• Explicitly loading ITKIOImageBase before calling imread causes factory registration + scalar image reading to succeed, though vector image reading still fails:

>>> import itk
>>> itk.ImageIOBase # force ITKIOImageBase module to load w/ dependencies and factories
<class 'itk.ITKIOImageBaseBasePython.itkImageIOBase'>
>>> len(itk.ObjectFactoryBase.GetRegisteredFactories())
20
>>> itk.imread(r'\path\to\test_image.mha')
<itk.itkImagePython.itkImageF3; proxy of <Swig Object of type 'itkImageF3 *' at 0x000002BC12E92630> >
>>> >>> itk.imread(r'spectra-pixel-image.mhd',pixel_type=itk.VariableLengthVector[itk.F])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\extras.py", line 1016, in imread
    raise RuntimeError("No ImageIO is registered to handle the given file.")
RuntimeError: No ImageIO is registered to handle the given file.

• Attempting to import a module with importlib.import_module seems to have no significant effect as itk_load_swig_module is called immediately thereafter in getattr. When importlib.import_module is removed the above procedure continues to succeed.
• Without loading ITKIOImageBase first, an error is observed in importlib.import_module as discussed previously:

>>> import itk
>>> itk.imread(r'C:\repos\unstable-us\ExternalData\test\Baseline\itkAttenuationImageFilterTestOutputBaseline.mha')
Loading itk.ITKIOBMPPython
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\extras.py", line 970, in imread
    load_factories('ImageIO')
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\base.py", line 352, in load_factories
    module = importlib.import_module(f"itk.{module_name}Python")
  File "C:\Users\tom.birdsong\Anaconda3\envs\venv-itk5\lib\importlib\__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1014, in _gcd_import
  File "<frozen importlib._bootstrap>", line 991, in _find_and_load
  File "<frozen importlib._bootstrap>", line 975, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 671, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 783, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\ITKIOBMPPython.py", line 13, in <module>
    from . import _ITKIOBMPPython
ImportError: PyCapsule_Import could not import module "_ITKCommonPython"

• Without loading ITKIOImageBase first, and with importlib.import_module removed, a different error is observed:

>>> import itk
>>> itk.imread(r'C:\repos\unstable-us\ExternalData\test\Baseline\itkAttenuationImageFilterTestOutputBaseline.mha')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\extras.py", line 970, in imread
    load_factories('ImageIO')
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\base.py", line 357, in load_factories
    factory = getattr(itk, f"{factory_class_prefix}{name}Factory")
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\lazy.py", line 139, in __getattribute__
    base.itk_load_swig_module(module, namespace)
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\base.py", line 109, in itk_load_swig_module
    itk_load_swig_module(dep, namespace)
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\base.py", line 235, in itk_load_swig_module
    init_function()
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\itkImageFileReaderPython.py", line 2324, in image_file_reader_init_docstring
    filter_class = itk.ITKIOImageBase.ImageFileReader
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\lazy.py", line 144, in __getattribute__
    base.load_module_needed_factories(module)
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\base.py", line 381, in load_module_needed_factories
    load_factories(needed_factories[name])
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\base.py", line 357, in load_factories
    factory = getattr(itk, f"{factory_class_prefix}{name}Factory")
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\lazy.py", line 143, in __getattribute__
    value = namespace[attr]
KeyError: 'BMPImageIOFactory'

The same error is observed when attempting to directly instantiate an ImageIO factory:

>>> import itk
>>> import BMPImageIO
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\lazy.py", line 139, in __getattribute__
    base.itk_load_swig_module(module, namespace)
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\base.py", line 109, in itk_load_swig_module
    itk_load_swig_module(dep, namespace)
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\base.py", line 235, in itk_load_swig_module
    init_function()
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\itkImageFileReaderPython.py", line 2324, in image_file_reader_init_docstring
    filter_class = itk.ITKIOImageBase.ImageFileReader
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\lazy.py", line 144, in __getattribute__
    base.load_module_needed_factories(module)
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\base.py", line 381, in load_module_needed_factories
    load_factories(needed_factories[name])
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\base.py", line 357, in load_factories
    factory = getattr(itk, f"{factory_class_prefix}{name}Factory")
  File "C:\repos\ITK-build-unstable\Wrapping\Generators\Python\itk\support\lazy.py", line 143, in __getattribute__
    value = namespace[attr]
KeyError: 'BMPImageIOFactory'

At this point in time I'm still unclear as to why template definitions from factory-defining modules are not being properly loaded into the global namespace. From these tracebacks it does not seem to be an issue of cyclic dependencies involving ITKCommon.

[tbirdso]

Looks like my issues were resolved by 1) removing the extra load_factories from imread and imwrite to instead rely on implicit loading when we instantiate ImageFileReader and ImageFileWriter, and 2) removing importlib.import_module to favor ITK's default module loading through itk_load_swig_module in __getattribute__. Will verify with a close eye on CI (may need to resolve conflicts first).

This does not address the FFT instantiation failures seen previously in CI, will follow up with investigation on that topic. Also, I am still seeing a failure to read vector images in the MetaImage format despite MetaImageIO being registered, will investigate what is missing there.

[tbirdso]

Rebased on master.

[thewtex]

@thewtex does this make sense to you? Please let me know if there are cases to be aware of that may not be covered by CI.

If it is working, that is good :-)

For import itk.BMPImageIO, we do not want to import all the other IO libraries as a side effect. Is this still the case?

[tbirdso]

we do not want to import all the other IO libraries as a side effect

From a design standpoint I don't see how this is possible while still achieving our desired goal of automatic factory loading. My understanding is that we have defined the problem as such:

When we load a Python module that defines a base class for override, we want to immediately also load all modules defining factory overrides of that class.

In the context of IO, this means that anytime we load ITKIOImageBase, we also want to load all modules defining ImageIO factory overrides.

By definition, itk::BMPImageIO depends on itk::ImageIOBase, therefore ITKBMPIO depends on ITKIOImageBase. To load ITKBMPIO we must first load ITKIOImageBase. When we load ITKIOImageBase we load all modules defining ImageIO factories. Therefore when we load ITKBMPIO it is necessary to also load all other ImageIO modules.

The only workaround I can see here is to start passing around the top-level module name in itk_load_swig_module recursion and adding special cases in loading logic based on what module is loading at the top level. That would get messy.

@thewtex Do you disagree? Am I misunderstanding the core problem statement here?

[thewtex]

When we load a Python module that defines a base class for override, we want to immediately also load all modules defining factory overrides of that class.

No, this is not correct. We want classes and functions that need the factories to have the factories loaded, e.g. ImageFileReader. We do not necessarily want to load all classes when a base class is loaded, e.g. ImageIOBase.

From a user perspective this also matches previous behavior with magic static initialization, where simply loading the module DLL would result in initialization overrides being available without any extra effort on the user's part.

Not quite -- the magic statics were in the DLL, but the magic statics loaded previously are were associated with the use of ImageFileReader, not ImageIOBase.

ITK/Modules/IO/ImageBase/include/itkImageFileReader.h

Lines 199 to 201 in cef0b65

	#if defined ITK_IMAGEIO_FACTORY_REGISTER_MANAGER || defined ITK_IO_FACTORY_REGISTER_MANAGER
	# include "itkImageIOFactoryRegisterManager.h"
	#endif

start passing around the top-level module name in itk_load_swig_module recursion and adding special cases in loading logic based on what module is loading at the top level. That would get messy.

Yes that is messy, please do not do that.

We could load the factories on a class-dependency basis (as was originally implemented). Or, they could be loaded on a module-level basis (closest to the current state).

[tbirdso]

@thewtex

No, this is not correct. We want classes and functions that need the factories to have the factories loaded, e.g. ImageFileReader. We do not necessarily want to load all classes when a base class is loaded, e.g. ImageIOBase.

Thanks for clarifying, this is a core issue to design of this PR and we certainly need to have a consensus on what we are actually trying to achieve with these changes in order to move forward.

With that said, I disagree with both this premise and the assessment of previous functionality. I would suggest that for this PR, which is really intended as a bug fix for breaking static initializations, we focus foremost on maintaining the previous user experience and then come back to the discussion of goals -- i.e. the PR should pass on the merits of maintaining "how it is" and then if needed we address "how it should be" in a subsequent patch.

The status quo (how it is)

In the status quo, with static initialization, factories are loaded via modules, not classes. For evidence we can show that in pip-installed itk==v5.3rc03, loading a base class such as ImageIOBase causes factories to be registered:

Python 3.8.5 (default, Sep  3 2020, 21:29:08) [MSC v.1916 64 bit (AMD64)] :: Anaconda, Inc. on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import itk
>>> itk.__version__
'5.3.0'        # using ITK 5.3rc03
>>> itk.auto_progress(2)
>>> itk.Array                         # Show that factory list is empty after loading ITKIOBase dependencies
Loading ITKPyBase... done
Loading ITKCommon... done
<itkTemplate itk::Array>
>>> len(itk.ObjectFactoryBase.GetRegisteredFactories())
0
>>> itk.ImageIOBase                  # Show that factory list is populated after loading itk.ImageIOBase
Loading ITKIOImageBase... done
<class 'itk.ITKIOImageBaseBasePython.itkImageIOBase'>
>>> len(itk.ObjectFactoryBase.GetRegisteredFactories())
20
>>> itk.ImageFileReader           # Show that calling a function that relies on overrides does not load additional modules
<itkTemplate itk::ImageFileReader>

What we can take away from this:

1. Factory registration is tied to modules. Although ImageIOBase does not by itself require factories and does not include a factory register manager in its header, 20 ImageIO factories are registered when we load itk.ImageIOBase. We can preserve this functionality in the given PR.
2. Factory overrides in the status quo are not accomplished through module loads. Despite the factory list being expanded, no ImageIO modules are actually loaded in Python via the ITKIOImageBase import, I assume because everything is handled on the C++ side with static initialization. However, we cannot preserve this functionality in this PR, because we fundamentally need to create and register factory objects in Python at init as an alternative to static initialization.

TL;DR: Under the status quo, when a module is loaded, all factory overrides for any base classes it contains are also loaded.

The proposal (how it should be)

The proposed change to the status quo is this:

We want classes and functions that need the factories to have the factories loaded, e.g. ImageFileReader. We do not necessarily want to load all classes when a base class is loaded, e.g. ImageIOBase.

I would argue that this is fundamentally opposed to what we are trying to accomplish with factory overrides at initialization. The idea of initialization overrides is that a consumer can program to the base class's abstract interface with no knowledge of or concern for any factory methods happening behind the scenes. When we introduce a class-based dependency, such as "ImageFileReader should load ImageIO factories before running", we violate that principle by introducing knowledge of factory mechanisms into the classes themselves. This is of relatively little impact for IO readers/writers because they are tightly joined to a central idea, IO. However, when we start looking at other classes such as FFT image filters it's unclear what classes should start to register FFT factories. Clearly any ITK class that consumes an FFT image filter needs that filter to have a backend. Do we start to add a configuration value to each class indicating that it needs to load FFT factories first? What about if we start adding other factory overrides, such as FFTDiscreteGaussianImageFilter? The maintenance burden begins to explode.

This problem is solved cleanly if we couple default factory initialization with Python module loading, as is proposed in this PR. All possible overrides are registered so that they are available as needed.

One weakness to this argument, unfortunately, is that it violates the idea of lazy loading to a certain degree. However, as discussed earlier, there's not a great alternative to maintain the status quo in the first place since we are trying to move away from static initialization and into a Python-level solution.

I appreciate any thoughts or feedback you have here. Thanks.

EDIT: From the last line of your comment it sounds like this may not be a major point of contention. Sorry if I misunderstood your position 😅

[thewtex]

A few things to keep in mind:

• The purpose of the PR is multi-fold
  • Avoid crashing in Slicer due to static initialization
  • Enable registration at runtime of IO format implementations from remote modules
  • Enable registration at runtime of filter implementations for FFT's from remote modules and for other filters
  • Fix FFT registration for Python and CMake
• The purpose of the PR is neither to maintain or deviate from the status-quo :-)
• The image IO factories were registered when the itk.ImageIOBase import occurred. However, this is an unintentional side-effect of ImageIOBase and ImageFileReader living in the same module.
• IO format use of factories is a different type of use case than base class / derived specialized implementation and it should not necessarily be handled in the same way.

From the last line of your comment it sounds like this may not be a major point of contention.

Yes, I agree that the current proposed approach of registering the factories when the module with the base classes is a reasonable approach! Most of the additional factory use will likely be base/derived implementation. We can avoid additional configuration to indicate the factory needs to be loaded. This is a reasonable tradeoff.

[tbirdso]

@thewtex I agree with your points. Thanks for your patience 😄

I will resolve the macOS failure and then the PR will be ready for any additional reviews + merge.

[tbirdso]

CI is green 🎉 Squashing fixups.

Pinging @jcfr @brad-t-moore @dzenanz for review

[thewtex]

LGTM!!

[tbirdso]

Squashed commits. CI was green from previous run before squash so I am proceeding with merge.

[jcfr]

Outstanding 💯 Thanks everyone for working on this 🙏

To confirm that Slicer is effectively able to import itk, would you have wheels available somewhere we could try to install ?

[dzenanz]

We don't have wheels, yet. @thewtex I guess we should tag RC4 and make wheels?

[thewtex]

I can make a few custom wheels for Slicer before RC4. @jcfr what is the Python version you want? 3.9?

[jcfr]

We are indeed using 3.9 (3.9.10 to be specific).

Thanks 🙏

[SimonRit]

I can make a few custom wheels for Slicer before RC4.

Would it be possible to have access to these wheels? I wonder if it would solve SimonRit/RTK#483... I'll be patient otherwise!

[thewtex]

Voila: https://drive.google.com/file/d/1ZUnX9Uzmbykl1I8oWgPiGVECrmGB_cCw/view?usp=sharing

[SimonRit]

Thanks! Unfortunately, SimonRit/RTK#483 is reproduced with these packages and the latest RTK CI package. I'm reopening the issue...