catalystneuro / nwb-conversion-tools

Create NWB files by converting and combining neural data in proprietary formats and adding essential metadata.
https://nwb-conversion-tools.readthedocs.io/en/main/
BSD 3-Clause "New" or "Revised" License
25 stars 12 forks source link

star import from datainterfaces module #114

Closed luiztauffer closed 3 years ago

luiztauffer commented 3 years ago

Even if a package is not going to use any specfic datainterfaces from nwb-conversion-tools, it's importing all of them indirectly:

https://github.com/catalystneuro/nwb-conversion-tools/blob/d1ca90789c522824d52e4cef99fe9e096c30aa67/nwb_conversion_tools/__init__.py#L2

This makes it prone to all sorts of datainterface-specific dependency errors such as this:

File "c:\users\luiz\anaconda3\envs\env_heidelberg\lib\site-packages\heidelberg_metadata_gui\__init__.py", line 24, in init_app
    from .converter import HeidelbergNWBConverter
  File "c:\users\luiz\anaconda3\envs\env_heidelberg\lib\site-packages\heidelberg_metadata_gui\converter\__init__.py", line 1, in <module>
    from .heidelberg_converter import HeidelbergNWBConverter
  File "c:\users\luiz\anaconda3\envs\env_heidelberg\lib\site-packages\heidelberg_metadata_gui\converter\heidelberg_converter.py", line 1, in <module>
    from nwb_conversion_tools import NWBConverter
  File "c:\users\luiz\documents\github\nwb-conversion-tools\nwb_conversion_tools\__init__.py", line 2, in <module>
    from .datainterfaces import *
  File "c:\users\luiz\documents\github\nwb-conversion-tools\nwb_conversion_tools\datainterfaces\__init__.py", line 1, in <module>
    from .neuroscopedatainterface import NeuroscopeRecordingInterface, NeuroscopeSortingInterface
  File "c:\users\luiz\documents\github\nwb-conversion-tools\nwb_conversion_tools\datainterfaces\neuroscopedatainterface.py", line 116, in <module>
    class NeuroscopeMultiRecordingTimeInterface(NeuroscopeRecordingInterface):
  File "c:\users\luiz\documents\github\nwb-conversion-tools\nwb_conversion_tools\datainterfaces\neuroscopedatainterface.py", line 119, in NeuroscopeMultiRecordingTimeInterface
    RX = se.NeuroscopeMultiRecordingTimeExtractor
AttributeError: module 'spikeextractors' has no attribute 'NeuroscopeMultiRecordingTimeExtractor'

sounds unreasonable to me to keep that star import. Any thoughts @bendichter @CodyCBakerPhD ?

CodyCBakerPhD commented 3 years ago

It's built to be similar to spikeextractors, where if you import the package, you gain access to all of the extractors. If you need a specific interface, you can always just do

from nwb_conversion_tools import MyDataInterface

Import errors like that shouldn't occur so long as the correct versions are being used; we are careful not to merge an interface unless its extractor has been merged to spikeextractors.

luiztauffer commented 3 years ago

I agree they shouldn’t occur, but errors like this point to a better way of organizing the package imo. If I’m building a converter for a lab that does not have electrophysiology, there’s no need for their environment be dependent on and susceptible to errors from packages such as spikeextractors, same thing for roiextractors and etc...

btw, the error above got solved with the spikeextractors version just released now

CodyCBakerPhD commented 3 years ago

So, what you're really proposing is rather that we distinguish spikeextractors and roiextractors to load lazily like the extractor specific dependents in spikeextractors itself, since someone might want to use nwb-conversion-tools for a specific modality, and would not need other modalities that are outside their area of use?

I'd be open to that; but imagine then, if someone did want to use, for instance, an IntanRecordingInterface. They would install nwb-conversion-tools, but upon attempting to use the interface, the lazy prompts would inform them they need to get both spikeextractors and pyintan. At what point does the annoyance of multiple manual installation become too much?

luiztauffer commented 3 years ago

Indeed, the question is now beyond the star import. I guess these two would be enough (for now)? But I see your point, it could become an annoyance