relative imports should be returned back.

Issue #48 resolved
Alireza Khorshidi created an issue

Relative importing was removed in the commit aac4ba5, because ReadtheDocs could not find modules and indices with it.

Though relative importing is very useful. I should look back into it.

Comments (6)

  1. andrew_peterson repo owner

    @akhorshi I wonder if we should take relative imports back out? I often find myself wanting to look for help on readthedocs which is not there (module docstrings) because of this issue.

    I personally am no longer using relative imports, now that we have settled on amp v0.5. It was useful when we had two versions, and also is useful for switching the package name e.g., from neural to amp. However, having good documentation may be more useful.

  2. andrew_peterson repo owner

    Also, we're a bit alone on using relative imports. It looks like ASE and numpy do not use them, in my brief scan of the source code.

  3. Muammar El Khatib

    I was investigating a bit about it. PEP8 suggests the use of absolute imports:

    Absolute imports are recommended, as they are usually more readable and tend to be better behaved (or at least give better error messages) if the import system is incorrectly configured (such as when a directory inside a package ends up on sys.path ):

    Relative imports apparently are not that bad (their use is a question of preference), but sometimes it is more difficult to understand where the errors come from. When used to import local packages, it does not seem to be a bad practice either[1, 2]. However, in Python 3 imports are treated as absolute[2, 3].

    Sources:

    1. http://stackoverflow.com/a/16748366/1995261

    2. http://stackoverflow.com/questions/16981921/relative-imports-in-python-3

    3. http://softwareengineering.stackexchange.com/a/159505

    Wouldn't it be good to change back to absolute paths where possible?.

  4. andrew_peterson repo owner

    I think we seem to still have a mix. In init.py, I see from .utilities import ... while in model/init.py I see from amp.utilities import ....

    I guess we should settle on one or the other. I can go either way on this. Also, I don't think this is necessarily a problem for python3; according to Muammar's source 3:

    "Python 3 has disabled implicit relative imports altogether; imports are now always interpreted as absolute, meaning that in the above example import baz will always import the top-level module. You will have to use the explicit import syntax instead (from . import baz)."

    I somewhat prefer the "explicit relative import syntax" as we know the imports are then coming from our same package. But not many developers seem to use it, and there's something to be said for standardizing with others.

  5. Log in to comment