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

application_importer : provide folders to be included #2655

Closed
adityaghantasala opened this issue Aug 9, 2018 · 33 comments
Closed

application_importer : provide folders to be included #2655

adityaghantasala opened this issue Aug 9, 2018 · 33 comments

Comments

@adityaghantasala
Copy link
Member

adityaghantasala commented Aug 9, 2018

Currently the application_importer always adds "python_scripts" and "constitutive_laws" to the python paths. These are hard coded.

Is there a specific reason to do this ? If not can a list of folders be passed by the ****Application.py so that all of them are added to the path ?

@jcotela
Copy link
Member

jcotela commented Aug 9, 2018

If I remember correctly, the plan was to have all python scripts for your application in python_scripts. I believe the constitutive_laws was at some point used by the old structural application, but is not used anymore (since constitutive laws are implemented in C), so I'd rather consider removing it.

What use did you have in mind for this new feature?

@adityaghantasala
Copy link
Member Author

Well it not a feature what I have in mind .. but more of organization of the python scripts into different folders solvers, processes, utilities(may be) , trilinos and so on depending on application .. ! all of them can be a different folder inside 'python_scripts' folder or outside and can these be added to the path depending on what a application specifically wants

@jcotela
Copy link
Member

jcotela commented Aug 9, 2018

I fully agree with having subfolders in python_scrips, although I would prefer not to add other "free" folders in the application structure (I believe keeping a uniform application structure helps both users and developers to find where things are).

One thing we were discussing with @oberbichler and @philbucher is that it would be good if applications behaved as native python modules, which would allow both a subfolder structure and a submodule organization in python (I am thinking from FluidDynamicsApplication import trilinos_solvers or something of the sort). Somewhat counterintuitively, now only the C++ parts of kratos behave as native python modules, the python parts are included through the hackish method you found in the applications_importer.

I have some ideas about how all this could be implemented without destroying backwards compatibility, but at this point this is more of a wish than a planned feature.

@adityaghantasala
Copy link
Member Author

What you are describing has also the same effect as I was hoping.
I believe discussing this further and implementing changes will benefit all.

@jcotela
Copy link
Member

jcotela commented Sep 12, 2018

Just for future reference: #2848 is also caused by the hackish handling of application modules, and whatever changes we propose in the future should be designed to solve it.

@adityaghantasala
Copy link
Member Author

I was thinking of something like this in application_importer at line

python_path = os.path.join(application_path, 'python_scripts')

import sys
current_dir_name = "path/to/application/python_scripts"

# Append all the sub directories containing the solver interfaces to the path
for dirName, subdirList, fileList in os.walk(current_dir_name):
    sys.path.append(dirName)

Then each sub folder in python_scripts is no longer a module but is just on he python path.

@adityaghantasala
Copy link
Member Author

adityaghantasala commented Sep 24, 2018

The following code works for me

from __future__ import print_function, absolute_import, division #makes KratosMultiphysics backward compatible with python 2.6 and 2.7

import sys
import os
import imp # Required to import the module from the source file
applications_root = os.path.dirname(os.path.realpath(__file__)) # This is example. This is the root folder of the applications
application_name = 'KratosExampleApplication' # Name of the application
application_folder = 'KratosExampleApplication' # Application folder

application_path = os.path.join(applications_root, application_folder) # creating the path to the applicaiton where python_scripts folder exists
python_scripts_path = os.path.join(application_path, 'python_scripts') 
sys.path.append(python_scripts_path)# adding python_scripts to the path as already done. NOTE: This can be skipped actually

current_app_module = sys.modules[__name__] # getting the object of the current application module

# Append all the sub directories containing the solver interfaces to the path
for dir_name, subdir_list, file_list in os.walk(python_scripts_path): # walking in python_scripts folder. i.e iterating over the subfolders and files recursively
    dir_name_wo_path = os.path.basename(dir_name) # Getting the directory name without  its path
    if (dir_name_wo_path != '__pycache__'): # we do not want to look inside __pycache__ folder
        for file_name in file_list: # looping over files in this folder
            file_extension = os.path.splitext(file_name)[1] # obtaining the extension of the file
            if (file_extension != '.pyc'): # we do not need .pyc files
                file_name_with_path = os.path.join(dir_name, file_name) # name of the current file with full path. needed for importing.
                file_module_name =  os.path.splitext(file_name)[0] # (just) the file name with out .py
                file_module = imp.load_source(file_module_name, file_name_with_path) # importing the file module
                setattr(current_app_module, file_module_name, file_module) # setting the file module into the Application module

The idea is to add the contents of sub folders of 'python_scripts' to the Application module. I am also adding a zip file with an example
python_module_import_test.zip

@philbucher
Copy link
Member

Does this (imp) work with python2?
I remember looking into this a while ago and remember there were some diffs btw python2&3 but I don’t remember the details

@adityaghantasala
Copy link
Member Author

I tested it with python 2.7 .. it works .. !

@oberbichler
Copy link
Contributor

oberbichler commented Sep 24, 2018

Minimal modification for supporting the subfolder names

class Module(object): # << new
    pass

# walking in python_scripts folder. i.e iterating over the subfolders and files recursively
for dir_name, subdir_list, file_list in os.walk(python_scripts_path):

    # Getting the directory name without  its path
    dir_name_wo_path = os.path.basename(dir_name)

     # we do not want to look inside __pycache__ folder
    if (dir_name_wo_path == '__pycache__'):
        continue

    module = Module() # << new

    setattr(current_app_module, dir_name_wo_path, module) # << new

    # looping over files in this folder
    for file_name in file_list:
        # obtaining the extension of the file
        file_extension = os.path.splitext(file_name)[1]

        # we do not need .pyc files
        if (file_extension == '.pyc'):
            continue

        # name of the current file with full path. needed for importing.
        file_name_with_path = os.path.join(dir_name, file_name)

        # (just) the file name with out .py
        file_module_name = os.path.splitext(file_name)[0]

        # importing the file module
        file_module = imp.load_source(file_module_name, file_name_with_path)

        # setting the file module into the Application module
        setattr(module, file_module_name, file_module) # << new

Then you can do:
class1 = current_app_module.sub_folder_one.test_module_one.TestModuleOne()

But I am not sure if this will work for all possible scenarios...

@adityaghantasala
Copy link
Member Author

Yes that is a nice idea .. I tested it .. it works with both python 2.7 and 3.4

@adityaghantasala
Copy link
Member Author

@oberbichler But I am not sure if this will work for all possible scenarios... which scenarios do you think it will not work ?

@philbucher
Copy link
Member

Another question:
Will this interfere if we some day start using proper python modules in Kratos (@jcotela)?

@oberbichler
Copy link
Contributor

@adityaghantasala I would say the scenarios we do not expect. Users can do crazy stuff with python. But I think we need some limitations and can not support all python functionalities since we are using a 'hacky' solution.

@jcotela
Copy link
Member

jcotela commented Sep 25, 2018

@philbucher this should work as an alternative to the modules. To be honest I would prefer a "native" solution since, as @oberbichler is saying, we never know when this might fail to reproduce the behavior of a real module. I am now looking for alternative ways to combine pybind and native python in a single module but this is an improvement over the current situation, so I think we should consider it.

@adityaghantasala
Copy link
Member Author

@jcotela a solution involving pybind means that in the configure file (or somehow) we should include the (sub)folders which will be treated as modules ? or can it be automated to take all the subfolders in python_scripts to make them modules ?

@jcotela
Copy link
Member

jcotela commented Sep 25, 2018

In my opinion the trick is to point the application to the python_scripts folder in a natural way. Once that is done, making python aware of the subfolders should only involve adding an __init__.py to each of them.

@adityaghantasala
Copy link
Member Author

adityaghantasala commented Sep 25, 2018

I personally like a solution without adding __init__.py . The reason being : as I see the subfolders in python_scripts folder exist more for classification of scripts and organizing them. Also there is this extra task of adding this file (even though its empty) which has not much to do with the implementation. This extra file means more maintenance (seeing that this file does not have any unwanted / unfitting code) ... !

@oberbichler
Copy link
Contributor

I understand the reasons for a custom solution but in general I would prefer the __init__.py etc. because it is part of the python ecosystem -> standard solution. Usually that makes things easier.

@adityaghantasala
Copy link
Member Author

my only concern about having init.py is maintenance. Also as soon as init.py is there people can do user/problem/app specific things which can break testing and may be other thing .. ! If it should be a empty (and is somehow forced to be empty) yes I am totally for it. Also in this case I think some automation can be done to create these files (which excludes possibility of user/problem/app specific code in it) ... !

@oberbichler
Copy link
Contributor

oberbichler commented Oct 8, 2018

and is somehow forced to be empty

Why?

@adityaghantasala
Copy link
Member Author

imagine someone puts
import numpy as np
in the init file ...

@oberbichler
Copy link
Contributor

imagine someone puts
import numpy as np
in any other script file ...

@adityaghantasala
Copy link
Member Author

yes .. it can very well happen ..
but if there is no file it means one less possibility of that happening .. :-p

@philbucher
Copy link
Member

I agree with @adityaghantasala in the sense it is better to keep things simple
We are already not using the standard way of using modules in Python, therefore I think it is good to limit the things that can cause problems

@jcotela
Copy link
Member

jcotela commented Oct 8, 2018

Well, in my view the main argument for adopting the __init__.py is to use the standard way to define modules in Python.

@philbucher
Copy link
Member

@jcotela can we use the standard way without major changes?

@jcotela
Copy link
Member

jcotela commented Oct 8, 2018

No, but we can leave the "adding everything to path" active during the transition period (or that is my hope at least).

@philbucher
Copy link
Member

Hm that also sounds good

+1 for adding the init.py then

@philbucher
Copy link
Member

@adityaghantasala in contrast to your solution, have you tried to make Kratos to use the regular python way of importing submodules?

I was discussing this with @jcotela and @oberbichler the other day and we thought this might be a better way to go for the future

We should discuss it before proceeding with this

@adityaghantasala
Copy link
Member Author

this means having init.py if I am correct ?

@oberbichler
Copy link
Contributor

The __init__.py is optional

@philbucher
Copy link
Member

solved properly in #3217

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants