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

Homogenize imports of optional libraries #430

Open
mrava87 opened this issue Aug 27, 2022 · 3 comments
Open

Homogenize imports of optional libraries #430

mrava87 opened this issue Aug 27, 2022 · 3 comments
Assignees
Labels
Core Pulls for core PyLops library enhancement New feature or request
Projects

Comments

@mrava87
Copy link
Collaborator

mrava87 commented Aug 27, 2022

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 mrava87 added Core Pulls for core PyLops library enhancement New feature or request labels Aug 27, 2022
@cako
Copy link
Collaborator

cako commented Aug 28, 2022

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
Copy link
Collaborator Author

mrava87 commented Aug 28, 2022

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 added a commit to mrava87/pylops that referenced this issue Sep 24, 2022
cako pushed a commit that referenced this issue Oct 8, 2022
@mrava87 mrava87 closed this as completed Dec 9, 2022
@mrava87
Copy link
Collaborator Author

mrava87 commented Dec 24, 2022

This is currently not implemented in kirchhoff and lsm

@mrava87 mrava87 reopened this Dec 24, 2022
mrava87 added a commit to mrava87/pylops that referenced this issue Feb 22, 2023
cako pushed a commit that referenced this issue Feb 25, 2023
* WIP: reduce memory footprint of Kirchhoff operator

* WIP: added matvec with dynamic

* feature: added _ampsrcrec methods

* minor: homogenize variable names in kirchhoff methods

* minor: handled remaining in #430

* doc: fix outputs of _traveltime_table
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Pulls for core PyLops library enhancement New feature or request
Projects
No open projects
v2.0.0
  
Awaiting triage
Development

Successfully merging a pull request may close this issue.

2 participants