ReactionMechanismSimulator.jl icon indicating copy to clipboard operation
ReactionMechanismSimulator.jl copied to clipboard

Overhaul RMG-RMS Python-Julia dependencies

Open hwpang opened this issue 11 months ago • 11 comments

hwpang avatar Mar 21 '24 20:03 hwpang

@mjohnson541 pointed out that this will likely break RMG's CI. Could you point out where in RMG CI do we need to change to avoid breaking it? cc @JacksonBurns

hwpang avatar Mar 22 '24 13:03 hwpang

@mjohnson541 Recording our offline discussion here: We should replace pyjulia with JuliaCall in RMG (see here: https://juliapy.github.io/PythonCall.jl/stable/pycall/). It should make the installation process easier. cc @JacksonBurns

hwpang avatar Mar 22 '24 15:03 hwpang

Codecov Report

Attention: Patch coverage is 51.59574% with 91 lines in your changes missing coverage. Please review.

Project coverage is 48.42%. Comparing base (1f37bfd) to head (6ce8bfa).

:exclamation: Current head 6ce8bfa differs from pull request most recent head 827a1a4

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

Files Patch % Lines
src/PhaseState.jl 61.76% 52 Missing :warning:
src/ReactionMechanismSimulator.jl 0.00% 27 Missing :warning:
src/Reactor.jl 44.44% 10 Missing :warning:
src/Plotting.jl 0.00% 1 Missing :warning:
src/fluxdiagrams.jl 0.00% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #256      +/-   ##
==========================================
- Coverage   48.71%   48.42%   -0.30%     
==========================================
  Files          31       31              
  Lines        8313     8351      +38     
==========================================
- Hits         4050     4044       -6     
- Misses       4263     4307      +44     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Mar 22 '24 15:03 codecov[bot]

@hwpang you can open a pull request against RMG-Py and change this line of the CI file (https://github.com/ReactionMechanismGenerator/RMG-Py/blob/eed950a389738e70894c25e1d8388bb20ef75947/.github/workflows/CI.yml#L155) to install this branch of RMS from your repository. We can then just see the failures live.

JacksonBurns avatar Mar 22 '24 16:03 JacksonBurns

RMS-RMG twin PR: https://github.com/ReactionMechanismGenerator/RMG-Py/pull/2640

hwpang avatar Mar 22 '24 22:03 hwpang

I've done some verification on the juliacall end:

  1. The primary trick I used to make pyrms from pyjulia works with juliacall
  2. I think we can get around the need for diffeqpy, at least I think I circumvented our original reason for needing it...which should be nice because pyrms will no longer need the full DifferentialEquations.jl installed.

I think in terms of merge order this would look like: merge the RMS PR, merge a pyrms PR, build new binaries for pyrms, and then merge RMG PR.

mjohnson541 avatar Mar 23 '24 23:03 mjohnson541

@mjohnson541 Can you make the pyrms PR since you've looked into this? Thanks

hwpang avatar Mar 29 '24 16:03 hwpang

@mjohnson541 Can you make the pyrms PR since you've looked into this? Thanks

Yes, I'm waiting until this PR is building properly, right now I wouldn't be able to test the pyrms PR properly.

mjohnson541 avatar Mar 29 '24 18:03 mjohnson541

@mjohnson541 I addressed your comment and had a question. I will write pretty commit messages after we agree on the changes

hwpang avatar Apr 25 '24 20:04 hwpang

Removing the duplicate export would cause the test to fail. I undo the change.

hwpang avatar May 14 '24 14:05 hwpang

@mjohnson541 This is ready for another review / approve. Need to clean up the commits after approval and coordinate to merge at the same time with https://github.com/ReactionMechanismGenerator/RMG-Py/pull/2640.

hwpang avatar May 14 '24 14:05 hwpang