Kratos: Error in packaging trying to import python with full path

[ddiezrod]

We have found in @KratosMultiphysics/altair side, that binaries created with cx_freeze throw an error when there is an import like:
from KratosMultiphysics.FluidDynamicsApplication.fluid_solver import FluidSolver

however as we include the location to the path, this works fine:
from fluid_solver import FluidSolver

Problem is that when we do install, we copy python files inside "kratos" directory instead of "KratosMultiphysics". I understand this is done to avoid installing again after modifying a python file when your install directory is kratos itself., but I think its reasonable to change this install directory to be more pythonic despite the mentioned disadvatange.

Just to mention that I did try copying manually python files into "KratosMultiphysics" and everything works fine.

@KratosMultiphysics/technical-committee @KratosMultiphysics/altair

[philbucher]

Just to understand correctly, the problem occurs when you package Kratos (like we do for the release)?

several thought from my side:

• from KratosMultiphysics.FluidDynamicsApplication.fluid_solver import FluidSolver is the correct/pythonic way to import the py-scripts, from fluid_solver import FluidSolver is deprecated and slowly removed step by step. New apps don't even support this any more and many existing apps are already updated
• In order to have "real" python modules we should install the py-scripts to KratosMultiphysics/AppXXX/. The reason this is not done is because this would require compiling even if only python-scripts are modified, which so far was considered a no-go. This is what you describe as working.
• since this does not happen for the the regular release-packaging, could this be a problem with cx_freeze itself? Actually this is the first time I hear that this is not working.

As solution I propose to install the py-scripts to KratosMultiphysics when creating a binary, I think this is a reasonable solution and should not affect the current development process (@pooyan-dadvand this is what I suggested last week)

[jcotela]

@ddiezrod the KratosMultiphysics that is giving you trouble is not just the name of the folder, it is the name of the Python module (although, technically it just means that there is a folder somewhere in the python path with this name and an __init__.py file inside it).

If I understand you correctly, you don't want to copy the python files anywhere, just consider the regular kratos folder structure as your "install dir" and package that through cxx_freeze. If so, you could try renaming your main kratos folder to KratosMultiphysics and copying the __init__.py file in kratos/python_interface/ (and everything else there too) to the root kratos dir. I think this should be the start of a solution, although we probably need to modify some of the paths within __init__.py (see the KratosPaths class within that file) to suit your specific setup.

[pooyan-dadvand]

What @ddiezrod is proposing is to change the cmake install process to copy the .py scripts in their corresponding hierarchy inside the KratosMultiphysics folder/module by default and avoid calls like from fluid_solver import FluidSolver.

@philbucher I don't want to do this during the packaging because I don't have a generic way to do this clearly by myself without asking all apps doing it by themselves. Also, thinking more about it, I see this more straight forward than the "workaround" of adding our modules to the path.

I understand that an in_place_install flag is needed for developers who want to change the pythons in place without reinstalling again.

[RiccardoRossi]

please note that we need any mechanism to work "in place" ... i mean without having to install the python files. I consider this vital...

[ddiezrod]

Thanks @pooyan-dadvand for clarifying mi point! I agree it makes sense to have some flag like "INSTALL_IN_SOURCE_DIR" that maintains the current behaviour.

[jcotela]

@pooyan-dadvand @ddiezrod can you clarify exactly what structure do you want? Take for example the python from your example:

applications/FluidDynamicsApplication/python_scripts/fluid_solver.py

Where do you want to install it? The "natural" place for a pure python module should be:

KratosMultiphisics/FluidDynamicsApplication/fluid_solver.py

If that is the case, my understanding is that calls like

from KratosMultiphysics.FluidDynamicsApplication.fluid_solver import FluidSolver

are the correct, pythonic way of importing a submodule, while

from fluid_solver import FluidSolver

is a hack that only works in kratos because we make funny things with the system path when we define the module.

I am not opposed to installing the python files, but any changes should make kratos work more like a native python module, not less. If the imports with the full path as mentioned do not work, we need to fix them, not forbid them.

[ddiezrod]

@jcotela You got it right, that's the most natural structure and the one we'd like to have. My first comment was probably quite confusing, but our point was to be able to do imports with full path, not to forbid it. Of course we can change this in our side, but we wanted to comment it here as it seems to be a logical step.

[jcotela]

@ddiezrod thanks for the clarification. However, I do not fully understand what you are trying to achieve. What would your desired folder structure look like? Do you want the "duplicate" python scripts from applications to be created inside the existing kratos folder? Do you have a KratosMultiphysics folder inside kratos? Subfolders for each application?

[roigcarlo]

I think it would be good that if the user chooses to install Kratos in a different location, we copy the scripts inside the module rather than copying them in applications/appname/python_scripts, while keeping the other behavior otherwise.

As I understand the trick with the python_scripts folder relies only in a append found in the __init__ file of the app module, which should be easy enough to modify during the install.

[ddiezrod]

Ok, right now the structure of the python_scripts is the following independently that if you install "in place" or somewhere else:

kratos/python_scripts
applications/FluidDynamicsApplication/python_scripts

When packaging this with cx_frezze, imports with full path fail. To be able to do these imports structure should be:

    ../python_scripts
    ../FluidDynamicsApplication/python_scripts

Which is more or less what @roigcarlo suggests I guess

[jcotela]

I believe that the "natural" python structure would be

KratosMultiphyisics/
  + __init__.py (from kratos/python_interface)
  + (contents of kratos/python_scripts) 
  FluidDynamicsApplication/
    + __init__.py (renamed version of applications/FluidDynamicsApplication/FluidDynamicsApplication.py)
    + (contents of applications/FluidDynamicsApplication/python_scripts)

With this, imports with full paths should work out of the box, since they just reflect the folder structure. Recent changes we have done with @philbucher go in this direction and I think we should go for it eventually.

[philbucher]

The description of @jcotela is the correct / fully pythonic way

[rubenzorrilla]

Thanks @pooyan-dadvand for clarifying mi point! I agree it makes sense to have some flag like "INSTALL_IN_SOURCE_DIR" that maintains the current behaviour.

Just to mention that I think that the current behaviour should be the default and that I think that the new behaviour has to be the one activated by the flag.

[philbucher]

Thanks @pooyan-dadvand for clarifying mi point! I agree it makes sense to have some flag like "INSTALL_IN_SOURCE_DIR" that maintains the current behaviour.

Just to mention that I think that the current behaviour should be the default and that I think that the new behaviour has to be the one activated by the flag.

I absolutely agree, this would be a big change otherwise

[pooyan-dadvand]

My idea was to go for the pythonic structure @jcotela proposing. (Thanks for the example) If we want to have python interface lets have it in pythonic way.

Just to mention that I think that the current behaviour should be the default and that I think that the new behaviour has to be the one activated by the flag.

I completely disagree,

The pythonic one should be by default. If developers wants to maintain in source pythons for their convenience then they should activate by setting the flag but standard install should go in correct way of use.

[RiccardoRossi]

i am with @rubenzorrilla and @philbucher on this.

we install a few times a year but we develop all the days...so developement should be made easy

[philbucher]

I was looking at the CMake in the apps, doesn't INSTALL_PYTHON_FILES already do what you would like to have?
Did you try this @ddiezrod ?

[ddiezrod]

@philbucher Yes, we use this variable, but the resulting structure is not the one proposed by @jcotela, The structure you obtain imitates kratos structure:

kratos/ (contents of kratos/python_scripts)
applications/FluidDynamicsApplication/ (contents of applications/FluidDynamicsApplication/python_scripts)

We could change the behaviour of this variable but then users that want to use kratos "in place" should set it to off.

[philbucher]

To me changing this variable would make full sense (to get the behaviour @jcotela described)
I don't see when the other structure (the current behavior) would be used.

[rubenzorrilla]

For me makes also sense. Apart of what @philbucher is pointing out, to introduce a new variable, which will probably have a similar name to the current one, to get the behaviour @jcotela is describing might be misleading.

[ddiezrod]

Just to clarify that I would also be in favor of that option.

[jcotela]

So it seems that the consensus would be to change the behavior of INSTALL_PYTHON_FILES to build a proper python module folder instead of replicating the current kratos folder structure. @ddiezrod Where would you want to install this then? I think that the folder containing the python files still needs to be called KratosMultiphysics for all this to work (I am not sure if you wanted it to be kratos directly or not).

[ddiezrod]

So it seems that the consensus would be to change the behavior of INSTALL_PYTHON_FILES to build a proper python module folder instead of replicating the current kratos folder structure. @ddiezrod Where would you want to install this then?

Rihgt now, files are installed in CMAKE_INSTALL_PREFIX and if it is not defined, then files are installed in bin directory inside kratos. I think we can keep it this way.

I think that the folder containing the python files still needs to be called KratosMultiphysics for all this to work (I am not sure if you wanted it to be kratos directly or not).

Yes, its fine for us that the name is KratosMultiphysics

[RiccardoRossi]

@jcotela my problem is that i need the mechanism to work _without installing the python files_

if this is not accomplished everytime you change a script you need to install. furthermore, when you have an error the error is thrown by the installed file and not by the original file, which makes very cumbersome to debug.

isn't it possible to change the folder structure of Kratos so that it is more pythonic?
that way one could install by copying everything aside for the list of directories which contain the cpp files.

[RiccardoRossi]

i have a 1watt idea:

we could define a "kratos_install" cmake function which instead of copying files to the new directory optionally creates symbolic links.
This is sort of hacky, but it should work out of the box with minimal changes

[RiccardoRossi]

ok, i have been spending a few hours on this, and i can confirm that also with nuitka (not just CXFreeze) the mechanism doesn't work.

i am posting here the file minimal.py

~py
import KratosMultiphysics
import KratosMultiphysics.FluidDynamicsApplication
import KratosMultiphysics.FluidDynamicsApplication.fluid_solver - without this line everythign works
~

this is the script i use for compilation with nuitka. It doesn't work when i try to install fluid_solver, but i guess it will work properly otherwise, and i guess it can be useful when the files are installed.

~~~

here it should import this correctly

python3 minimal.py

compiling via nutika. I am importing explicitly the directories, not sure if it is needed

rm -rf minimal.dist/
rm -rf minimal.build/

python3 -m nuitka --follow-imports \
--standalone \
--include-plugin-directory=/home/rrossi/kratos_git/KratosMultiphysics \
--include-plugin-directory=/home/rrossi/kratos_git/applications/FluidDynamicsApplication/python_scripts \
minimal.py

compying the .so modules into the dist subdirectory

cp ~/kratos_git/libs/*.so minimal.dist/

checking that it works

echo "********* running the packaged file - should work"
./minimal.dist/minimal

now forgetting where kratos is, so that normal usage fails

export LD_LIBRARY_PATH=
export PYTHONPATH=
echo "******** running the original file - should not work since we forgot where kratos is"
python3 minimal.py
echo "******** running the packaged file - should work even now"
./minimal.dist/minimal
~~~

I don't have any other better solution, so i guess that installing to a "pythonic" directory structure is the only way
Still i consider absolutely needed to have the possibility of avoiding copies of the files, so the only option i can see right now is to have cmake install to create symbolic links to the original files instead of copying.
(of course such behaviour should be optional...in releases we wouldn't use it).
What do you guys think?

[loumalouomega]

Just to mention that in case we solve this, we will be one step closer to create wheel files (the ones used on pip) and the corresponding counterpart on conda. This will be very helpful for distribution

[RiccardoRossi]

here i have a prototype implementation.

one can control if installing a link or a copy by the ON/OFF switch

~
cmake . -DINSTALL_LINK=OFF
~

this is only tried on linux. I expect this to work also on win10, provided that one uses a very recent cmake (apparently older versions were buggy).
having said this i would much appreciate if anyone can try it in win

install_test.zip

[philbucher]

@RiccardoRossi wait wait I don't get what your problem is now ... ?

To me it seems like everyone would be fine with having an option to install the python files to KratosMultiphysics if a configure-flag is set:

So it seems that the consensus would be to change the behavior of INSTALL_PYTHON_FILES to build a proper python module folder instead of replicating the current kratos folder structure. @ddiezrod Where would you want to install this then?

Rihgt now, files are installed in CMAKE_INSTALL_PREFIX and if it is not defined, then files are installed in bin directory inside kratos. I think we can keep it this way.

I think that the folder containing the python files still needs to be called KratosMultiphysics for all this to work (I am not sure if you wanted it to be kratos directly or not).

Yes, its fine for us that the name is KratosMultiphysics

[RiccardoRossi]

@philbucher i think it is very problematic to have two completely different installing mechanism, for which the import mechanism doesn't have any relation. for example which one would u use in the CI?

My point here is that we will have two different directory structures depending on wheter we kept the files where we are or if you installed to a different path. This in turn reflect in a difference in the importing mechanism (the __init__.py of the different applications), which becomes way simpler if you have a pythonic structure. The bad thing is that we would have to deal always with the two different cases, which i see as a big pain.

taking this into account my idea would be to install always to the pythonic structure BUT to have in the install path links to the original files instead of copies. this way one could change the files in the destination paths while guaranteeing that they are also changed in the root directory.

the file i posted simply provides a function doing the linking (equivalent of ln -s origin_file destination_file ) in cmake

[philbucher]

@RiccardoRossi can we have a short call on Monday?
I think we are talking abt different things, better discuss it in person

[RiccardoRossi]

@Bucher, Philipp [e-mail] unfortunately i will be all the
week out at a conference ...

On Sat, Jun 29, 2019 at 6:08 PM Philipp Bucher [e-mail]
wrote:

@RiccardoRossi https://github.com/RiccardoRossi can we have a short
call on Monday?
I think we are talking abt different things, better discuss it in person

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/KratosMultiphysics/Kratos/issues/5101?email_source=notifications&email_token=AB5PWEOVFGPP66PHWPI5PXTP46CHDA5CNFSM4HY6UAZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY33RXQ#issuecomment-506968286,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB5PWEMKJWMOT7JS6KTAVZDP46CHDANCNFSM4HY6UAZQ
.

--

Riccardo Rossi

PhD, Civil Engineer

member of the Kratos Team: www.cimne.com/kratos

Associate Professor at Universitat Politècnica de Catalunya, BarcelonaTech
(UPC)

Full Research Professor at International Center for Numerical Methods in
Engineering (CIMNE)

C/ Gran Capità, s/n, Campus Nord UPC, Building C1, First Floor

08034 – Barcelona – Spain – www.cimne.com -

T.([phone] skype: rougered4

http://www.cimne.com/

https://www.facebook.com/cimne http://blog.cimne.com/
http://vimeo.com/cimne http://www.youtube.com/user/CIMNEvideos
http://www.linkedin.com/company/cimne https://twitter.com/cimne

Les dades personals contingudes en aquest missatge són tractades amb la
finalitat de mantenir el contacte professional entre CIMNE i voste. Podra
exercir els drets d'accés, rectificació, cancel·lació i oposició,
dirigint-se a [email protected]. La utilització de la seva adreça de
correu electronic per part de CIMNE queda subjecte a les disposicions de la
Llei 34/2002, de Serveis de la Societat de la Informació i el Comerç
Electronic.

Imprimiu aquest missatge, només si és estrictament necessari.
http://www.cimne.com/

[RiccardoRossi]

to be sure i understant, I understand that the proposal is to have an installation structure as

~
install_dir
| -- KratosMultiphysics
| -- __init__.py #this one loads the Kratos "core"
|
| -- FluidDynamicsApplication
| | -- __init__.py #here the loader for FluidDynamicsApplciation
| | -- ... here A COPY (or in my proposal A LINK) all the content of the python_scripts directory
|
| -- StructuralMechanicsApplication
| | -- __init__.py #here the loader for StructuralMechanicsApplication
| | -- ... here A COPY (or in my proposal A LINK) of all the content of the python_scripts directory
|
~

such a structure would actually be very nice, and the "__init__.py" would be much (much!!) simpler than they are now. It would be also much easier to package as @loumalouomega is commenting.

HOWEVER to change on of kratos python scripts you need to change it in the source_dir and reinstall.
if by chance you change it here (easy to do, since the error are thrown from those files when you execute!!) then your changes are lost when you reinstall.
I had to do this for a proejct, and it is a real pain in the ass, so i REALLY would like to avoid having this behaviour in our normal pipeline

my proposal to avoid this issue is to have links to the original python files within this tree

[philbucher]

@RiccardoRossi I am not sure if using links is the way to go for this
For packaging it won't work I think

Again, I think this would be better to be discussed in person

[loumalouomega]

I believe that links doesn't work too

El dom., 30 jun. 2019 18:34, Philipp Bucher [e-mail]
escribió:

@RiccardoRossi https://github.com/RiccardoRossi I am not sure if using
links is the way to go for this
For packaging it won't work I think

Again, I think this would be better to be discussed in person

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/KratosMultiphysics/Kratos/issues/5101?email_source=notifications&email_token=AEYQZADUD5FCNASR2ZOAR6DP5DOADA5CNFSM4HY6UAZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY4PO4Y#issuecomment-507049843,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEYQZAB2U2IA5YYLZZQTVJTP5DOADANCNFSM4HY6UAZQ
.

[RiccardoRossi]

@philbucher it will not work for packaging but then it is as simple as copying instead of linking.
please take a look at the prototype implementation in #5145

the point is that while developing this option will be very useful since there are no two copies of the same file!!

having said this, ok about voice speaking. i'll try to give you a whatsapp call during the week

[philbucher]

having said this, ok about voice speaking. i'll try to give you a whatsapp call during the week

@RiccardoRossi I think now is not urgent any more since you already did the work :D
We can speak next week, enjoy your conference

[roigcarlo]

@RiccardoRossi If it will not work for packaging I wouldn't do it at all. The whole problem here as I understand is packaging with cx_freeze etc... If we implement this and then we have to do something special for packaging, we have the same problem again.

However, I do thing that the solution with links is a more or less good compromise, but I though it would be done differently. When we spoke I though you said you wanted to have simlinks in the KratosMultiphysics/application folder for the installation in place, and copying the files there instead of the applications dir while installing in a different location.

Examples:

• Installing Kratos in the source dir:

| -- Applications
      | -- FluidDynamicsApplication
            | -- python_scripts
                  | -- Here the original python scripts (these are the files that you will have to modify to make changes in the code) 
| -- KratosMultiphysics
      | -- __init__.py #this one loads the Kratos "core"
      |
      | -- FluidDynamicsApplication
      |     | -- __init__.py  #here the loader for FluidDynamicsApplciation
      |     | -- ... here A LINK of all the content of the python_scripts directory
      |
      | -- StructuralMechanicsApplication
      |     | -- __init__.py  #here the loader for StructuralMechanicsApplication
      |     | -- ... here A LINK of all the content of the python_scripts directory

Notice that there is virtually no change between this and how you are working now. Except if you want to create a new *.py, in this case you have to install again.

• Installing Kratos in a different location:

# Notice that wether we copy it in applications/foo/python_scripts or 
# KratosMultiphysics/foo, a copy is done.
install_dir
| -- KratosMultiphysics
      | -- __init__.py #this one loads the Kratos "core"
      |
      | -- FluidDynamicsApplication
      |     | -- __init__.py  #here the loader for FluidDynamicsApplciation
      |     | -- ... here A COPY all the content of the python_scripts directory
      |
      | -- StructuralMechanicsApplication
      |     | -- __init__.py  #here the loader for StructuralMechanicsApplication
      |     | -- ... here A COPY of all the content of the python_scripts directory

The main advantage here is that by default when installing Kratos keeps organized as a python module, and we use the simlinks to mimic this structure so the CI works in the same way, and not the other way around.

Also there is an alternative solution to this which consist on "merging" the applications and KratosMultiphysics folder. This way the python submodule and the application folder are technically the same, so no additional copies are required and one could just (or similar, we may have to iterate over this code):

import os

__all__ = []

for module in os.listdir(os.path.dirname(__file__)):
    if module != '__init__.py' and module[-3:] == '.py':
        __all__.append(module[:-3])

to expose the scripts in the python_scripts folder to the module level (@ddiezrod maybe this works with cx_freeze and the current structure too, its worth a try in my opinion). Despise being possible I wouldn't do it but the option is there.

[roigcarlo]

So, I spoke with @RiccardoRossi and seems that I was a little confused about what he was proposing.
Technically we are just saying the same. And basically resumes in:

• Kratos installation is always in a different directory
• INSTALL_LINK=ON will create LINKS to the *.py files from applications/foo/python_scripts to KratosMultiphysics/foo
• INSTALL_LINK=OFF will COPY to the *.py files from applications/foo/python_scripts to KratosMultiphysics/foo

so INSTALL_LINK=ON for developing and INSTALL_LINK=OFF for packaging. Is that right?

[ddiezrod]

So, I spoke with @RiccardoRossi and seems that I was a little confused about what he was proposing.
Technically we are just saying the same. And basically resumes in:

• Kratos installation is always in a different directory
• INSTALL_LINK=ON will create LINKS to the *.py files from applications/foo/python_scripts to KratosMultiphysics/foo
• INSTALL_LINK=OFF will COPY to the *.py files from applications/foo/python_scripts to KratosMultiphysics/foo

so INSTALL_LINK=ON for developing and INSTALL_LINK=OFF for packaging. Is that right?

This sounds fine to me. Maybe @jrubiogonzalez has something else to add as he has been fighting with this, specially in places like process_factory where imports can be quite tricky.

[RiccardoRossi]

also about the flag...let s avoid another
DoNOTdefineNdebug,

so proposals are very welcome :-)

On Mon, Jul 1, 2019, 10:44 AM Daniel Diez [e-mail] wrote:

So, I spoke with @RiccardoRossi https://github.com/RiccardoRossi and
seems that I was a little confused about what he was proposing.
Technically we are just saying the same. And basically resumes in:

• Kratos installation is always in a different directory
• INSTALL_LINK=ON will create LINKS to the *.py files from
  applications/foo/python_scripts to KratosMultiphysics/foo
• INSTALL_LINK=OFF will COPY to the *.py files from
  applications/foo/python_scripts to KratosMultiphysics/foo

so INSTALL_LINK=ON for developing and INSTALL_LINK=OFF for packaging. Is
that right?

This sounds fine to me. Maybe @jrubiogonzalez
https://github.com/jrubiogonzalez has something else to add as he has
been fighting with this, specially in places like process_factory when
imports can be quite tricky.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/KratosMultiphysics/Kratos/issues/5101?email_source=notifications&email_token=AB5PWELZ2EQO5YCTHYNM4UDP5HGZTA5CNFSM4HY6UAZ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODY5S67I#issuecomment-507195261,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB5PWELPOMSWJUEZHMIXM6DP5HGZTANCNFSM4HY6UAZQ
.

[pooyan-dadvand]

Seems to be a good compromise. But I would go for copy as default.

[rubenzorrilla]

I also think that this is a good compromise solution. With regard to the default behavior, I'd be in favor of keeping the current one.

My unique concern is if it will also work in Windows (I'm not familiar so I'm just raising the question).

Finally, I'd suggest to use a more self-descriptive variable. Maybe LINK_BASED_PYTHON_INSTALL.

[pooyan-dadvand]

@ddiezrod is this the one that solved by using the new import?

[ddiezrod]

Yes @pooyan-dadvand

[rubenzorrilla]

Can it be closed then?

[ddiezrod]

I think you also need https://github.com/KratosMultiphysics/Kratos/pull/5825

[roigcarlo]

This should now be solved. If you can confirm we will close it. Thx!

[RiccardoRossi]

@ddiezrod can u confirm this is now solved?

[ddiezrod]

@RiccardoRossi I am currently checking it

[ddiezrod]

it does work, closing this. Thank you very much guys!