Overhaul RMG-RMS Python-Julia dependencies & Add tests for many testless Arkane network algorithms #2640

hwpang · 2024-03-22T18:10:02Z

Motivation or Problem

PythonCall.jl, CondaPkg.jl, and PythonPlot.jl supersede PyCall.jl, Conda.jl, and PyPlot.jl packages. The latter have caused many troubles during RMS installation.

Some key differences between PythonCall and PyCall

PythonCall has a compatible JuliaCall package that provide symmetric interface going from Julia to Python and Python to Julia
"PythonCall by default never copies mutable objects when converting, but instead directly wraps the mutable object. This means that modifying the converted object modifies the original, and conversion is faster" (See here)

I put together a RMG-RMS twin PR at ReactionMechanismGenerator/ReactionMechanismSimulator.jl#256 to switch to use PythonCall.jl, CondaPkg.jl, and PythonPlot.jl from their old versions PyCall.jl, Conda.jl, and PyPlot.jl.

This PR makes necessary changes on the RMG end to keep the RMG-RMS compatibility.

Moreover, during working on this, I found that many new network algorithms in Arkane are not tested. I have added additional tests for them.

Description of Changes

Some major changes include

Use pyjuliacall instead of pyjulia to call Julia modules from Python.
Use juliaup to install Julia instead of using the julia binary from conda-forge. I have found that user the latter can cause many issues on the RMG end as it doesn't check package depedencies appropriately. juliaup is Julia's official version manager.
Get rid of unnecessary dependencies on pyrms and diffeqpy. We use diffeqpy to get CVODE_BDF and solve but that is unnecessary. Now we get CVODE_BDF from Sundials and solve from SciMLBase. This helps RMG to reduce package dependencies even more.

Testing

A clear and concise description of testing that you've done or plan to do.

Reviewer Tips

Suggestions for verifying that this PR works or other notes for the reviewer.

rmgpy/rmg/reactors.py

JacksonBurns · 2024-04-03T18:48:32Z

@hwpang also consider editing the Dockerfile:

RMG-Py/Dockerfile

Line 65 in 14e4861

    
           ENV JULIA_CPU_TARGET="x86-64,haswell,skylake,broadwell,znver1,znver2,znver3,cascadelake,icelake-client,cooperlake,generic"

hwpang · 2024-04-03T21:09:11Z

@hwpang also consider editing the Dockerfile:

RMG-Py/Dockerfile

Line 65 in 14e4861

ENV JULIA_CPU_TARGET="x86-64,haswell,skylake,broadwell,znver1,znver2,znver3,cascadelake,icelake-client,cooperlake,generic"

It doesn't seem that this can be removed by this PR. From here, it sees that is used to avoid compilation overhead

codecov · 2024-04-05T00:33:00Z

Codecov Report

Attention: Patch coverage is 54.54545% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 55.45%. Comparing base (7519944) to head (4bec89e).
Report is 18 commits behind head on main.

❗ Current head 4bec89e differs from pull request most recent head a7fe72f

Please upload reports for the commit a7fe72f to get more accurate results.

Files	Patch %	Lines
rmgpy/pdep/sls.py	22.22%	14 Missing ⚠️
rmgpy/rmg/reactors.py	70.27%	11 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2640      +/-   ##
==========================================
+ Coverage   54.88%   55.45%   +0.57%     
==========================================
  Files         125      125              
  Lines       37025    37057      +32     
==========================================
+ Hits        20321    20551     +230     
+ Misses      16704    16506     -198

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

hwpang · 2024-04-05T02:11:28Z

All tests have passed 🥳🥳🥳 @mjohnson541 Could you review before I clean up the commits? @JacksonBurns Let's work out a plan on the merge sequence with your other PRs

mjohnson541

Heroic effort!

Sorry, about the missing pdep algorithm tests, that's my bad.

I have a few questions about how these changes work, but it looks pretty good.

environment.yml

examples/arkane/networks/acetyl+O2_sls/input.py

mjohnson541 · 2024-04-05T23:21:59Z

rmgpy/rmg/reactors.py

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


Are we breaking workflows for people that use system images to avoid compilation?

Also curious about this

Do you mean by using a (global) Julia binary instead of the one come with the conda environment?

It looks like this tool: https://github.com/MichaelHatherly/SystemImageLoader.jl allows adding of system images to juliaup.

This implies we could imagine later using some of these new tools like: https://julialang.github.io/PackageCompiler.jl/dev/index.html to generate a system image with RMS already precompiled and use the other tool to add it to juliaup during installation. The caveat being if you ever want to modify RMS you have to remove that image. However, the process doesn't look particularly difficult anymore. PackageCompiler looks pretty straightforward. Not as sure about the other package.

I don't think any of us is using this function, and I have also never used this. I don't think we should get ahead of ourselves to provide a service that maybe no one will use. I prefer to wait till someone complains about not having this option anymore, or just let them figure it out rather than doing their work for them.

JacksonBurns

You're a wizard! Thanks for taking this on. A number of small-ish comments throughout.

Overall comment - I did a quick search all for python-jl and found nothing, so I think you totally caught them all (thanks!).

.github/workflows/CI.yml

Dockerfile

documentation/source/users/rmg/releaseNotes.rst

rmgpy/rmg/reactors.py

hwpang · 2024-04-25T20:57:46Z

@JacksonBurns @mjohnson541 I've addressed most of the comments and have a couple of clarifying questions

Co-authored-by: Jackson Burns <33505528+JacksonBurns@users.noreply.github.com>

…object

hwpang · 2024-05-14T14:19:58Z

@mjohnson541 @JacksonBurns I have addressed all the comments. Please review/approve. Thanks.

JacksonBurns mentioned this pull request Mar 22, 2024

Fixing conda binary build via optional RMS install (dependent on #2631) #2641

Open

hwpang force-pushed the test_rms branch 2 times, most recently from 36734da to 56640a9 Compare March 22, 2024 22:23

hwpang mentioned this pull request Mar 22, 2024

Overhaul RMG-RMS Python-Julia dependencies ReactionMechanismGenerator/ReactionMechanismSimulator.jl#256

Open

mjohnson541 reviewed Apr 1, 2024

View reviewed changes

rmgpy/rmg/reactors.py Outdated Show resolved Hide resolved

hwpang changed the title ~~Testing PythonCall compatibility with RMG~~ Overhaul RMG's Julia dependencies Apr 2, 2024

hwpang changed the title ~~Overhaul RMG's Julia dependencies~~ Overhaul RMG-RMS Python-Julia dependencies Apr 2, 2024

hwpang changed the title ~~Overhaul RMG-RMS Python-Julia dependencies~~ Overhaul RMG-RMS Python-Julia dependencies & Add tests for many testless Arkane explorer algorithms Apr 3, 2024

hwpang changed the title ~~Overhaul RMG-RMS Python-Julia dependencies & Add tests for many testless Arkane explorer algorithms~~ Overhaul RMG-RMS Python-Julia dependencies & Add tests for many testless Arkane network algorithms Apr 3, 2024

hwpang requested review from mjohnson541 and JacksonBurns April 5, 2024 13:50

mjohnson541 reviewed Apr 5, 2024

View reviewed changes

JacksonBurns requested changes Apr 9, 2024

View reviewed changes

hwpang force-pushed the test_rms branch 2 times, most recently from 95ad95c to a8b42d1 Compare April 25, 2024 20:57

hwpang added 10 commits May 14, 2024 10:19

switch to branch

7feb483

Use pyjuliacall instead of pyjulia

a56bcbc

No need to install julia from conda

94c5f14

update

81cf924

update

f9933ad

Swap the order

01818c0

update

a23d541

doc

4ecb2cd

Use the preexisting mamba

b524f75

Use JULIA_CONDAPKG_EXE=$CONDA/condabin/conda

b9e822d

hwpang and others added 27 commits May 14, 2024 10:19

debug

e77327a

update doc

46e7341

Add tests

9533fe7

Don't track plots

0573953

don't track yaml files

d92657d

update name

0b985bc

track yaml files

956ee47

See where we get julia from

4e7e568

Add Pkg.

c0be490

debug

a4da2b8

update

095efc7

debug

7608692

use Sundials solver and convert list to julia vector

6bd28bd

Resolve AttributeError: 'Interface' object has no attribute 'species'

69ccd2b

Convert to julia object

8b968a8

Remove duplicate test

ed00c27

Add comments and juliaup status

3810a7e

Add back changes mistakenly done by formatter

c468447

Don't change release note

8318a90

Add docstring

f05549c

Update rmgpy/rmg/reactors.py

9bfbc95

Co-authored-by: Jackson Burns <33505528+JacksonBurns@users.noreply.github.com>

Fix

4179d05

Avoid duplicating

d72a373

Avoid casting to list

233be9d

Switch to use SciMLBase and Sundials under RMS

fd23efc

Add changelog

1591be3

Add comment and docstring to emphasize only supporting native python …

92ee4fd

…object

hwpang force-pushed the test_rms branch from 179c7b3 to 92ee4fd Compare May 14, 2024 14:19

Merge branch 'main' into test_rms

a7fe72f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul RMG-RMS Python-Julia dependencies & Add tests for many testless Arkane network algorithms #2640

Overhaul RMG-RMS Python-Julia dependencies & Add tests for many testless Arkane network algorithms #2640

hwpang commented Mar 22, 2024 •

edited

JacksonBurns commented Apr 3, 2024

hwpang commented Apr 3, 2024

codecov bot commented Apr 5, 2024 •

edited

hwpang commented Apr 5, 2024 •

edited

mjohnson541 left a comment

mjohnson541 Apr 5, 2024

JacksonBurns Apr 9, 2024

hwpang Apr 25, 2024

mjohnson541 Apr 25, 2024 •

edited

hwpang May 10, 2024

JacksonBurns May 13, 2024

JacksonBurns left a comment

hwpang commented Apr 25, 2024

hwpang commented May 14, 2024

Overhaul RMG-RMS Python-Julia dependencies & Add tests for many testless Arkane network algorithms #2640

Are you sure you want to change the base?

Overhaul RMG-RMS Python-Julia dependencies & Add tests for many testless Arkane network algorithms #2640

Conversation

hwpang commented Mar 22, 2024 • edited

Motivation or Problem

Description of Changes

Testing

Reviewer Tips

JacksonBurns commented Apr 3, 2024

hwpang commented Apr 3, 2024

codecov bot commented Apr 5, 2024 • edited

Codecov Report

hwpang commented Apr 5, 2024 • edited

mjohnson541 left a comment

Choose a reason for hiding this comment

mjohnson541 Apr 5, 2024

Choose a reason for hiding this comment

JacksonBurns Apr 9, 2024

Choose a reason for hiding this comment

hwpang Apr 25, 2024

Choose a reason for hiding this comment

mjohnson541 Apr 25, 2024 • edited

Choose a reason for hiding this comment

hwpang May 10, 2024

Choose a reason for hiding this comment

JacksonBurns May 13, 2024

Choose a reason for hiding this comment

JacksonBurns left a comment

Choose a reason for hiding this comment

hwpang commented Apr 25, 2024

hwpang commented May 14, 2024

hwpang commented Mar 22, 2024 •

edited

codecov bot commented Apr 5, 2024 •

edited

hwpang commented Apr 5, 2024 •

edited

mjohnson541 Apr 25, 2024 •

edited