Avoid Unnecessary Julia Operations + Optional RMS Installation #2631
Conversation
JacksonBurns
force-pushed
the
fix/avoid_unnecessary_julia
branch
3 times, most recently
from
52ada46
to
ab9468e
Compare
|
Also TODO: use Also also TODO: after updating the documentation, add a link to the Julia-related installation steps in the exception that is thrown when attempting to call RMS without it installed. See #2631 (comment) |
|
I'm all in favor of avoiding the julia start up cost for runs that don't need it. Which should solve issue #2547. Although, as I noted in that issue Arkane is dependent on pyjulia entirely independently of RMS. However, RMS is not an optional install or anything you can treat as an optional backend. We have published RMG features that are not available without RMS, RMS is the only sensible way right now to add a new reactor to RMG, RMG electrochemistry is entirely dependent on RMS because none of the RMG reactors can run it. Doing so does not save anyone time maintaining the installation because the installation with RMS has to work regardless. The initial install time for RMS is not particularly burdensome. It will even make debugging PRs more complicated because there will be more differences between users' local copies and the tests. |
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:01:07 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Passed Edge Comparison ✅Original model has 106 species.
Observables Test Case: Aromatics Comparison
✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! aromatics Passed Observable Testing ✅Regression test liquid_oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:02:19 liquid_oxidation Failed Core Comparison ❌Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 202 species. Non-identical kinetics! ❌
kinetics:
Observables Test Case: liquid_oxidation Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! liquid_oxidation Passed Observable Testing ✅Regression test nitrogen:Reference: Execution time (DD:HH:MM:SS): 00:00:01:25 nitrogen Passed Core Comparison ✅Original model has 41 species. nitrogen Passed Edge Comparison ✅Original model has 132 species.
Observables Test Case: NC Comparison
✅ All Observables varied by less than 0.200 on average between old model and new model in all conditions! nitrogen Passed Observable Testing ✅Regression test oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:02:33 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species.
Observables Test Case: Oxidation Comparison
✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! oxidation Passed Observable Testing ✅Regression test sulfur:Reference: Execution time (DD:HH:MM:SS): 00:00:00:57 sulfur Passed Core Comparison ✅Original model has 27 species. sulfur Failed Edge Comparison ❌Original model has 89 species.
Observables Test Case: SO2 Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! sulfur Passed Observable Testing ✅Regression test superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:00:36 superminimal Passed Core Comparison ✅Original model has 13 species. superminimal Passed Edge Comparison ✅Original model has 18 species. Regression test RMS_constantVIdealGasReactor_superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:02:31 RMS_constantVIdealGasReactor_superminimal Passed Core Comparison ✅Original model has 13 species. RMS_constantVIdealGasReactor_superminimal Passed Edge Comparison ✅Original model has 13 species.
Observables Test Case: RMS_constantVIdealGasReactor_superminimal Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_constantVIdealGasReactor_superminimal Passed Observable Testing ✅Regression test RMS_CSTR_liquid_oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:05:52 RMS_CSTR_liquid_oxidation Failed Core Comparison ❌Original model has 35 species. RMS_CSTR_liquid_oxidation Failed Edge Comparison ❌Original model has 197 species.
Observables Test Case: RMS_CSTR_liquid_oxidation Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_CSTR_liquid_oxidation Passed Observable Testing ✅Regression test fragment:Reference: Execution time (DD:HH:MM:SS): 00:00:00:42 fragment Passed Core Comparison ✅Original model has 10 species. fragment Passed Edge Comparison ✅Original model has 33 species.
Observables Test Case: fragment Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! fragment Passed Observable Testing ✅Regression test RMS_constantVIdealGasReactor_fragment:Reference: Execution time (DD:HH:MM:SS): 00:00:03:09 RMS_constantVIdealGasReactor_fragment Passed Core Comparison ✅Original model has 10 species. RMS_constantVIdealGasReactor_fragment Passed Edge Comparison ✅Original model has 27 species.
Observables Test Case: RMS_constantVIdealGasReactor_fragment Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_constantVIdealGasReactor_fragment Passed Observable Testing ✅beep boop this comment was written by a bot 🤖 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2631 +/- ##
==========================================
- Coverage 54.90% 54.26% -0.65%
==========================================
Files 125 125
Lines 37050 37061 +11
==========================================
- Hits 20342 20110 -232
- Misses 16708 16951 +243 ☔ View full report in Codecov by Sentry. |
| @@ -44,8 +40,7 @@ RUN git clone --single-branch --branch main --depth 1 https://github.com/Reactio | |||
|
|
|||
| WORKDIR /rmg/RMG-Py | |||
| # build the conda environment | |||
| RUN conda env create --file environment.yml && \ | |||
| conda clean --all --yes | |||
There was a problem hiding this comment.
What is the function of this line conda clean --all --yes?
There was a problem hiding this comment.
This was added here in #2437 to reduce the size of the finished docker image. As per the conda docs it just removes some junk files we don't need.
| @@ -1558,6 +1559,8 @@ def read_input_file(path, rmg0): | |||
| exec(f.read(), global_context, local_context) | |||
| except (NameError, TypeError, SyntaxError) as e: | |||
| logging.error('The input file "{0}" was invalid:'.format(full_path)) | |||
| if NO_JULIA: | |||
| logging.error("During runtime, import of Julia dependencies failed. To use phase systems and RMS reactors, install RMG-Py with RMS.") | |||
There was a problem hiding this comment.
I think we should add a link to the documentation page regarding how to install Julia dependencies
There was a problem hiding this comment.
Good call! I will add this to my TODO comment
| - rmg::symmetry | ||
|
|
||
| # packages we would like to stop maintaining (and why) | ||
| - rmg::diffeqpy |
There was a problem hiding this comment.
I just learn that diffeqpy is used for PDep calculations here:
Line 36 in bfaee1c
There was a problem hiding this comment.
Thanks for pointing this out! It's good that there are other methods already implemented as well, and informative error catch will go nicely here. A note for when this is added: we should probably only code the 'hey your julia stuff is missing' error string in one place and then import it elsewhere.
Thanks for pointing that out! I have added a comment on that issue.
I see this as less about removing features and more about increasingly the 'extend-ability' of RMG. Our primary distribution method (Docker) will still include all of the RMS stuff, so by default everyone will still have access to every feature we have ever promised. I'll also add that this is somewhat being forced as a technical issue, or at least an issue with my technical understanding. There may very well be a way to get the binary creation to work, but it will certainly be more difficult than the workflow that this change would allow: We have also historically 'promised' binaries, and I see this as the only way to provide those.
Partly replied to this above - CI will continue to install RMS so it will continue to work in the future. Second point is a very real challenge, I agree. I can readily imagine circumstances where a user might suggest changes to RMG that pass RMG-only tests locally and then fail RMG+RMS CI on GitHub. This might ultimately be resolved with good practices (i.e. requiring people to run RMG+RMS for development) but I am open to input from the dev team on this. |
Once the julia imports are avoided running RMG without using julia features will not have a meaningful distinction between running RMG without RMS installed. So aside from installation, there won't be a meaningful distinction in difficulty for users.
I haven't seen the build script you devised, but this at least sounds resolvable to me. It sounds like RMS is being imported before julia is linked to the correct python (which will cause it to start installing things). Julia-python linking is most easily done inside the python you want to link to by calling |
|
Oh wait, sorry...the issue isn't that...the issue is that the python that you're building with in conda is not the python in use when you run the binary. The linking needs done for the run python. Should just be the linking, the package installs and everything should be fine. |
My apologies, I'm not sure I understand what you mean, could you restate this another way?
The the and the recipethough this last component would be a little bit simpler now that some of the deps have been removed. I agree that this should be resolvable, but I'm also anxious about the maintainability of the results. Seems like it would be house of cards waiting to crash down, whereas a system that built an RMG binary independently and then added RMS on top of it would allow for greater flexibility in the two steps. |
|
Sorry, I felt like the reply stuff was getting too nested. On that point, I guess I don't understand why making RMS an optional install makes future development and plug-ability any easier given that after installation the user experience is the same for a user not using those features. Super interested about why you needed that Functors line in the build. Overall on the issues with the build my understanding is that if RMG starts using an RMS feature, RMS tries to install stuff?
So it seems that our problem is simply that while everything we need is installed PyCall doesn't know where the right python/conda env is. This can be set by defining environment variables: PYTHON and maybe also CONDA_JL_HOME (see https://github.com/JuliaPy/PyCall.jl and https://github.com/JuliaPy/Conda.jl). In the build we can set environmental variables that will automatically load when the environment loads so in theory if we do this for PYTHON and CONDA_JL_HOME RMS will grab the appropriate python and find that the packages it needs are already there. An example of doing this from the build file I wrote for julia binaries is below: |
|
At a high level I'm still skeptical that this is possible - the whole point of Conda.jl seems to be building Julia packages that require Python rather than Python Packages that require Julia. I can give this script a shot, but I must emphasize that this is a maintainability concern. As soon as There's also just such a natural separation between mechanism generator in Python + reactor simulator in Julia that it seems (to me) unnecessary to try and combine the two. This would decouple our release schedule, allow people to extend RMG without also installing RMS, etc. To me this is also just occam's razor. Why do this (comparatively) complex installation when we could so easily separate the two? I don't recall the exact reason the |
|
I'm happy to explain how the python-julia system works, why what I proposed is a robust solution to your particular problem as described (which honestly is rare to find with conda-build problems), what role PyCall.jl and Conda.jl play in the system and why there aren't any concerns about maintainability related to them in particular. I know this part is new for the binaries and you guys aren't as experienced with these systems as I am. I'm surprised you didn't bring this up with me earlier in the process when you ran into this issue. I would hope that I could have helped you avoid the frustrations it seems you've clearly run into. I'm well aware building binaries is not a fun process especially when dealing with systems you aren't intimately familiar with. However, I think it would be best to have such a conversation offline. |
Actually, in addition to this we also need to run Pkg.build("PyCall") once in the conda environment after construction...cleanest would be the first time the conda environment activates, although calling julia.install() inside RMG will also do this. |
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:01:05 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Failed Edge Comparison ❌Original model has 106 species. Non-identical thermo! ❌
Identical thermo comments: Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(Cs-(Cds-Cds)(Cds-Cds)(Cds-Cds)H) + group(Cds-Cds(Cds-Cds)(Cds-Cds)) + group(Cds-CdsCsH) + group(Cds-CdsCsH) + group(Cds-Cds(Cds-Cds)H) + group(Cds-Cds(Cds-Cds)H) + group(Cds-CdsCsH) + group(Cdd-CdsCds) + Estimated bicyclic component: polycyclic(s4_6_6_ane) - ring(Cyclohexane) - ring(Cyclohexane) + ring(124cyclohexatriene) + ring(124cyclohexatriene) Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics: Non-identical kinetics! ❌
kinetics:
Observables Test Case: Aromatics Comparison
✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! aromatics Passed Observable Testing ✅Regression test liquid_oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:02:08 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 202 species. Non-identical kinetics! ❌
kinetics:
Observables Test Case: liquid_oxidation Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! liquid_oxidation Passed Observable Testing ✅Regression test nitrogen:Reference: Execution time (DD:HH:MM:SS): 00:00:01:22 nitrogen Passed Core Comparison ✅Original model has 41 species. nitrogen Passed Edge Comparison ✅Original model has 132 species.
Observables Test Case: NC Comparison
✅ All Observables varied by less than 0.200 on average between old model and new model in all conditions! nitrogen Passed Observable Testing ✅Regression test oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:02:20 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species.
Observables Test Case: Oxidation Comparison
✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! oxidation Passed Observable Testing ✅Regression test sulfur:Reference: Execution time (DD:HH:MM:SS): 00:00:00:54 sulfur Passed Core Comparison ✅Original model has 27 species. sulfur Failed Edge Comparison ❌Original model has 89 species.
Observables Test Case: SO2 Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! sulfur Passed Observable Testing ✅Regression test superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:00:34 superminimal Passed Core Comparison ✅Original model has 13 species. superminimal Passed Edge Comparison ✅Original model has 18 species. Regression test RMS_constantVIdealGasReactor_superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:02:26 RMS_constantVIdealGasReactor_superminimal Passed Core Comparison ✅Original model has 13 species. RMS_constantVIdealGasReactor_superminimal Passed Edge Comparison ✅Original model has 13 species.
Observables Test Case: RMS_constantVIdealGasReactor_superminimal Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_constantVIdealGasReactor_superminimal Passed Observable Testing ✅Regression test RMS_CSTR_liquid_oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:06:05 RMS_CSTR_liquid_oxidation Passed Core Comparison ✅Original model has 37 species. RMS_CSTR_liquid_oxidation Failed Edge Comparison ❌Original model has 206 species.
Observables Test Case: RMS_CSTR_liquid_oxidation Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_CSTR_liquid_oxidation Passed Observable Testing ✅Regression test fragment:Reference: Execution time (DD:HH:MM:SS): 00:00:00:39 fragment Passed Core Comparison ✅Original model has 10 species. fragment Passed Edge Comparison ✅Original model has 33 species.
Observables Test Case: fragment Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! fragment Passed Observable Testing ✅Regression test RMS_constantVIdealGasReactor_fragment:Reference: Execution time (DD:HH:MM:SS): 00:00:03:04 RMS_constantVIdealGasReactor_fragment Passed Core Comparison ✅Original model has 10 species. RMS_constantVIdealGasReactor_fragment Passed Edge Comparison ✅Original model has 27 species.
Observables Test Case: RMS_constantVIdealGasReactor_fragment Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_constantVIdealGasReactor_fragment Passed Observable Testing ✅beep boop this comment was written by a bot 🤖 |
JacksonBurns
force-pushed
the
fix/avoid_unnecessary_julia
branch
from
dc1585d
to
24284bc
Compare
JacksonBurns
changed the title
|
Rebased onto the latest cclib CI fix in #2638 |
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:01:05 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Failed Edge Comparison ❌Original model has 106 species. Non-identical thermo! ❌
Identical thermo comments:
Observables Test Case: Aromatics Comparison
✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! aromatics Passed Observable Testing ✅Regression test liquid_oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:02:09 liquid_oxidation Failed Core Comparison ❌Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 202 species. Non-identical kinetics! ❌
kinetics:
Observables Test Case: liquid_oxidation Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! liquid_oxidation Passed Observable Testing ✅Regression test nitrogen:Reference: Execution time (DD:HH:MM:SS): 00:00:01:21 nitrogen Passed Core Comparison ✅Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 132 species. Non-identical thermo! ❌
thermo: Thermo group additivity estimation: group(O2s-CdN3d) + group(N3d-OCd) + group(Cd-HN3dO) + ring(oxirene) + radical(CdJ-NdO) Non-identical kinetics! ❌
kinetics:
Observables Test Case: NC Comparison
✅ All Observables varied by less than 0.200 on average between old model and new model in all conditions! nitrogen Passed Observable Testing ✅Regression test oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:02:23 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species.
Observables Test Case: Oxidation Comparison
✅ All Observables varied by less than 0.500 on average between old model and new model in all conditions! oxidation Passed Observable Testing ✅Regression test sulfur:Reference: Execution time (DD:HH:MM:SS): 00:00:00:54 sulfur Passed Core Comparison ✅Original model has 27 species. sulfur Failed Edge Comparison ❌Original model has 89 species.
Observables Test Case: SO2 Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! sulfur Passed Observable Testing ✅Regression test superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:00:34 superminimal Passed Core Comparison ✅Original model has 13 species. superminimal Passed Edge Comparison ✅Original model has 18 species. Regression test RMS_constantVIdealGasReactor_superminimal:Reference: Execution time (DD:HH:MM:SS): 00:00:02:27 RMS_constantVIdealGasReactor_superminimal Passed Core Comparison ✅Original model has 13 species. RMS_constantVIdealGasReactor_superminimal Passed Edge Comparison ✅Original model has 13 species.
Observables Test Case: RMS_constantVIdealGasReactor_superminimal Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_constantVIdealGasReactor_superminimal Passed Observable Testing ✅Regression test RMS_CSTR_liquid_oxidation:Reference: Execution time (DD:HH:MM:SS): 00:00:06:08 RMS_CSTR_liquid_oxidation Failed Core Comparison ❌Original model has 37 species. RMS_CSTR_liquid_oxidation Failed Edge Comparison ❌Original model has 206 species.
Observables Test Case: RMS_CSTR_liquid_oxidation Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_CSTR_liquid_oxidation Passed Observable Testing ✅Regression test fragment:Reference: Execution time (DD:HH:MM:SS): 00:00:00:40 fragment Passed Core Comparison ✅Original model has 10 species. fragment Passed Edge Comparison ✅Original model has 33 species.
Observables Test Case: fragment Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! fragment Passed Observable Testing ✅Regression test RMS_constantVIdealGasReactor_fragment:Reference: Execution time (DD:HH:MM:SS): 00:00:03:06 RMS_constantVIdealGasReactor_fragment Passed Core Comparison ✅Original model has 10 species. RMS_constantVIdealGasReactor_fragment Passed Edge Comparison ✅Original model has 27 species.
Observables Test Case: RMS_constantVIdealGasReactor_fragment Comparison
✅ All Observables varied by less than 0.100 on average between old model and new model in all conditions! RMS_constantVIdealGasReactor_fragment Passed Observable Testing ✅beep boop this comment was written by a bot 🤖 |
Replaces part of #2539 which 1. fixed the conda recipe 2. added a subpackage 3. avoided unnecesary RMS operations and 4. grew totally un-reviewable. This PR contains only 3. - later PRs will implement 1. and 2.
Description of Changes
Two new internal-only boolean variables are added,
NO_JULIAandrequires_rms:NO_JULIAis set based on whether or not the Julia dependencies (which are now optional) are installed. This resolves Arkane takes a long time to run due to loading Julia dependencies #2547 by allowing for a standalone installation of RMG or Arkane.requires_rmsis set if the user calling RMG actually has a reactor system that requires an RMS-based reactor. This avoids a lot of work (see which lines can now be skipped in this PR) when running RMG from the CLI inside the Docker container (or a source install with RMS).I have also renamed the reactors file for clarity, to differentiate it from reactors.py which has the RMG-based pure-python reactors.
Impact of Changes
As of right now, the CI and Docker image still install Julia and RMS as they do now, so the version of RMG that we send out has all the features RMS offers.
This change is intended for:
TODO Before Merging
Update user-facing docs to provide details on how to do a julia-less installation and indicate which features require Julia.
I am opening and requesting review now so that we can hash out the finer points of the implementation, and then I can write up the docs for the users once that is settled.