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 7 commits behind head on main.

❗ Current head 4bec89e differs from pull request most recent head a8b42d1. Consider uploading reports for the commit a8b42d1 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.

mjohnson541 · 2024-04-05T23:15:11Z

environment.yml

@@ -49,8 +49,8 @@ dependencies:
  - conda-forge::rdkit >=2022.09.1

 # general-purpose external software tools
-  - conda-forge::julia=1.9.1
-  - conda-forge::pyjulia >=0.6
+  - conda-forge::juliaup


This makes sense. What does this mean as far as how we think about julia version?

Chiming in with some notes from offline conversation, just to make sure I'm on the same page as well. From my understanding, juliaup installs a Julia binary in the conda environment, but 'hides' it from conda so that that juliaup can manage the version. I think this results in the Julia version being whatever the default provided by juliaup is, which I think is just the latest minor release.

We can also explicitly manage julia version through commands such as juliaup add 1.9.0. I incline to let the Julia version update whenever we could and avoid sticking to an old version.

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.

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

JacksonBurns · 2024-04-09T20:31:59Z

.github/workflows/CI.yml

@@ -151,8 +151,9 @@ jobs:
      - name: Install and link Julia dependencies
        timeout-minutes: 120 # this usually takes 20-45 minutes (or hangs for 6+ hours).
        run: |
-          python -c "import julia; julia.install(); import diffeqpy; diffeqpy.install()"
-          julia -e 'using Pkg; Pkg.add(PackageSpec(name="ReactionMechanismSimulator",rev="main")); using ReactionMechanismSimulator'
+          which julia


Just a reminder that we can remove this before merging

How do you feel about running juliaup status here in the CI instead to print out the Julia version we are using? Just to make it easier for us to keep track of things.

.github/workflows/CI.yml

JacksonBurns · 2024-04-09T20:32:58Z

.github/workflows/CI.yml

-          julia -e 'using Pkg; Pkg.add(PackageSpec(name="ReactionMechanismSimulator",rev="main")); using ReactionMechanismSimulator'
+          which julia
+          export JULIA_CONDAPKG_EXE=$CONDA/condabin/mamba
+          julia -e 'ENV["JULIA_CONDAPKG_BACKEND"] = "Current"; using Pkg; Pkg.add("SciMLBase"); Pkg.add("Sundials"); Pkg.add(Pkg.PackageSpec(name="ReactionMechanismSimulator", url="https://github.com/hwpang/ReactionMechanismSimulator.jl.git", rev="fix_installation")); using ReactionMechanismSimulator'


Does RMS not require sundials or SciMLBase on its own? Why do we have to specify them separately?

RMS does require sundials and SciMLBase. I think it's better to install them to help distinguish the RMS dependency (used in reactors.py) and the general Julia dependency (used in network.py). If I don't install them explicitly here, I can still access them under RMS's dependency, but it makes network.py seem like it depends on RMS, which can be misleading. What do you think?

Dockerfile

documentation/source/users/rmg/releaseNotes.rst

rmgpy/rmg/reactors.py

JacksonBurns · 2024-04-09T21:00:48Z

rmgpy/rmg/reactors.py

An overall comment/question. How can we differentiate to_julia and to_rms in our heads? Having some difficulty understanding from just the code which is required where.

to_julia handles general types in Julia (think about dict, arrays), and to_rms handles custom types defined by RMS

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

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

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

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 from f9747a7 to 95ad95c Compare April 25, 2024 20:56

hwpang added 12 commits April 25, 2024 16:56

switch to branch

fb40473

Use pyjuliacall instead of pyjulia

7669d38

No need to install julia from conda

e8abb45

update

f54f7a3

update

3ca6c64

Swap the order

1bd5e5f

update

0282700

doc

44461ec

Use the preexisting mamba

203ce18

Use JULIA_CONDAPKG_EXE=$CONDA/condabin/conda

5449fa2

Add channels

5bcc02f

skip make for now for debugging

e5ec5d7

hwpang and others added 28 commits April 25, 2024 16:56

debug

01ec2e0

convert empty list

b2e59b6

Add additional test file for sls

2b451a7

Should be seval

6334101

debug

6801019

update doc

c08035d

Add tests

ea10090

Don't track plots

34115dd

don't track yaml files

b7f6568

update name

e06fcbf

track yaml files

5b7790b

See where we get julia from

bd82cce

Add Pkg.

ed66233

debug

21c271d

update

3720ac1

debug

9fa52ad

use Sundials solver and convert list to julia vector

9ccddb6

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

28fd9a6

Convert to julia object

a964531

Remove duplicate test

8897bc6

Add comments and juliaup status

2f0d088

Add back changes mistakenly done by formatter

4630ed4

Don't change release note

df471df

Add docstring

1c7f9f2

Update rmgpy/rmg/reactors.py

d6b25ed

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

Fix

3c6eb78

Avoid duplicating

1c4bed4

Avoid casting to list

a8b42d1

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

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

JacksonBurns Apr 9, 2024

hwpang Apr 25, 2024

mjohnson541 Apr 25, 2024 •

edited

JacksonBurns left a comment

JacksonBurns Apr 9, 2024

hwpang Apr 25, 2024 •

edited

JacksonBurns Apr 9, 2024

hwpang Apr 25, 2024

JacksonBurns Apr 9, 2024

hwpang Apr 25, 2024

hwpang commented Apr 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjohnson541 Apr 25, 2024 • edited

Choose a reason for hiding this comment

JacksonBurns left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwpang Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwpang commented Apr 25, 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

hwpang Apr 25, 2024 •

edited