Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IO factory registration for Python modules #2233

Closed
dzenanz opened this issue Jan 5, 2021 · 12 comments
Closed

IO factory registration for Python modules #2233

dzenanz opened this issue Jan 5, 2021 · 12 comments
Assignees
Labels
type:Enhancement Improvement of existing methods or implementation
Milestone

Comments

@dzenanz
Copy link
Member

dzenanz commented Jan 5, 2021

Description

Automatically register remote module IOs for regular image reading and writing in Python.

Expected behavior

pip install itk-scanco

image = itk.imread("file.isq") # image is successfully read

Actual behavior

pip install itk-scanco

ImageType = itk.Image[itk.ctype('signed short'), 3]
reader = itk.ImageFileReader[ImageType].New()
imageio = itk.ScancoImageIO.New()
reader.SetImageIO(imageio)
reader.SetFileName(file_name)
reader.Update()
image = reader.GetOutput() # image is successfully read

Additional Information

Starting point for investigation:

if pixel_type:
image_IO = itk.ImageIOFactory.CreateImageIO(
io_filename, itk.CommonEnums.IOFileMode_ReadMode
)

@dzenanz dzenanz added the type:Enhancement Improvement of existing methods or implementation label Jan 5, 2021
@dzenanz dzenanz self-assigned this Jan 5, 2021
@thewtex
Copy link
Member

thewtex commented Jan 5, 2021

Perhaps in one of the other support modules we could add a register_remote_module_factories function, e.g.

_REMOTE_FACTORIES_REGISTERED = False

def register_remote_module_factories():
	if _REMOTE_FACTORIES_REGISTERED:
		return
	
	imageio_factories = filter(lambda x: 'ImageIOFactory' in x, dir(itk))
    for factory in imageio_factories:
       # Do we have to worry about duplicate registration here?
       factory.RegisterOneFactory()
	_REMOTE_FACTORIES_REGISTERED = True

Then, call register_remote_module_factories from itk.imread, itk.imwrite.

@blowekamp
Copy link
Member

Another possibility is to use ITK's dynamic loading of shared libraries which contain the "itkLoad" method for factory registration. Here is a PR related to generating the required code for factories (ImageIO only?) with CMake:

#134

@thewtex
Copy link
Member

thewtex commented Jan 5, 2021

ITK's dynamic loading of shared libraries

For performance reasons, we do not want to load all possible shared libraries.

@blowekamp
Copy link
Member

blowekamp commented Jan 5, 2021

For performance reasons, we do not want to load all possible shared libraries.

If the installation structure is such that the libraries with dynamic "itkLoad" methods are in one path/location this could be overcome. However, this may not be the best solution for the issue above, just presenting an option.

@thewtex thewtex added this to the ITK v5.2.0 milestone Feb 17, 2021
@thewtex thewtex assigned thewtex and unassigned dzenanz Feb 17, 2021
thewtex added a commit to thewtex/ITK that referenced this issue Mar 3, 2021
@thewtex thewtex modified the milestones: ITK v5.2.0, ITK v5.3.0 Mar 3, 2021
@stale
Copy link

stale bot commented Jul 2, 2021

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the status:Use_Milestone_Backlog Use "Backlog" milestone instead of label for issues without a fixed deadline label Jul 2, 2021
@dzenanz
Copy link
Member Author

dzenanz commented Jul 2, 2021

Did we decide not to do this? When custom IO is required, it needs to be specified in the imread call.

@stale stale bot removed the status:Use_Milestone_Backlog Use "Backlog" milestone instead of label for issues without a fixed deadline label Jul 2, 2021
@thewtex
Copy link
Member

thewtex commented Jul 3, 2021

We should still add this support so the io is not required to be passed to imread.

@tbirdso
Copy link
Contributor

tbirdso commented Nov 9, 2021

Revisiting this issue as it is especially relevant with FFT factories being added in #2836 which do not have an extra Reader/Writer class to typically handle default registration.

Will investigate additional code injection in igenerator.py, possibly with a new factories field in <Module>Config.py output files that forces files with relevant factories to be actively loaded where ncessary.

@tbirdso

@tbirdso
Copy link
Contributor

tbirdso commented Jan 12, 2022

Proposing that we bump this to a later milestone than ITK 5.3. This issue has merit but depends on factory synchronization in #2909 / #3071 which is not yet merged. Will require nontrivial changes to Python module loading and dependency checks, including new field(s) in igenerator.py.

Ideally if ITK remote modules register factories to the base object factory list on load, then a workaround for this is to include an itk.force_load() step to ensure all loads take place before any calls that attempt to use the object factories. However, I haven't tested this with remote modules recently and I'm not sure that it will be quite that straightforward.

This issue certainly still has merit and can be pursued in a 5.3.post or 5.4.pre milestone.

cc @dzenanz @thewtex

@dzenanz dzenanz modified the milestones: ITK 5.3.0, ITK 5.4.0 Jan 12, 2022
@thewtex thewtex assigned thewtex and unassigned tbirdso Feb 2, 2022
@tbirdso
Copy link
Contributor

tbirdso commented Feb 2, 2022

@thewtex Are you taking this one on for 5.3 or still bumped to 5.4?

@thewtex
Copy link
Member

thewtex commented Feb 2, 2022

@tbirdso looking at this for 5.3 since it impacts another issue reported in Slicer.

@thewtex thewtex modified the milestones: ITK 5.4.0, ITK 5.3.0 Feb 2, 2022
@tbirdso
Copy link
Contributor

tbirdso commented Mar 10, 2022

🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:Enhancement Improvement of existing methods or implementation
Projects
None yet
Development

No branches or pull requests

4 participants