From b178895d7127300774a4e86f5e0b17b020ec0bba Mon Sep 17 00:00:00 2001 From: Richard West Date: Thu, 18 May 2023 11:19:08 -0400 Subject: [PATCH 1/7] Refactor julia importing in reactors.py This should have no material change, just a refactoring to make it easier to understand the control flow (which now that I understand, doesn't make much sense to me). It was introduced in https://github.com/ReactionMechanismGenerator/RMG-Py/pull/2364 These timing tests are with julia 1.8.5 h245b042_0 conda-forge pyjulia 0.6.1 pyhd8ed1ab_0 conda-forge time python-jl rmg.py test/regression/superminimal/input.py on my 2019 MacBook Pro 2.6Ghz i7 reports 229.82s user 19.77s system 109% cpu 3:48.45 total although RMG reports Execution time (DD:HH:MM:SS): 00:00:00:40 i.e. it's spent 3 minutes 8 seconds (84% of the total) compiling julia stuff that's never used. By running python instead of python-jl python rmg.py test/regression/superminimal/input.py you can get it to run, and it compiles a bunch of julia stuff, with more logging, and takes (two runs) 486.58s user 17.46s system 105% cpu 7:58.05 total 499.59s user 17.69s system 105% cpu 8:12.54 total (again, only 35s reported for RMG Execution time) And by running python -O rmg.py test/regression/superminimal/input.py you can get it to run the "if not __debug__:" branch which skips the Julia runtime making steps, and causes it to print out some errors like julia.core.UnsupportedPythonError: It seems your Julia and PyJulia setup are not supported. and crash. Later, running time python-jl rmg.py test/regression/superminimal/input.py I get a lot of warnings like WARNING: Method definition getGibbs(P, N) where {N<:Number, P<:ReactionMechanismSimulator.AbstractThermo} in module ReactionMechanismSimulator at /opt/miniconda3/envs/rmg_env4/share/julia/packages/ReactionMechanismSimulator/xoDOp/src/Calculators/Thermo.jl:6 overwritten on the same line (check for duplicate calls to `include`). ** incremental compilation may be fatally broken for this module ** and then 1143.19s user 82.00s system 98% cpu 20:47.85 total --- rmgpy/rmg/reactors.py | 41 ++++++++++++++++++++++++----------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/rmgpy/rmg/reactors.py b/rmgpy/rmg/reactors.py index 0293d5cbcc..9dcba274b6 100644 --- a/rmgpy/rmg/reactors.py +++ b/rmgpy/rmg/reactors.py @@ -36,24 +36,31 @@ import logging import itertools -if __debug__: - try: - from os.path import dirname, abspath, join, exists - path_rms = dirname(dirname(dirname(abspath(__file__)))) - from julia.api import Julia - jl = Julia(sysimage=join(path_rms,"rms.so")) if exists(join(path_rms,"rms.so")) else Julia(compiled_modules=False) - from pyrms import rms - from diffeqpy import de - from julia import Main - except: - pass -else: - try: - from pyrms import rms - from diffeqpy import de - from julia import Main - except: +try: + from julia.api import Julia + from os.path import dirname, abspath, join, exists + system_image_path = join(dirname(dirname(dirname(abspath(__file__)))),"rms.so") + if not __debug__: + """ + This means that python was run with the -O flag. + I don't know why or how often that is done (all it does is + remove assert statements), nor why one would then + need or wish to skip creating a Julia runtime, but for now I'm trying + to refactor without changing behaviour. + """ pass + elif exists(system_image_path): + jl = Julia(sysimage=system_image_path) + else: + jl = Julia(compiled_modules=False) # Disable incremental precompilation of modules. + from pyrms import rms + from diffeqpy import de + from julia import Main +except Exception as e: + logging.exception(e) + logging.warning("Could not import pyrms, RMS simulations will not be available") + pass + from rmgpy.species import Species from rmgpy.reaction import Reaction From 7c005ffb6d1f2dde724baece33bcae5f83d18846 Mon Sep 17 00:00:00 2001 From: Calvin Pieters Date: Thu, 2 Mar 2023 19:41:01 +0200 Subject: [PATCH 2/7] Added instructions into the RMG website for creating system image of rms --- .../source/users/rmg/installation/anacondaDeveloper.rst | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/documentation/source/users/rmg/installation/anacondaDeveloper.rst b/documentation/source/users/rmg/installation/anacondaDeveloper.rst index 5494d2661d..ccfec39430 100644 --- a/documentation/source/users/rmg/installation/anacondaDeveloper.rst +++ b/documentation/source/users/rmg/installation/anacondaDeveloper.rst @@ -151,6 +151,11 @@ Installation by Source Using Anaconda Environment for Unix-based Systems: Linux python -c "import julia; julia.install(); import diffeqpy; diffeqpy.install()" +#. Create custom system image of Julia with RMS compiled, + to greatly accelerate launching:: + + python -m julia.sysimage rms.so + #. Finally, you can run RMG from any location by typing the following (given that you have prepared the input file as ``input.py`` in the current folder). :: python-jl replace/with/path/to/rmg.py input.py From ca36b62da17e19af06670745fc9d31f639bea721 Mon Sep 17 00:00:00 2001 From: Calvin Pieters Date: Thu, 2 Mar 2023 19:19:19 +0200 Subject: [PATCH 3/7] Adjusted the reactors.py import of Julia to always look for rms.so This already did, but now does it even if you call with `python -O`. (i.e. __debug__==False) Co-authored-by: Calvin Pieters Co-authored-by: Richard West --- rmgpy/rmg/reactors.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/rmgpy/rmg/reactors.py b/rmgpy/rmg/reactors.py index 9dcba274b6..71ddc66c6a 100644 --- a/rmgpy/rmg/reactors.py +++ b/rmgpy/rmg/reactors.py @@ -35,12 +35,18 @@ import sys import logging import itertools +from os.path import dirname, abspath, join, exists try: from julia.api import Julia - from os.path import dirname, abspath, join, exists system_image_path = join(dirname(dirname(dirname(abspath(__file__)))),"rms.so") - if not __debug__: + if exists(system_image_path): + logging.info(f"Using system Julia system image at {system_image_path}") + jl = Julia(sysimage=system_image_path) + elif __debug__: + "This is the normal case (__debug__ is True by default)" + jl = Julia(compiled_modules=False) # Disable incremental precompilation of modules. + else: """ This means that python was run with the -O flag. I don't know why or how often that is done (all it does is @@ -49,10 +55,6 @@ to refactor without changing behaviour. """ pass - elif exists(system_image_path): - jl = Julia(sysimage=system_image_path) - else: - jl = Julia(compiled_modules=False) # Disable incremental precompilation of modules. from pyrms import rms from diffeqpy import de from julia import Main From 6110a5d682df7199a2128fa5f036ca834d3b090f Mon Sep 17 00:00:00 2001 From: Richard West Date: Fri, 19 May 2023 09:27:52 -0400 Subject: [PATCH 4/7] Use comments instead of inline string literals. --- rmgpy/rmg/reactors.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/rmgpy/rmg/reactors.py b/rmgpy/rmg/reactors.py index 71ddc66c6a..7e03e5be98 100644 --- a/rmgpy/rmg/reactors.py +++ b/rmgpy/rmg/reactors.py @@ -44,16 +44,14 @@ logging.info(f"Using system Julia system image at {system_image_path}") jl = Julia(sysimage=system_image_path) elif __debug__: - "This is the normal case (__debug__ is True by default)" + # This is the normal case (__debug__ is True by default) jl = Julia(compiled_modules=False) # Disable incremental precompilation of modules. else: - """ - This means that python was run with the -O flag. - I don't know why or how often that is done (all it does is - remove assert statements), nor why one would then - need or wish to skip creating a Julia runtime, but for now I'm trying - to refactor without changing behaviour. - """ + # This means that python was run with the -O flag. + # I don't know why or how often that is done + # (all it does usually is remove assert statements), + # nor why one would then need or wish to skip creating a Julia runtime, + # but for now I'm trying to refactor without changing behaviour. pass from pyrms import rms from diffeqpy import de From cbc52e895d9dfbb4679ea306b82d69a10dfdad5c Mon Sep 17 00:00:00 2001 From: Richard West Date: Fri, 19 May 2023 09:56:40 -0400 Subject: [PATCH 5/7] Use rmgpy.get_path to get the path to the rms.so system image Also reduce imports into local namespace --- rmgpy/rmg/reactors.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/rmgpy/rmg/reactors.py b/rmgpy/rmg/reactors.py index 7e03e5be98..99a817108c 100644 --- a/rmgpy/rmg/reactors.py +++ b/rmgpy/rmg/reactors.py @@ -35,12 +35,14 @@ import sys import logging import itertools -from os.path import dirname, abspath, join, exists +import os +from rmgpy import get_path try: from julia.api import Julia - system_image_path = join(dirname(dirname(dirname(abspath(__file__)))),"rms.so") - if exists(system_image_path): + system_image_path = os.path.join(os.path.dirname(get_path()), "rms.so") + # Should be RMG-Py/rms.so for a normal install, alongside rmg.py + if os.path.exists(system_image_path): logging.info(f"Using system Julia system image at {system_image_path}") jl = Julia(sysimage=system_image_path) elif __debug__: From 613fb28634a56a5f925a29c679f78d62a3704833 Mon Sep 17 00:00:00 2001 From: Richard West Date: Fri, 19 May 2023 10:17:44 -0400 Subject: [PATCH 6/7] Print statements about using the Julia system image. I wanted to use logging, but this is imported before the logging module is imported and set up. Strangely, using the system image I made like this in julia: using PackageCompiler using ReactionMechanismSimulator create_sysimage(["ReactionMechanismSimulator"]; sysimage_path="rms.so") my time to launch is no faster than not having it. Here are a couple of runs of python-jl rmg.py test/regression/superminimal/input.py 289.01s user 20.75s system 109% cpu 4:43.95 total 305.97s user 22.11s system 106% cpu 5:09.14 total compared to without the special system image: 291.06s user 22.26s system 107% cpu 4:52.13 total in both cases, actually running RMG was about 1 minute. --- rmgpy/rmg/reactors.py | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/rmgpy/rmg/reactors.py b/rmgpy/rmg/reactors.py index 99a817108c..43142a15af 100644 --- a/rmgpy/rmg/reactors.py +++ b/rmgpy/rmg/reactors.py @@ -43,18 +43,22 @@ system_image_path = os.path.join(os.path.dirname(get_path()), "rms.so") # Should be RMG-Py/rms.so for a normal install, alongside rmg.py if os.path.exists(system_image_path): - logging.info(f"Using system Julia system image at {system_image_path}") + print(f"Using system Julia system image at {system_image_path}") jl = Julia(sysimage=system_image_path) - elif __debug__: - # This is the normal case (__debug__ is True by default) - jl = Julia(compiled_modules=False) # Disable incremental precompilation of modules. else: - # This means that python was run with the -O flag. - # I don't know why or how often that is done - # (all it does usually is remove assert statements), - # nor why one would then need or wish to skip creating a Julia runtime, - # but for now I'm trying to refactor without changing behaviour. - pass + print(f"Couldn't find Julia system image at {system_image_path}. " + "Using default Julia system image.") + if __debug__: + # This is the normal case (__debug__ is True by default) + jl = Julia(compiled_modules=False) # Disable incremental precompilation of modules. + else: + # This means that python was run with the -O flag. + # I don't know why or how often that is done + # (all it does usually is remove assert statements), + # nor why one would then need or wish to skip creating a Julia runtime, + # and allow incremental precompilation to be its default, + # but for now I'm trying to refactor without changing behaviour. + pass from pyrms import rms from diffeqpy import de from julia import Main From c616741f86d485306e6d6680087a9c0ce03dd9ce Mon Sep 17 00:00:00 2001 From: Richard West Date: Thu, 18 May 2023 21:33:16 -0400 Subject: [PATCH 7/7] TEMPORARY: A CI step that lets you connect to a shell to debug. The debug session opens only if regression tests fail Thanks to https://github.com/mxschmitt/action-tmate --- .github/workflows/CI.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index 0480e53959..9801c621c4 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -134,6 +134,11 @@ jobs: exit 1 fi + # For debugging + - name: Setup tmate session + uses: mxschmitt/action-tmate@v3 + if: ${{ failure() }} + # Upload Regression Results as Failed if above step failed - name: Upload Failed Results if: failure()