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

Standard way of importing python-modules #5402

Closed
philbucher opened this issue Aug 16, 2019 · 4 comments
Closed

Standard way of importing python-modules #5402

philbucher opened this issue Aug 16, 2019 · 4 comments

Comments

@philbucher
Copy link
Member

With the "old" way of importing applications being deprecated, we now should find a common solution for importing the python scripts
Previously we had to do:

import KratosMultiphysics.FluidDynamicsApplication
import vms_solver

now we have several options:

import KratosMultiphysics.FluidDynamicsApplication.vms_solver as vms_solver
from KratosMultiphysics.FluidDynamicsApplication import vms_solver

However, what is also possible and where we had several discussions (e.g. here and here) are explicit relative imports:

# in vms_sovler.py:
from KratosMultiphysics.FluidDynamicsApplication import fluid_solver # absolute import
# OR:
from . import fluid_solver # explicit relative import

from PEP8:
"Absolute imports are recommended, as they are usually more readable and tend to be better behaved ..."

"However, explicit relative imports are an acceptable alternative to absolute imports, especially when dealing with complex package layouts where using absolute imports would be unnecessarily verbose"

I tried both ways already and suggest (up for discussion the following):

  • By default I would go for absolute imports, just as PEP8 suggests.
  • Since however Kratos has a relatively large/complex file structure I would also allow explicit relative imports, if the developer decides to use it, since it does shorten the paths quite a bit (e.g. here)
    => especially when there is a subfolder-structure inside python_scripts (which is now possible and already in use, see CoSimApp and HDF5App) since it can make the paths much shorter
  • What I would not allow is to use explicit relative imports across directories (e.g. importing from parent directory etc), bcs this becomes really unreadable then example

FYI @armingeiser @adityaghantasala

@armingeiser
Copy link
Member

Thanks for starting this discussion.

I also think that explicit relative imports are easy to read for imports from within an application (package).
Personally i am not against navigating back if an import from a parent directory of the same package is made.

However we will implement in #5262 what will be decided here to be Kratos style conform.

@RiccardoRossi
Copy link
Member

@armingeiser @philbucher i do not want to enter in stylistic questions,
nevertheless what i think is really important to achieve is that our importing mechanism becomes more pythonic, in the sense that it follows the standard python rules.
Right now this is not the case due to the manual edit of the python path we do under the hood.
This will be corrected soon and is the thing we must all beware

@philbucher philbucher moved this from To do to Next meeting todo in LEGACY Technical Committee Sep 25, 2019
@pooyan-dadvand
Copy link
Member

@KratosMultiphysics/technical-committee promotes the absolute import rather than the relative one due to its clarity and considering that we don't have many imports in each file, so the verbosity wouldn't be a problem.

@pooyan-dadvand pooyan-dadvand moved this from Next meeting todo to In progress in LEGACY Technical Committee Sep 25, 2019
@armingeiser armingeiser added this to To do in ShapeOptimizationApplication via automation Sep 25, 2019
@philbucher
Copy link
Member Author

closing since I added it to the style guide: https://github.com/KratosMultiphysics/Kratos/wiki/Style-Guide#imports

LEGACY Technical Committee automation moved this from In progress to Done Nov 13, 2019
ShapeOptimizationApplication automation moved this from To do to Done Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

4 participants