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

Only detect plugins when they need to exist in global #1036

Merged
merged 1 commit into from
Apr 3, 2019

Conversation

watsonjj
Copy link
Contributor

@watsonjj watsonjj commented Apr 3, 2019

As first described in #1035, I am having trouble with the following issue with the io plugins, specifically regarding the TargetIOR1Calibrator:

(cta) 09:27 [Jason ~/Software/ctapipe_io_targetio]  (master)  $ python -c "from ctapipe.calib.camera.r1 import CameraR1Calibrator"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Users/Jason/Software/ctapipe/ctapipe/calib/__init__.py", line 5, in <module>
    from .camera import *
  File "/Users/Jason/Software/ctapipe/ctapipe/calib/camera/__init__.py", line 8, in <module>
    from .dl1 import *
  File "/Users/Jason/Software/ctapipe/ctapipe/calib/camera/dl1.py", line 14, in <module>
    from ...image import NeighbourPeakIntegrator, NullWaveformCleaner
  File "/Users/Jason/Software/ctapipe/ctapipe/image/__init__.py", line 1, in <module>
    from .hillas import *
  File "/Users/Jason/Software/ctapipe/ctapipe/image/hillas.py", line 11, in <module>
    from ..io.containers import HillasParametersContainer
  File "/Users/Jason/Software/ctapipe/ctapipe/io/__init__.py", line 12, in <module>
    detect_and_import_io_plugins()
  File "/Users/Jason/Software/ctapipe/ctapipe/core/plugins.py", line 16, in detect_and_import_io_plugins
    return detect_and_import_plugins(prefix='ctapipe_io_')
  File "/Users/Jason/Software/ctapipe/ctapipe/core/plugins.py", line 10, in detect_and_import_plugins
    in pkgutil.iter_modules()
  File "/Users/Jason/Software/ctapipe/ctapipe/core/plugins.py", line 11, in <dictcomp>
    if name.startswith(prefix)
  File "/Users/Jason/anaconda3/envs/cta/lib/python3.7/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "/Users/Jason/Software/ctapipe_io_targetio/ctapipe_io_targetio/__init__.py", line 15, in <module>
    from .calibration import TargetIOR1Calibrator
  File "/Users/Jason/Software/ctapipe_io_targetio/ctapipe_io_targetio/calibration.py", line 1, in <module>
    from ctapipe.calib import CameraR1Calibrator
ImportError: cannot import name 'CameraR1Calibrator' from 'ctapipe.calib' (/Users/Jason/Software/ctapipe/ctapipe/calib/__init__.py)

It happens because when importing the CameraR1Calibrator, the __init__.py files end up leading to import the io plugins, which in turns tries to import the TargetIOR1Calibrator, which requires CameraR1Calibrator to be defined.

One solution I have is to be a bit more careful about when we import the plugins, instead only importing when we need them to exist in the global namespace. I.e. when we are using the factory methods.

@maxnoe
Copy link
Member

maxnoe commented Apr 3, 2019

While I think this is a good short-term fix, we should also do what I proposed in #1035 and disentangle our subpackages more.

@codecov
Copy link

codecov bot commented Apr 3, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@1498c74). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1036   +/-   ##
=========================================
  Coverage          ?   83.52%           
=========================================
  Files             ?      188           
  Lines             ?    10658           
  Branches          ?        0           
=========================================
  Hits              ?     8902           
  Misses            ?     1756           
  Partials          ?        0
Impacted Files Coverage Δ
ctapipe/io/__init__.py 100% <ø> (ø)
ctapipe/io/eventsource.py 98.11% <100%> (ø)
ctapipe/core/component.py 96.55% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1498c74...ee2cb41. Read the comment docs.

Copy link
Member

@dneise dneise left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this.
I believe, I sometimes tend to put too much 'function' into init files, which can be extremely surprising ...

So moving stuff out there might actually be the right choice here ... also longterm ...

@watsonjj
Copy link
Contributor Author

watsonjj commented Apr 3, 2019

@maxnoe Would you be happy with approving this?

@maxnoe
Copy link
Member

maxnoe commented Apr 3, 2019

A sorry, yes, the comment was meant to be the approval, missed the correct radio selection

@watsonjj watsonjj merged commit ebe9168 into cta-observatory:master Apr 3, 2019
@watsonjj watsonjj deleted the plugin_fix branch April 29, 2019 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants