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.

hwpang · 2024-06-07T15:34:39Z

The CI is failing because PythonCall.jl has recently added a requirement for python version to be >=3.8 here. During RMG subgroup discussion, we have decided to wait to merge this PR after #2635.

JacksonBurns · 2024-06-27T14:09:07Z

@hwpang I'm revisiting this following some of the discussion in #2684 - I realize now that using the conda binary for juliaup might not be the best idea and that we would be better off using their official installation instructions. I say this because there are already reports on the juliaup conda feedstock that it is installing packages in the incorrect conda environment: conda-forge/juliaup-feedstock#62

The official juliaup installation instructions say to use a shell script (https://github.com/JuliaLang/juliaup?tab=readme-ov-file#mac-and-linux) and provide a GitHub actions step for our CI (https://github.com/JuliaLang/juliaup?tab=readme-ov-file#continuous-integration-ci) - your thoughts?

mjohnson541 · 2024-06-27T15:45:16Z

I just tested setting JULIAUP_DEPOT_PATH to a path inside the env before telling juliaup to install julia, without setting it it installs .julia etc in the default location for julia (not ideal for conda), with it defined it installs it where I told it to, so we probably need to set that to a path we choose inside the conda environment.

no longer needed thanks to ReactionMechanismGenerator/RMG-Py#2640

JacksonBurns · 2024-06-28T15:43:54Z

Development moved to #2687

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 16 commits May 14, 2024 10:19

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

mjohnson541 mentioned this pull request Jun 10, 2024

Avoid Unnecessary Julia Operations + Optional RMS Installation #2631

Open

JacksonBurns mentioned this pull request Jun 18, 2024

Complete conda-recipes Overhaul ReactionMechanismGenerator/conda-recipes#18

Merged

rwest mentioned this pull request Jun 24, 2024

Upgrading to Python 3.11 #2635

Closed

mjohnson541 mentioned this pull request Jun 27, 2024

RMG Improvement Proposal [RIP]: Make ReactionMechanismSimulator.jl Optional [COMMENTS WELCOME!] #2684

Open

rwest mentioned this pull request Jun 27, 2024

[Python 3.9] Overhaul RMG-RMS Python-Julia dependencies (juliaup and pythoncall), Add Tests for Arkane Network Algorithms #2687

Draft

JacksonBurns added a commit to ReactionMechanismGenerator/conda-recipes that referenced this pull request Jun 28, 2024

delete diffeqpy and pyrms

0bdfe65

no longer needed thanks to ReactionMechanismGenerator/RMG-Py#2640

JacksonBurns closed this Jun 28, 2024

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

Loading

JacksonBurns commented Apr 3, 2024

hwpang commented Apr 3, 2024

codecov bot commented Apr 5, 2024 •

edited

Loading

hwpang commented Apr 5, 2024 •

edited

Loading

mjohnson541 left a comment

mjohnson541 Apr 5, 2024

JacksonBurns Apr 9, 2024

hwpang Apr 25, 2024

mjohnson541 Apr 25, 2024 •

edited

Loading

hwpang May 10, 2024

JacksonBurns May 13, 2024

JacksonBurns left a comment

hwpang commented Apr 25, 2024

hwpang commented May 14, 2024

hwpang commented Jun 7, 2024

JacksonBurns commented Jun 27, 2024

mjohnson541 commented Jun 27, 2024

JacksonBurns commented Jun 28, 2024

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

Conversation

hwpang commented Mar 22, 2024 • edited Loading

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 Loading

Codecov Report

hwpang commented Apr 5, 2024 • edited Loading

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 Loading

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 Jun 7, 2024

JacksonBurns commented Jun 27, 2024

mjohnson541 commented Jun 27, 2024

JacksonBurns commented Jun 28, 2024

hwpang commented Mar 22, 2024 •

edited

Loading

codecov bot commented Apr 5, 2024 •

edited

Loading

hwpang commented Apr 5, 2024 •

edited

Loading

mjohnson541 Apr 25, 2024 •

edited

Loading