Add environment validation and error handling to RMS installation script #2828
Conversation
|
Can you edit the docker file so that it will still build with this new script? I think you will just need to set the environment variable you added inside the dockerfile |
|
Also, the reason we didn't set Current is that Julia would break the environment when allowed to install its own packages into rmg_env. The exact reason is buried somewhere in the comments of the Python 3.9 upgrade, but @rwest might recall. My first thought is that it broke sundials, but I can run through that PR tomorrow and check. |
|
Reading
We could just set the path right and install the required packages? |
This is what we currently do, though we let |
The main problem is when installing RMS, the correct Julia environment was not activated, even though we set the |
ssun30
changed the title
|
I'm not against adding something to RMG to make co-developing RMG and RMS easier, but I'm not sure it needs to be an automated script like this. I believe a better fit would be some documentation for developers pointing out how to follow the same 'spirit' as the IMO an automated script to handle a complex developer install doesn't make a lot of sense. Someone looking to do something this complicated should be able to run the commands on their own, especially with some help from a nice docs page. |
|
Some of these additions I'm definitely a fan of. I like checking things more thoroughly too. As a middle ground the "dev" install could perhaps be triggered just by setting environment variables before running the install_rms script rather than it asking everybody what type of install they want? And yes we need better documentation. 👍 |
I have made "standard" mode default so it won't be asking the user which mode to use. Now the "developer" mode will only be used if the user use the command |
ssun30
force-pushed
the
fix_install_rms
branch
2 times, most recently
from
efb971c to
3cc8470
Compare
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:50 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Passed Edge Comparison ✅Original model has 106 species. DetailsObservables 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:01 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 214 species. DetailsObservables 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:04 nitrogen Passed Core Comparison ✅Original model has 41 species. nitrogen Passed Edge Comparison ✅Original model has 133 species. DetailsObservables 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:01:51 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species. DetailsObservables 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 ✅Errors occurred during observable testing
WARNING:root:Initial mole fractions do not sum to one; normalizing.
|
ssun30
force-pushed
the
fix_install_rms
branch
from
69554d1 to
0353123
Compare
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:50 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Failed Edge Comparison ❌Original model has 106 species. Non-identical thermo! ❌
Identical thermo comments: DetailsObservables 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:01:59 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 214 species. Non-identical kinetics! ❌
kinetics: DetailsObservables 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:03 nitrogen Failed Core Comparison ❌Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 133 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: DetailsObservables 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:01:50 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species. DetailsObservables 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 ✅Errors occurred during observable testing
WARNING:root:Initial mole fractions do not sum to one; normalizing.
|
ssun30
force-pushed
the
fix_install_rms
branch
from
0353123 to
3f93536
Compare
rwest
force-pushed
the
fix_install_rms
branch
from
b53d63c to
1b1624a
Compare
|
I've just rebased this onto main, which required resolving a couple of merge conflicts. I'm curious about why we want the last commit "Replace return 1 with exit 1" given the commit message in c46a861 |
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:48 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Failed Edge Comparison ❌Original model has 106 species. Non-identical thermo! ❌
Identical thermo comments: DetailsObservables 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:01:57 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 214 species. Non-identical kinetics! ❌
kinetics: DetailsObservables 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:00:59 nitrogen Failed Core Comparison ❌Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 133 species. DetailsObservables 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:01:47 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species. DetailsObservables 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 ✅Errors occurred during observable testing
WARNING:root:Initial mole fractions do not sum to one; normalizing.
|
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:48 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Failed Edge Comparison ❌Original model has 106 species. Non-identical thermo! ❌
Identical thermo comments: DetailsObservables 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:01:57 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 214 species. DetailsObservables 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:00:59 nitrogen Failed Core Comparison ❌Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 133 species. DetailsObservables 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:01:47 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species. DetailsObservables 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 ✅Errors occurred during observable testing
WARNING:root:Initial mole fractions do not sum to one; normalizing.
|
Added developer install option for install_rms.sh
Initialize Julia environment before installing RMS Add extra debug info
Developer mode will only be enabled if the user specify "developer" option for install_rms.sh
As explained in c46a861 we don't want to terminate the shell that's running this script, we just want to exit the script, because our instructions are to "source install_rms.sh" which runs it in the current shell.
There was some inconsistency about whether this would be an environment variable or an argument to the script. Hopefully this is clearer. Call it like RMS_INSTALLER=developer source install_rms.sh or set an environment variable first like export RMS_INSTALLER=continuous source install_rms.sh Options are: standard (default) continuous (doesn't pause for confirmation prompts) developer (asks for path to RMS)
Rather than always ask the user to type in the path, they can set it with a variable before hand. We also check it looks like the right sort of path.
Should also add to the installation instructions.
Use quoted heredoc delimiters ('EOF' instead of EOF) to prevent bash from
interpreting backslashes and variable substitutions within Julia code blocks.
This fixes ParseError caused by escaped quotes being consumed by bash.
Also changed to access shell variables via Julia's ENV dict for clarity.
Seemed to be a problem only on the macos github runner.
Hopefully this works cross platform.
Replace bash-specific `read -p` syntax with portable `printf` + `read` combination that works in both bash and zsh. This fixes "no coprocess" error when sourcing the script in zsh shells.
Previously the (which conda) was returning, on macos, some garbage shell function or alias, leading to this mess:
JULIA_CONDAPKG_BACKEND=Null
JULIA_CONDAPKG_EXE=conda () {
\local cmd="${1-__missing__}"
case "$cmd" in
(activate | deactivate) __conda_activate "$@" ;;
eturn (install | update | upgrade | remove | uninstall) __conda_exe "$@" ||
__conda_activate reactivate ;;
(*) __conda_exe "$@" ;;
esac
}
JULIA_PYTHONCALL_EXE=/Users/rwest/miniconda3/envs/rmg_env15/bin/python
PYTHON_JULIAPKG_EXE=/Users/rwest/.juliaup/bin/julia
PYTHON_JULIAPKG_PROJECT=/Users/rwest/miniconda3/envs/rmg_env15/julia_env
which totally messed up the conda environment, making it impossible to activate.
This method now finds the actual conda excutable.
I'm sure could be better written, but this is better than nothing. I also fixed a small bug that sphinx was complaining about in the releaseNotes
rwest
force-pushed
the
fix_install_rms
branch
from
0b7fb02 to
ccc0f97
Compare
rwest
left a comment
rwest
left a comment
There was a problem hiding this comment.
I now approve, but I made the last dozen or so commits, so someone else should review it. This new script works for me on MacOS with zsh (surprisingly many differences) and works on the Github Actions CI. Would be good if reviewers could try it in other settings too.
Regression Testing Results
Detailed regression test results.Regression test aromatics:Reference: Execution time (DD:HH:MM:SS): 00:00:00:48 aromatics Passed Core Comparison ✅Original model has 15 species. aromatics Failed Edge Comparison ❌Original model has 106 species. Non-identical thermo! ❌
Identical thermo comments: DetailsObservables 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:01:57 liquid_oxidation Passed Core Comparison ✅Original model has 37 species. liquid_oxidation Failed Edge Comparison ❌Original model has 214 species. Non-identical kinetics! ❌
kinetics: DetailsObservables 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:00:59 nitrogen Failed Core Comparison ❌Original model has 41 species. nitrogen Failed Edge Comparison ❌Original model has 133 species. DetailsObservables 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:01:47 oxidation Passed Core Comparison ✅Original model has 59 species. oxidation Passed Edge Comparison ✅Original model has 230 species. DetailsObservables 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 ✅Errors occurred during observable testing
WARNING:root:Initial mole fractions do not sum to one; normalizing.
|
documentation/source/users/rmg/installation/anacondaDeveloper.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Jackson Burns <[email protected]>
3 participants
Added developer install option for install_rms.sh
Motivation or Problem
Some of our group members have noticed that they have a Python compatibility problem when installing RMS. It seems that their default system Python version is 3.12 or newer. I've identified the problem to be the
JULIA_CONDAPKG_BACKENDenvironment variable, which is set to "Null", meaning it's using whichever Python is currently installed and in the user's PATH, and the user must have already installed any Python packages that they need.The intended behavior is for
CondaPkgto use the currently activated Conda environment (say, rmg_env) and use the Python binaries within that environment, so it ensures compatibility. For that, the correct environment variable value should be "Current". Furthermore, theJULIA_CONDAPKG_EXEvariable needs to be specified (to $which conda).Update: later discussion below reverts this change. "Null" is correct, but there were other issues.
Description of Changes
CondaPkgconfigurations to use the correct backend.install_rms.shto use a developer build of RMS.