Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overhaul RMG-RMS Python-Julia dependencies #256

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hwpang
Copy link
Contributor

@hwpang hwpang commented Mar 21, 2024

No description provided.

@hwpang hwpang force-pushed the fix_installation branch 2 times, most recently from bb14830 to 1997ef9 Compare March 21, 2024 21:51
@hwpang
Copy link
Contributor Author

hwpang commented Mar 22, 2024

@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
Copy link
Contributor Author

hwpang commented Mar 22, 2024

@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

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

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

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

❗ Current head 6ce8bfa differs from pull request most recent head b96b603. Consider uploading reports for the commit b96b603 to get more accurate results

Files Patch % Lines
src/PhaseState.jl 61.76% 52 Missing ⚠️
src/ReactionMechanismSimulator.jl 0.00% 27 Missing ⚠️
src/Reactor.jl 44.44% 10 Missing ⚠️
src/Plotting.jl 0.00% 1 Missing ⚠️
src/fluxdiagrams.jl 0.00% 1 Missing ⚠️
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     

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

@hwpang hwpang requested a review from mjohnson541 March 22, 2024 15:40
@JacksonBurns
Copy link
Contributor

@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.

@hwpang
Copy link
Contributor Author

hwpang commented Mar 22, 2024

RMS-RMG twin PR: ReactionMechanismGenerator/RMG-Py#2640

@mjohnson541
Copy link
Collaborator

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.

@hwpang
Copy link
Contributor Author

hwpang commented Mar 29, 2024

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

@mjohnson541
Copy link
Collaborator

@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.

@hwpang hwpang changed the title Attempting to fix installation by switching to PythonCall, CondaPkg, and PythonPlot Overhaul RMG-RMS Python-Julia dependencies Apr 2, 2024
Copy link
Collaborator

@mjohnson541 mjohnson541 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks great! Just a few questions.

.github/workflows/CI.yml Show resolved Hide resolved
src/ReactionMechanismSimulator.jl Show resolved Hide resolved
src/PhaseState.jl Outdated Show resolved Hide resolved
@hwpang
Copy link
Contributor Author

hwpang commented Apr 25, 2024

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

@hwpang
Copy link
Contributor Author

hwpang commented May 14, 2024

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

@hwpang
Copy link
Contributor Author

hwpang commented May 14, 2024

@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 ReactionMechanismGenerator/RMG-Py#2640.

Copy link
Collaborator

@mjohnson541 mjohnson541 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants