9
\$\begingroup\$

We have a program with many dependencies. Because the user may not want to use all of them, the program should skip over failed imports and only raise an error if absolutely necessary. e.g. if the user tries to call that class.

I have implemented a way of doing this but I'm not sure it is as good as it could be, it certainly seems somewhat inelegant. I presume there are also some overlooked bugs.

The important file structure is:

Program:
  - engines
    - __init__.py
    - base_engine.py
    - ...
    - psi4.py
    - rdkit.py
  - run.py

The base_engine.py contains the base class Engines which all the other engines (PSI4 in psi4.py, RDKit in rdkit.py ... ) inherit from, as well as a Default class.

If any class fails to import, Default is imported instead, but with the name of the class that failed to import (see the __init__.py file).

base_engine.py:

class Engines:
    """
    Engines base class containing core information that all other engines (PSI4, etc) will have.
    """

    def __init__(self, molecule):

        self.molecule = molecule


class Default:
    """
    If there is an import error, this class replaces the class which failed to be imported.
    Then, only if initialised, an import error will be raised notifying the user of a failed call.
    """

    def __init__(self, *args, **kwargs):
        # self.name is set when the failed-to-import class is set to Default.
        raise ImportError(
            f'The class {self.name} you tried to call is not importable; '
            f'this is likely due to it not being installed.')

__init__.py:

try:
    from .psi4 import PSI4
except ImportError:
    from .base_engine import Default as PSI4
    setattr(PSI4, 'name', 'PSI4')

try:
    from .rdkit import RDKit
except ImportError:
    from .base_engine import Default as RDKit
    setattr(RDKit, 'name', 'RDKit')

psi4.py:

from Program.engines.base_engine import Engines

# Example import that would fail
import abcdefg


class PSI4(Engines):
    """
    Writes and executes input files for psi4.
    """

    def __init__(self, molecule):

        super().__init__(molecule)

    def generate_input(self):

        ...

run.py:

from Program.engines import PSI4

PSI4('example_molecule').generate_input()

So now when classes are imported at the top of the run.py file, there is no problem even if there's an import error; if there's a failed import with PSI4 because abcdefg cannot be imported, Default is simply imported as PSI4 and given the name PSI4 via the setattr().

Then, only if that Default class is called the ImportError is raised and the user can see that the issue was with PSI4.

This seems to work quite well, even when there are multiple failed imports. We can also add extended error messages for each different failed import. Is this the best way of doing something like this, though? It gets quite messy since we have so many files in our engines package.

Please let me know if some relevant code has been omitted and I can add it back. Thanks for any help.


EDIT for @jpmc26:

I appreciate the time spent on your post but it's not practical for us. The program still fails fast (when necessary) because the imports are first initialised when configs are set. We have ~12 (large) stages of execution, each with multiple options which are handled by these configs. We handle this by reading the configs and terminal commands and calling whatever the option is e.g. EngineForStageFive = PSI4 is set from the configs, then we just call EngineForStageFive(args).do_something() where do_something() exists for all of the stage five classes available. All of this is nicely handled by our terminal command and config file interpreters.

The PSI4 class for example is called many times and repeatedly calling for its import with some additional logic is not what we want in our run file. We'd end up repeating a lot of code unnecessarily. e.g. every stage would need a long if/elif chain or dictionary to determine how it would be used.

\$\endgroup\$
  • \$\begingroup\$ How many functions other than __init__ does an engine have? Is generate_input the only one? \$\endgroup\$ – jpmc26 Jun 25 at 21:38
  • \$\begingroup\$ @jpmc26 It varies really. They all have at least one method other than the init, up to seven being the most. \$\endgroup\$ – QuantumChris Jun 27 at 11:20
10
\$\begingroup\$

There is one issue with your approach which is, in the case of multiple import failure, only the last one can be properly reported:

>>> try:
...   from .foo import Foo
... except ImportError:
...   from .base_engine import Default as Foo
...   setattr(Foo, 'name', 'Foo')
... 
>>> try:
...   from .bar import Bar
... except ImportError:
...   from .base_engine import Default as Bar
...   setattr(Bar, 'name', 'Bar')
... 
>>> Bar()
Traceback (most recent call last):
…
ImportError: The class Bar you tried to call is not importable; this is likely due to it not being installed.
>>> Foo()
Traceback (most recent call last):
…
ImportError: The class Bar you tried to call is not importable; this is likely due to it not being installed.

You instead want to generate a new class with the proper message each time.

Something along the line should do:

def mock_missing(name):
    def init(self, *args, **kwargs):
        raise ImportError(
            f'The class {name} you tried to call is not importable; '
            f'this is likely due to it not being installed.')
    return type(name, (), {'__init__': init})

Usage being:

try:
    from .foo import Foo
except ImportError:
    Foo = mock_missing('Foo')
\$\endgroup\$
7
\$\begingroup\$

This answer builds upon the solution presented by Mathias Ettinger


If you want to get rid of the try: ... except: ... boilerplate you could implement some kind of custom loader for your engines.

This is something I wrote quickly, which is likely far from perfect, but it works.

in __init__.py:

import importlib


def mock_missing(name, msg):
    def init(self, *args, **kwargs):
        raise ImportError(
            f'The class {name} you tried to call is not importable; '
            f'this is likely due to it not being installed. '
            f'Original reason: {msg}')
    return type(name, (), {'__init__': init})


def try_load(engine, module):
    """Try to load an engine, return a mock class if it was not found

    Inspired by https://stackoverflow.com/a/10675081
    """
    try:
        module = importlib.import_module(module, __name__)
        return getattr(module, engine)
    except (ModuleNotFoundError, AttributeError) as ex:
        return mock_missing(engine, msg=str(ex))

To test it:

from engines import try_load

PSI4 = try_load("PSI4", ".psi4")
a = PSI4()

# module does not exist
try:
    PSI5 = try_load("PSI5", ".PSI5")
    b = PSI5()
    raise AssertionError("This should have failed!")
except ImportError as ex:
    print(str(ex))

# class does not exist in module
try:
    PSI6 = try_load("PSI6", ".psi4")
    c = PSI6()
    raise AssertionError("This should have failed!")
except ImportError as ex:
    print(str(ex))

This outputs:

The class PSI5 you tried to call is not importable; this is likely due to it not being installed. Original reason: No module named 'engines.psi5'
The class PSI6 you tried to call is not importable; this is likely due to it not being installed. Original reason: module 'engines.psi4' has no attribute 'PSI6'
\$\endgroup\$
  • \$\begingroup\$ If I could accept two answers I would! Thanks for the help. \$\endgroup\$ – QuantumChris Jun 25 at 8:39
  • \$\begingroup\$ No problem. Glad it helped you. \$\endgroup\$ – AlexV Jun 25 at 8:45
  • \$\begingroup\$ I would be interested to hear about the reason to downvote this answer. \$\endgroup\$ – AlexV Jun 25 at 15:12
7
\$\begingroup\$

That's an odd use of the word "default", which, etymologically speaking, means to remove failure, and generally refers to a fallback setting that "just works". I suggest calling it Missing instead.

\$\endgroup\$
  • \$\begingroup\$ Good idea, I've replaced the name. \$\endgroup\$ – QuantumChris Jun 25 at 8:34
6
\$\begingroup\$

Whenever you find yourself trying to replace a standard language feature, the first thing you should think is,

There's probably something I'm missing. What's the standard approach to this kind of problem?

Your code feels inelegant because you're not using a standard approach. When your code starts to feel weird like that, it's time to take a step back and look for the standard approaches and tools for the kind of problem you're facing. Code becomes elegant when it's something you would expect to see, and you can create that expectation by adhering to the norms and paradigm of the language and tools you're using.


In this case, the standard approach is not to import something until you're actually going to use it. In this context, that means that instead of importing all these classes into your package (__init__.py), let callers decide what engines they require and import them. I've run into situations where my code fails late into its execution because of a dependency that isn't properly installed; it is frustrating and wasted my time. You want your design to encourage fail fast. Having your callers explicitly import the engines from your package that they need will save them trouble.

Example calling code:

from Program.engines.psi4 import PSI4

PSI4('example_molecule').generate_input()

If other parts of your code use the engines, then let the caller inject the engine as a dependency:

class ProgramWorker(object):
    def __init__(self, engine):
        if engine is None:
            raise ValueError('engine must have a value')
        self.engine = engine

    def do_something(self, x, y):
        intermediate = self.engine.calculate_something(x, y)
        # do more with intermediate
        return result

Parameterizing the engine at initialization time like this is an example of dependency inversion, and needing to select one of several implementations is one of the few situations in Python where a more OO approach makes a lot of sense. Instead of having the class find its dependencies, you let callers inject the dependency as an argument. This gives callers greater flexibility, more control, and more transparency into the prerequisites of your code. In particular, they retain all the fail fast properties of normal imports and are given full control over what to load and what not to. Callers would use it like this:

from Program.engines.psi4 import PSI4
from Program.somewhere import ProgramWorker

eng = PSI4('example_molecule')
w = ProgramWorker(eng)
w.do_something()

This wouldn't work if your engines don't have a common interface, but in that case, you shouldn't be using an inheritance structure. If different engines are required for different functionality, then it would be better just to split up the functionality into separate modules based on dependencies.

run.py Command line

Assuming your run.py is a command line application, then you can control the engine with a command line option. Combined with the above, it would then look something like this:

import click

@click.command()
@click.options('--engine', type=click.Choice(['psi4', 'rdkit', ...])), default='psi4', help='The engine to use for computations')
def main(engine):
    if engine_name == 'psi4':
        from .engine.psi4 import PSI4
        eng = PSI4('example_molecule')
    elif engine_name == 'rdkit'
        from .engine.rdkit import RDKit
        eng = RDKit('example_molecule')
    elif engine_name == ...:
        ...
    else:
        raise ValueError('Unknown engine: ' + engine)

    eng.generate_input()

if __name__ == '__main__'
    main()

(You don't have to use click; it's just the easiest command line parser I know of.)

Here, you can just import the engine inline because you need to delay the import until the option has been parsed. This is okay since this block of code, being the command line implementation, will necessarily execute almost immediately after being invoked.

If you need this "select an engine" functionality in multiple places, you could encode that in your engines package (the __init__.py) as a function, but it's unlikely that you have several different scripts that need this as its main purpose is to support the command line. If you have multiple commands that use an engine, you can use a click.group to enable them to share these parameters and this code.


Naming

You should probably rename your modules to avoid conflicts with the names of packages you're depending on: psi4engine or psi4_engine, rdkitengine or rdkit_engine

I would also suffix your engine classes with Engine to avoid possible name conflicts and to increase the clarity of your code: PSI4Engine, RDKitEngine.

Optional dependencies

You might already be doing this if you're creating a package from your code, but in case you're not, you should be declaring packages like PSI4 and RDKit as optional dependencies.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the reply, I've edited in an explanation (too long for here) of why this wouldn't really work for us. \$\endgroup\$ – QuantumChris Jun 27 at 13:22
  • \$\begingroup\$ @HoboProber See the, "This wouldn't work if your engines don't have a common interface, but in that case, you shouldn't be using an inheritance structure. ..." section. All of this feels like a hack around a higher level design problem; in other words, an XY problem. I'm confident there's a better way of organizing the engine and command code, though I can't say what that is without seeing it. \$\endgroup\$ – jpmc26 Jun 27 at 16:19

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.