Make and use a julia system image #2443

rwest · 2023-05-18T20:12:27Z

This originally had in its scope some changes to how we use pyjulia, but that has been broken off into #2444

Motivation or Problem

Time to launch is awful (for me on my mac) because it recompiles a ton of julia stuff every time you import anything from reactors.py
There's a way to launch Julia with a pre-compiled system image (pyjulia docs) which should both get around the static linking problem inherent in conda's python, perhaps remove the need to use python-jl, and accelerate time to launch.
Some of the code around this was a bit confusing.

Description of Changes

incorporates the changes from Creating and utilising a custom system image of Julia with RMS #2381 that advises developers to build a system image of julia with RMS compiled.
refactors that code to be easier (for me) to understand

I am hoping that, before merging, this PR will get this working nicely and greatly accelerate time to launch.

Testing

It is not yet ready to review for a merge, but I'm opening the pull request now (and requesting reviews)

to start discussion
for something for folks to hack on
to see if it passes the CI tests

Caveats

For me, on my intel Mac with julia and pyjulia coming from the conda-forge channel, it does not yet (2023-05-19) accelerate anything 😞 . Oh, and the current instructions on building system image don't quite work.

JacksonBurns

This will be a nice addition to RMG-Py once it is working - as of writing the conda-forge pyjulia causes build to fail because we have skipped the linking step present in our custom build:

sed -i 's|bin/python|bin/python3|g' $(which python-jl)

(from ChatGPT) This command uses the sed command to replace all occurrences of bin/python with bin/python3 in the file specified by $(which python-jl). The -i option modifies the file in place. So, when you run this command, it will replace all instances of bin/python with bin/python3 in the specified file.

I think we can just run this command after solving the conda environment and get the same effect? Or would building the system image remove the need to do so?

I have also left some stylistic comments.

The CI should build a Julia image so that this use case is actually tested, yes?

documentation/source/users/rmg/installation/anacondaDeveloper.rst

rmgpy/rmg/reactors.py

JacksonBurns · 2023-05-18T21:48:24Z

rmgpy/rmg/reactors.py

+    from pyrms import rms
+    from diffeqpy import de
+    from julia import Main
+except Exception as e:


Bare exceptions are also considered bad practice. We should try to catch specific exceptions - failing to import Julia api could be because Julia is not installed, failed to load Julia with the image, specific imports fail, etc.

I agree in principle, bare exceptions are a code smell. But I'm hoping that in future this whole thing can go away. I think it was introduced so ARC could pass nose tests or something - where this file is imported somewhere but nothing in it needs to actually run.
For this use case, all exceptions should be treated the same, so I'm going to leave it as is, until we get rid of it entirely.
At least I added some logging so we can see what the exception is.

JacksonBurns · 2023-05-18T21:54:57Z

rmgpy/rmg/reactors.py

Stylistically I think the "normal" case should come first in the if/elif/else.

I also think the failure to import pyrms, diffeqpy, or Julia should raise an exception not log a warning - not being able to simulate reactors should stop execution outright IMO. I am aware that other packages currently assume that this import fails and ignore it, but we are burying a significant runtime issue here. Other codes should do something like

try: from rmgpy import reactors except JuliaImportError as jie: warnings.warn("Julia import failed as expected")

Raising an exception to be handled by the calling code would mean the rest of this file would not get imported, which is what this "try/pass" thing is trying to achieve. I think the use case is code that needs to import this module, but doesn't actually execute anything important in it.
Not very satisfactory, I agree, (in more ways than one) but fixing is beyond scope of this PR.

JacksonBurns · 2023-05-18T22:18:59Z

Possibly unhelpful idea: PythonCall.jl is an alternative to PyJulia that is better maintained. They have a page to help move to their tool.
Can we consider doing so?
Would doing so require lots of editing RMS? And potentially also pyrms? Or even RMG's reactors.py?
Why do they call it rush hour when nothing moves?

codecov · 2023-05-19T05:36:15Z

Codecov Report

Merging #2443 (85a6207) into main (cd2b77a) will increase coverage by 0.01%.
The diff coverage is 58.82%.

❗ Current head 85a6207 differs from pull request most recent head c616741. Consider uploading reports for the commit c616741 to get more accurate results

@@            Coverage Diff             @@
##             main    #2443      +/-   ##
==========================================
+ Coverage   48.12%   48.13%   +0.01%     
==========================================
  Files         110      110              
  Lines       30629    30633       +4     
  Branches     7989     7989              
==========================================
+ Hits        14739    14745       +6     
+ Misses      14362    14357       -5     
- Partials     1528     1531       +3

Impacted Files	Coverage Δ
rmgpy/rmg/reactors.py	`20.87% <58.82%> (+0.13%)`	⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

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 #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

This already did, but now does it even if you call with `python -O`. (i.e. __debug__==False) Co-authored-by: Calvin Pieters <calvinpieters@gmail.com> Co-authored-by: Richard West <r.west@northeastern.edu>

Also reduce imports into local namespace

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.

The debug session opens only if regression tests fail Thanks to https://github.com/mxschmitt/action-tmate

github-actions · 2023-09-07T08:07:31Z

This pull request is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant pull request, otherwise it will automatically be closed in 30 days.

rwest requested review from calvinp0 and JacksonBurns May 18, 2023 20:12

JacksonBurns mentioned this pull request May 18, 2023

add helpful error message for failed julia dependency imports #2359

Closed

JacksonBurns requested changes May 18, 2023

View reviewed changes

rwest force-pushed the pyjulia branch from 5f1ff3c to 85a6207 Compare May 19, 2023 14:18

rwest mentioned this pull request May 19, 2023

Fix our use of pyjulia so we can use the conda-forge version. #2444

Merged

rwest changed the title ~~Tweaking Pyjulia and related instructions; make julia system image~~ Make and use a julia system image May 19, 2023

rwest marked this pull request as draft May 19, 2023 19:18

rwest mentioned this pull request May 22, 2023

Updating Conda environment requirements #2322

Closed

2 tasks

JacksonBurns mentioned this pull request May 23, 2023

Python 3.7 End-of-Life and Upgrading to Python 3.11 #2445

Closed

JacksonBurns linked an issue May 23, 2023 that may be closed by this pull request

Create system image of RMS using Julia #2379

Closed

rwest force-pushed the pyjulia branch from 85a6207 to c9b36a4 Compare May 24, 2023 14:05

rwest and others added 7 commits June 7, 2023 00:05

Added instructions into the RMG website for creating system image of rms

7c005ff

Adjusted the reactors.py import of Julia to always look for rms.so

ca36b62

This already did, but now does it even if you call with `python -O`. (i.e. __debug__==False) Co-authored-by: Calvin Pieters <calvinpieters@gmail.com> Co-authored-by: Richard West <r.west@northeastern.edu>

Use comments instead of inline string literals.

6110a5d

Use rmgpy.get_path to get the path to the rms.so system image

cbc52e8

Also reduce imports into local namespace

TEMPORARY: A CI step that lets you connect to a shell to debug.

c616741

The debug session opens only if regression tests fail Thanks to https://github.com/mxschmitt/action-tmate

rwest force-pushed the pyjulia branch from c9b36a4 to c616741 Compare June 7, 2023 04:05

JacksonBurns added the Python 3.11 Transition PRs and Issues related to transitioning from Python 3.7 to 3.11 label Jun 8, 2023

github-actions bot added the stale stale issue/PR as determined by actions bot label Sep 7, 2023

github-actions bot added the abandoned abandoned issue/PR as determined by actions bot label Oct 8, 2023

github-actions bot closed this Oct 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make and use a julia system image #2443

Make and use a julia system image #2443

rwest commented May 18, 2023 •

edited

Loading

JacksonBurns left a comment

JacksonBurns May 18, 2023

rwest May 19, 2023

JacksonBurns May 18, 2023

rwest May 19, 2023

JacksonBurns commented May 18, 2023

codecov bot commented May 19, 2023 •

edited

Loading

github-actions bot commented Sep 7, 2023

Make and use a julia system image #2443

Make and use a julia system image #2443

Conversation

rwest commented May 18, 2023 • edited Loading

Motivation or Problem

Description of Changes

Testing

Caveats

JacksonBurns left a comment

Choose a reason for hiding this comment

JacksonBurns May 18, 2023

Choose a reason for hiding this comment

rwest May 19, 2023

Choose a reason for hiding this comment

JacksonBurns May 18, 2023

Choose a reason for hiding this comment

rwest May 19, 2023

Choose a reason for hiding this comment

JacksonBurns commented May 18, 2023

codecov bot commented May 19, 2023 • edited Loading

Codecov Report

github-actions bot commented Sep 7, 2023

rwest commented May 18, 2023 •

edited

Loading

codecov bot commented May 19, 2023 •

edited

Loading