-
Notifications
You must be signed in to change notification settings - Fork 34
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
base: main
Are you sure you want to change the base?
Conversation
bb14830
to
1997ef9
Compare
@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 |
@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 |
Codecov ReportAttention: Patch coverage is
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. |
@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. |
RMS-RMG twin PR: ReactionMechanismGenerator/RMG-Py#2640 |
I've done some verification on the juliacall end:
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 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. |
There was a problem hiding this 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.
6ce8bfa
to
b96b603
Compare
@mjohnson541 I addressed your comment and had a question. I will write pretty commit messages after we agree on the changes |
Removing the duplicate export would cause the test to fail. I undo the change. |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
…that are only available locally
No description provided.