pylops icon indicating copy to clipboard operation
pylops copied to clipboard

Homogenize imports of optional libraries

Open mrava87 opened this issue 2 years ago • 3 comments

Motivation

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

  • cupy-cusignal: via the backend.py module with availability check performed in deps.py
  • others: direct import in files with try-except pattern

Proposed solution

  • Add *_enabled variables for all other optional dependences in deps.py
  • Add a function *_import for each optional dependences in deps.py that parses error messages during imports of those optional dependencies. This function looks like this (eg for devito):
def devito_import(module):
    if devito_enabled:
        try:
            import devito
        except Exception as e:
            devito_message = f"Failed to import devito (error:{e})."
    else:
        devito_message = (
            f"Devito package not installed. In order to be able to use"
            f'the {module} module run "pip install devito".'
        )
    return devito_message

and use it in combination with *_enabled when import this library, eg:

if deps.devito_enabled:
    import devito

    from examples.seismic import AcquisitionGeometry, Model
    from examples.seismic.acoustic import AcousticWaveSolver
else:
    devito = None
    devito_message = deps.devito_import("twoway")

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?

mrava87 avatar Aug 27 '22 22:08 mrava87

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?

cako avatar Aug 28 '22 04:08 cako

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 avatar Aug 28 '22 11:08 mrava87

This is currently not implemented in kirchhoff and lsm

mrava87 avatar Dec 24 '22 19:12 mrava87