PyLops / pylops

PyLops – A Linear-Operator Library for Python
https://pylops.readthedocs.io
GNU Lesser General Public License v3.0
420 stars 101 forks source link

Homogenize imports of optional libraries #430

Open mrava87 opened 2 years ago

mrava87 commented 2 years ago

Motivation

In PyLops, we have an increasing number of optional dependencies. For these dependencies 2 import patters have started to emerge:

Proposed solution

instead of the old way

try:
    import devito

    from examples.seismic import AcquisitionGeometry, Model
    from examples.seismic.acoustic import AcousticWaveSolver
except ModuleNotFoundError:
    devito = None
    devito_message = (
        "Devito package not installed. In order to be able to use"
        'the twoway module run "pip install devito".'
    )
except Exception as e:
    devito = None
    devito_message = f"Failed to import devito (error:{e})."

So far the only downside I see for the new version is that a library gets imported twice instead ones. Is that a big deal?

Opinions?

cako commented 2 years ago

Nice, I think this is definitely something we should tackle. With respect to the "double import", nothing to worry about since Python does not actually import it twice. It just skips it. To actually reimport a module one has to importlib.reload(themodule).

One thing, this pattern of try, import devito, otherwise devito = None triggers errors with mypy. One technique is described here, basically using global bools to keep track. So all in all I do agree with the design, but I would suggest skipping the devito = None altogether and just checking if deps.devito_enabled whever you want to check for it. Then one never accidentally tries to call a function of the optional module outside of a if deps.devito_enabled block. What do you think?

mrava87 commented 2 years ago

Great, I wasn't aware about the double import thingy :)

Alright, good point about the None. I actually believe that in most places we never effectively do checks of the form if devito is not Note, so the variable could be left emtpy. Let me try to make a PR following the code pattern I suggested for all optional dependencies and we can then finalize the discussion over there

mrava87 commented 1 year ago

This is currently not implemented in kirchhoff and lsm