Openbabel/pybel imports

[oliver-s-lee]

This issue is inspired by a hotfix I quickly wrote to solve a strange openbabel import error, but I thought it was worth a wider discussion because of a few more issues that are worth addressing.

Currently, in a few places (at least cclib/io/filewriter.py and cclib/io/ccio.py) we do something like this:

_has_openbabel = find_package("openbabel")
if _has_openbabel:
    from cclib.bridge import makeopenbabel
    # Open Babel 3.0 and above
    try:
        from openbabel import openbabel as ob
        import openbabel.pybel as pb
    # Open Babel 2.4.x and below
    except:
        import openbabel as ob
        import pybel as pb

The specific error that I've fixed is that it's possible (at least in older Babel 2.0) to install openbabel but not pybel. In this case, filewriter.py is not importable. More importantly, through a series of import hops, filewriter gets imported from the main cclib __init__.py file, meaning that no part of cclib is usable on such a system (even though the openbabel bits probably aren't getting used).

The current patch is something like:

_has_openbabel = find_package("openbabel")
if _has_openbabel:
    from cclib.bridge import makeopenbabel
    # Open Babel 3.0 and above
    try:
        from openbabel import openbabel as ob
        import openbabel.pybel as pb
    # Open Babel 2.4.x and below
    except:
        import openbabel as ob
        try:
            import pybel as pb

        except ModuleNotFoundError:
            _has_openbabel = False

But this is really a bit backwards, more properly we should be doing something like:

try:
    try:
        # Babel 3.x
        from openbabel import openbabel as ob
        import openbabel.pybel as pb
        _has_openbabel = True
    except ModuleNotFoundError:
        # Babel 2.x
        import openbabel as ob
        import pybel as pb
        _has_openbabel = True
except Exception:
     # Maybe log an error.
     _has_openbabel = False

More generally though, it feels strange to be importing openbabel at the module level at all, considering it's not formally a dependency of cclib and it's mostly used for fallback stuff afaik? Having import cclib end up doing import openbabel was a bit of a surprise for me at least...

[berquist]

This is how all our optional dependencies work (such as the bridges), though Open Babel is the most pervasive in the codebase. Outside of the bridge my understanding is the same as yours, that it's only for fallback file reading.

My thoughts are:

• OB should remain optional, since it's big, non-trivial to install, and only used for a few rare things.
• I have seen other projects do optional imports better (less tedious), but I can't quickly remember what the projects are. The optional import system should be refactored.
• For this specific case, maybe it is time to require OB 3.x, since the first release is from 2019 and 2.4.1 is from 2016.

[oliver-s-lee]

My thoughts are largely the same. Personally, I don't mind putting rare, optional imports like this in the function body itself (in fact I would argue it's more appropriate there), but I know some people really don't like this style.

Re. obabel 2 vs 3, I would suggest not forcing the migration yet just because of the number of ancient computing clusters out in the wild. In my (limited) experience, the chance of finding obabel 2 vs 3 is probably close to 50:50

[oliver-s-lee]

While we're on the topic are we ok with importing obabel at all considering GPL? I have no idea what the consensus is for optional imports etc.

[berquist]

Personally, I don't mind putting rare, optional imports like this in the function body itself (in fact I would argue it's more appropriate there), but I know some people really don't like this style.

I would agree but also like the ability to give a more informative error message when an optional dependency isn't found other than "error: no module named xyz".

Re. obabel 2 vs 3, I would suggest not forcing the migration yet just because of the number of ancient computing clusters out in the wild. In my (limited) experience, the chance of finding obabel 2 vs 3 is probably close to 50:50

Ok, that's fine. But I am now wondering about what version of Python is being used.

[oliver-s-lee]

I would agree but also like the ability to give a more informative error message when an optional dependency isn't found other than "error: no module named xyz".

Yeah that makes sense, I'm sure we can incorporate that into a function.

Ok, that's fine. But I am now wondering about what version of Python is being used.

That's a good point, probably still a lot of 3.6 out there...