importing pyrms and diffeqpy during a debug session by calvinp0 · Pull Request #2364 · ReactionMechanismGenerator/RMG-Py

calvinp0 · 2023-01-27T13:35:42Z

Motivation or Problem

Updated the import of Julia so that now it will first check if it is in debug mode. If so, will import Julia() first prior to import pyrms/diffeqpy. Furthermore, during debug mode, will look for system image of RMS is exists. Refer to #2363

Description of Changes

I have edited reactors.py to now determine if the user is debugging. If the user is not debugging, it will follow the original code, trying importing pyrms/diffeqpy. If exception is met, it will pass (this is obviously important for testing in ARC and T3).

However, if the user is debugging, it will attempt to get the top path of the reactors.py file and then search for a system image of rms.so. If not found, it will load Julia without precompiled modules. This allows for pyrms and diffeqpy to be imported.

alongd

Overall looks good to me

I could suggest to begin with if gettrace(), and then regardless of the value of gettrace run the lines that were deleted here. Just a minor suggestion to avoid repetition

mjohnson541 · 2023-01-27T21:44:16Z

Looks good to me as well!

calvinp0 · 2023-01-29T17:22:00Z

I am getting an fail in the ci/rmg_tests. During a test, it says cannot find 'rms'
And looking at the install of RMS, I see this

ERROR: Unsatisfiable requirements detected for package StochasticDiffEq [789caeaf]:
 StochasticDiffEq [789caeaf] log:
 ├─possible versions are: 5.0.0-6.57.4 or uninstalled
 ├─restricted to versions 6.36.0 by an explicit requirement, leaving only versions 6.36.0
 └─restricted by compatibility requirements with OrdinaryDiffEq [1dea7af3] to versions: [5.0.0-6.20.0, 6.43.0-6.57.4] or uninstalled — no versions left
   └─OrdinaryDiffEq [1dea7af3] log:
     ├─possible versions are: 4.0.0-6.41.0 or uninstalled
     └─restricted to versions 6 by ReactionMechanismSimulator [c2d78dd2], leaving only versions 6.0.0-6.41.0
       └─ReactionMechanismSimulator [c2d78dd2] log:
         ├─possible versions are: 0.4.0 or uninstalled
         └─ReactionMechanismSimulator [c2d78dd2] is fixed to version 0.4.0

mjohnson541 · 2023-01-29T20:50:52Z

No idea why this popped up now instead of earlier, but this seems to be the result of us pinning the StochasticDiffEq.jl version in the RMG-tests install script at some point. This isn't done in the latest CI build and documentation build scripts, which run successfully. Removing the part of the lines here: https://github.com/ReactionMechanismGenerator/RMG-tests/blob/10da9f5656baeb31091cb287c232b4f9a0aca7e4/install.sh#L114 that install a specific version of StochasticDiffEq.jl should fix the build.

calvinp0 · 2023-02-01T13:29:15Z

@mjohnson541 Okay, so do you reckon I should make the changes in this branch or make another branch for that change regarding the unpinning of StoachasticDiffEq

mjohnson541 · 2023-02-01T17:12:42Z

RMG-tests is a different repository from RMG-Py so you won't be able to put the changes on this branch. I can review the PR.

mjohnson541 · 2023-02-01T17:15:29Z

It's interesting that all RMG-tests runs aren't failing: https://github.com/ReactionMechanismGenerator/RMG-tests/actions/runs/4060446913/jobs/6989541587

calvinp0 · 2023-02-01T18:22:26Z

Appears I don't have permissions to push a branch to RMG-tests.

But yeah, it is concerning to me that my change is having a failure during the installation of RMS. I can't imagine how my code change affects in particular

calvinp0 · 2023-02-01T18:25:01Z

@mjohnson541 I checked the link and it also has the same issue

ERROR: Unsatisfiable requirements detected for package StochasticDiffEq [789caeaf]:
 StochasticDiffEq [789caeaf] log:
 ├─possible versions are: 5.0.0-6.57.4 or uninstalled
 ├─restricted to versions 6.36.0 by an explicit requirement, leaving only versions 6.36.0
 └─restricted by compatibility requirements with OrdinaryDiffEq [1dea7af3] to versions: [5.0.0-6.20.0, 6.43.0-6.57.4] or uninstalled — no versions left
   └─OrdinaryDiffEq [1dea7af3] log:
     ├─possible versions are: 4.0.0-6.41.0 or uninstalled
     └─restricted to versions 6 by ReactionMechanismSimulator [c2d78dd2], leaving only versions 6.0.0-6.41.0
       └─ReactionMechanismSimulator [c2d78dd2] log:
         ├─possible versions are: 0.4.0 or uninstalled
         └─ReactionMechanismSimulator [c2d78dd2] is fixed to version 0.4.0

Yet, the tests seem to be okay with finding RMS

mjohnson541 · 2023-02-01T18:54:06Z

You should have permissions now.

mjohnson541 · 2023-02-01T18:55:47Z

I don't think RMG-tests has any examples yet that use RMS reactors so RMG-tests probably runs fine as long as it doesn't throw an error when the import fails.

calvinp0 · 2023-02-02T11:22:27Z

Yeah so I am a bit confused why my changes suddenly throws the rms error.

Is RMG-tests based upon the tests for RMG-Py? I am just wondering how I could debug it to see where the issue is occuring

mjohnson541 · 2023-02-02T18:05:31Z

Hopefully that will resolve the tests issue. RMG-tests runs a set of RMG jobs and looks at how estimation, species selection and some predictions have changed.

alongd · 2023-02-03T21:15:24Z

@Laxzal , can you try rebasing to fix the branch's history? I can help offline if needed

calvinp0 · 2023-02-06T11:35:05Z

@alongd It should be fixed now

@mjohnson541
I am unsure if this is because of the removal of the version pin but RMG-tests now gets this error

ERROR: InitError: PyError (PyImport_ImportModule
The Python package rmgpy.molecule could not be imported by pyimport. Usually this means
that you did not install rmgpy.molecule in the Python version being used by PyCall.
PyCall is currently configured to use the Python version at:
/usr/share/miniconda/envs/testing/bin/python
and you should use whatever mechanism you usually use (apt-get, pip, conda,
etcetera) to install the Python package containing the rmgpy.molecule module.
One alternative is to re-configure PyCall to use a different Python
version on your system: set ENV["PYTHON"] to the path/name of the python
executable you want to use, run Pkg.build("PyCall"), and re-launch Julia.
Another alternative is to configure PyCall to use a Julia-specific Python
distribution via the Conda.jl package (which installs a private Anaconda
Python distribution), which has the advantage that packages can be installed
and kept up-to-date via Julia.  As explained in the PyCall documentation,
set ENV["PYTHON"]="", run Pkg.build("PyCall"), and re-launch Julia. Then,
To install the rmgpy.molecule module, you can use `pyimport_conda("rmgpy.molecule", PKG)`,
where PKG is the Anaconda package that contains the module rmgpy.molecule,
or alternatively you can use the Conda package directly (via
`using Conda` followed by `Conda.add` etcetera).
) <class 'ModuleNotFoundError'>
ModuleNotFoundError("No module named 'rmgpy.rmgobject'")
  File "/home/runner/work/RMG-tests/RMG-tests/code/testing/RMG-Py/rmgpy/molecule/__init__.py", line 31, in <module>
    from rmgpy.molecule.element import Element, PeriodicSystem, get_element
  File "/home/runner/work/RMG-tests/RMG-tests/code/testing/RMG-Py/rmgpy/molecule/element.py", line 47, in <module>
    from rmgpy.quantity import Quantity
  File "/home/runner/work/RMG-tests/RMG-tests/code/testing/RMG-Py/rmgpy/quantity.py", line 43, in <module>
    from rmgpy.rmgobject import RMGObject, expand_to_dict
Stacktrace:
 [1] pyimport(name::String)
   @ PyCall /usr/share/miniconda/envs/testing/share/julia/site/packages/PyCall/twYvK/src/PyCall.jl:558
 [2] pyimport_conda(modulename::String, condapkg::String, channel::String)
   @ PyCall /usr/share/miniconda/envs/testing/share/julia/site/packages/PyCall/twYvK/src/PyCall.jl:716
 [3] __init__()
   @ ReactionMechanismSimulator /usr/share/miniconda/envs/testing/share/julia/site/packages/ReactionMechanismSimulator/eFaj4/src/ReactionMechanismSimulator.jl:22
 [4] _include_from_serialized(path::String, depmods::Vector{Any})
   @ Base ./loading.jl:696
 [5] _require_search_from_serialized(pkg::Base.PkgId, sourcepath::String)
   @ Base ./loading.jl:782
 [6] _require(pkg::Base.PkgId)
   @ Base ./loading.jl:1020
 [7] require(uuidkey::Base.PkgId)
   @ Base ./loading.jl:936
 [8] require(into::Module, mod::Symbol)
   @ Base ./loading.jl:923
during initialization of module ReactionMechanismSimulator

Could be the LD_LIBRARY_PATH issue?

codecov · 2023-02-06T12:46:03Z

Codecov Report

Merging #2364 (a8c4127) into main (7f26746) will increase coverage by 0.01%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##             main    #2364      +/-   ##
==========================================
+ Coverage   48.24%   48.25%   +0.01%     
==========================================
  Files         110      110              
  Lines       30612    30616       +4     
  Branches     7982     7982              
==========================================
+ Hits        14769    14774       +5     
  Misses      14298    14298              
+ Partials     1545     1544       -1

Impacted Files	Coverage Δ
rmgpy/rmg/reactors.py	`20.98% <90.00%> (+1.03%)`	⬆️
arkane/kinetics.py	`12.24% <0.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

calvinp0 · 2023-02-06T12:51:16Z

Looks like the PR passed. I think the changes to the debugging checked fixed it? I am not certain though

mjohnson541 · 2023-02-06T17:17:20Z

Yeah...I think there's still an issue on RMG-tests, it can probably be fixed just by making the install more and more like the CI builds until it's fixed, but since we aren't using RMS reactors RMG-tests seems to be able to avoid breaking, which is enough for this PR.

Can you squash and write an informative commit message?

- The change of importing of julia and related julia packages is now possible during an IDE debug mode. This is possible due to a conditional `__debug__` statement - if the user is attempting to debug with an IDE, the statement will return true. - System image of rms Additionally, if the user has an rms system image, 'rms.so', and they are using an IDE to debug, then the Julia function will utilise the system image

calvinp0 · 2023-02-06T17:57:53Z

Yeah...I think there's still an issue on RMG-tests, it can probably be fixed just by making the install more and more like the CI builds until it's fixed, but since we aren't using RMS reactors RMG-tests seems to be able to avoid breaking, which is enough for this PR.

Can you squash and write an informative commit message?

No worries. I have squashed and updated the commit message

kfir4444

Looks great @Laxzal

This should have no material change, just a refactoring to make it easier to understand the control flow (which now that I understand, doesn't make much sense to me). It was introduced in #2364 These timing tests are with julia 1.8.5 h245b042_0 conda-forge pyjulia 0.6.1 pyhd8ed1ab_0 conda-forge time python-jl rmg.py test/regression/superminimal/input.py on my 2019 MacBook Pro 2.6Ghz i7 reports 229.82s user 19.77s system 109% cpu 3:48.45 total although RMG reports Execution time (DD:HH:MM:SS): 00:00:00:40 i.e. it's spent 3 minutes 8 seconds (84% of the total) compiling julia stuff that's never used. By running python instead of python-jl python rmg.py test/regression/superminimal/input.py you can get it to run, and it compiles a bunch of julia stuff, with more logging, and takes (two runs) 486.58s user 17.46s system 105% cpu 7:58.05 total 499.59s user 17.69s system 105% cpu 8:12.54 total (again, only 35s reported for RMG Execution time) And by running python -O rmg.py test/regression/superminimal/input.py you can get it to run the "if not __debug__:" branch which skips the Julia runtime making steps, and causes it to print out some errors like julia.core.UnsupportedPythonError: It seems your Julia and PyJulia setup are not supported. and crash. Later, running time python-jl rmg.py test/regression/superminimal/input.py I get a lot of warnings like WARNING: Method definition getGibbs(P, N) where {N<:Number, P<:ReactionMechanismSimulator.AbstractThermo} in module ReactionMechanismSimulator at /opt/miniconda3/envs/rmg_env4/share/julia/packages/ReactionMechanismSimulator/xoDOp/src/Calculators/Thermo.jl:6 overwritten on the same line (check for duplicate calls to `include`). ** incremental compilation may be fatally broken for this module ** and then 1143.19s user 82.00s system 98% cpu 20:47.85 total

calvinp0 requested review from alongd, kfir4444 and mjohnson541 January 27, 2023 13:35

alongd reviewed Jan 27, 2023

View reviewed changes

calvinp0 force-pushed the Import_pyrms_during_debug branch from 58952ea to 5e6b6c0 Compare February 2, 2023 11:31

calvinp0 force-pushed the Import_pyrms_during_debug branch 2 times, most recently from a00afae to c7ccda9 Compare February 5, 2023 12:44

calvinp0 requested a review from alongd February 6, 2023 12:50

calvinp0 force-pushed the Import_pyrms_during_debug branch 2 times, most recently from 73bad55 to a55a669 Compare February 6, 2023 17:55

calvinp0 force-pushed the Import_pyrms_during_debug branch from a55a669 to a8c4127 Compare February 6, 2023 17:56

kfir4444 approved these changes Mar 2, 2023

View reviewed changes

ChrisBNEU merged commit 9f9bc89 into main Mar 2, 2023

ChrisBNEU deleted the Import_pyrms_during_debug branch March 2, 2023 16:48

ChrisBNEU mentioned this pull request Mar 3, 2023

Nosetests on Mac fail #2392

Closed

Conversation

calvinp0 commented Jan 27, 2023

Motivation or Problem

Description of Changes

Uh oh!

alongd left a comment

Choose a reason for hiding this comment

Uh oh!

mjohnson541 commented Jan 27, 2023

Uh oh!

calvinp0 commented Jan 29, 2023

Uh oh!

mjohnson541 commented Jan 29, 2023

Uh oh!

calvinp0 commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjohnson541 commented Feb 1, 2023

Uh oh!

mjohnson541 commented Feb 1, 2023

Uh oh!

calvinp0 commented Feb 1, 2023

Uh oh!

calvinp0 commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjohnson541 commented Feb 1, 2023

Uh oh!

mjohnson541 commented Feb 1, 2023

Uh oh!

calvinp0 commented Feb 2, 2023

Uh oh!

mjohnson541 commented Feb 2, 2023

Uh oh!

alongd commented Feb 3, 2023

Uh oh!

calvinp0 commented Feb 6, 2023

Uh oh!

codecov bot commented Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

calvinp0 commented Feb 6, 2023

Uh oh!

mjohnson541 commented Feb 6, 2023

Uh oh!

calvinp0 commented Feb 6, 2023

Uh oh!

kfir4444 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

calvinp0 commented Feb 1, 2023 •

edited

Loading

calvinp0 commented Feb 1, 2023 •

edited

Loading

codecov bot commented Feb 6, 2023 •

edited

Loading