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

Cleaning up Python module import infrastructure #3608

Merged
merged 18 commits into from
Jan 16, 2019
Merged

Conversation

jcotela
Copy link
Member

@jcotela jcotela commented Dec 7, 2018

Now that #3586 is merged, the Python import infrastructure can be greatly simplified. This PR cleans up some of the darker things going on when an application is loaded. We have also taken the occasion to finally remove the old application_importer, which has been deprecated for 7 years now...

This PR is 100% backwards compatible but it also introduces a "more Pythonic" module import function _ImportApplicationAsModule. This one is optional but, if application developers choose to use it in their application's __init__ instead the current one, it does not add the application#s python_scripts folder to the python path. This means that python scripts would have to be imported relative to the application

import KratosMultiphysics
import KratosMultiphysics.FluidDynamicsApplication as kfd

# the next one is the big change 
import kfd.navier_stokes_solver_vmsmonolithic as solver
# alternatively
from kfd.navier_stokes_solver_vmsmonolithic import NavierStokesSolverMonolithic

This allows Kratos applications to work like native python submodules but, since it would break current scripts, it is not enabled by default.

@jcotela jcotela added Cleanup Python Legacy Old code not maintained labels Dec 7, 2018
@jcotela jcotela requested a review from a team December 7, 2018 15:36
import inspect
caller = inspect.stack()[1] # Information about the file that imported this, to check for unexpected imports
application_importer.ImportApplication(application,application_name,application_folder,caller)
KM._ImportApplicationAsModule(application, application_name, application_folder, __path__)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note that we ported the MappingApp right away to see how it is being used

@jcotela jcotela added this to the Release 6.1 milestone Jan 10, 2019
@philbucher
Copy link
Member

@KratosMultiphysics/technical-committee it would be nice to have this asap
Two rather large apps (IGA and CoSimulation) could then use it from the beginning and users would not have to update painfully at some point

Copy link
Member

@pooyan-dadvand pooyan-dadvand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KratosMultiphysics/technical-committee approves the cleanup but let put on hold the optional part to maintain the backward compatibility

@jcotela jcotela merged commit c110c57 into master Jan 16, 2019
@jcotela jcotela deleted the core/new-app-importer branch January 16, 2019 13:57
@philbucher
Copy link
Member

@KratosMultiphysics/technical-committee just to clarify:
The change introduced here only removes the "old" way of adding stuff to the path for the next-gen python-interface of Kratos
Right now with the change I introduced in #3217 we have two ways of using python-modules: the pythonic way (e.g. import KratosMultiphysics.process_factory) or the "old"direct way (e.g. import process_factory)

The big apps already now support both ways (the later one for backwards compatibility)

Only new apps will use right away the new/correct/pythonic import system. There we don't need backwards-compatibility bcs there is no "old" way

From a user-point of view we already now use the "new" way for old and new apps
=> see e.g. the MainKratos.py in GiD:
FluidMainKratos
StructuralMainKratos

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup Legacy Old code not maintained Python
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants