Upgrade to OpenMOPAC #2417

JacksonBurns · 2023-04-20T22:24:02Z

The current version of MOPAC used by RMG is semi-closed source and requires an key. The new version, OpenMOPAC, is freely available and maintained by the community.

This PR will attempt to switch to this newer version. Doing so will fix some issues on the user side (see below) and also allow forked repositories to run the continuous integration testing (which currently fails due to the requirement for a license key, not present on forks).

Resolves #2328 which originally discussed the possibility of moving to OpenMOPAC.
Resolves #2251 which was facing issues with running the outdated version of MOPAC and attempted to install this new version without success.
Resolves #2407 also facing licensing issues.

PR #2322 attempted to do this in the past but also attempted to upgrade other packages and ran into difficulties; this PR will only address MOPAC->OpenMOPAC and those packages that are directly affected. This PR seemed to also be blocked by #2272 which fixed an issue with RDKit (see this comment from @xiaoruiDong) that has since been merged.

PR #2386 also attempted to loosen the environment restrictions but was blocked by other PRs that added support for updated versions of Cantera.

codecov · 2023-05-09T15:14:57Z

Codecov Report

Merging #2417 (ddf97ee) into main (f4d7e9c) will increase coverage by 0.01%.
The diff coverage is 58.82%.

@@            Coverage Diff             @@
##             main    #2417      +/-   ##
==========================================
+ Coverage   48.12%   48.13%   +0.01%     
==========================================
  Files         110      110              
  Lines       30653    30625      -28     
  Branches     7995     7988       -7     
==========================================
- Hits        14751    14741      -10     
+ Misses      14371    14358      -13     
+ Partials     1531     1526       -5

Impacted Files	Coverage Δ
rmgpy/qm/gaussian.py	`26.00% <25.00%> (ø)`
rmgpy/qm/symmetry.py	`80.48% <61.11%> (ø)`
rmgpy/qm/molecule.py	`80.81% <72.54%> (ø)`
rmgpy/qm/mopac.py	`73.97% <75.47%> (+6.15%)`	⬆️

... and 1 file with indirect coverage changes

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

hwpang

Looks good to me. Would like @rwest's comments in this before merging.

rwest

Haven't tried compiling or running (but I presume you have). Just read it on github. Looks ok. Please can you push the binaries to the official rmg channel and refer to that in the conda environment file?

rwest · 2023-05-09T17:29:55Z

environment.yml

@@ -34,9 +35,9 @@ dependencies:
 - conda-forge::openbabel >= 3
 - pandas
 - psutil
- - rmg::pydas >=1.0.2
+ - jacksonburns::pydas >=1.0.3


can you push these PyD* conda binaries to the official rmg channel?
(better: is the recipe to build them in an official repo, and even better run automatically?)

I will move them over now, as well as build on a mac.

The recipes are in the ReactionMechanismGenerator/PyD* repos, but are not being built automatically

rwest · 2023-05-09T17:35:20Z

rmgpy/qm/mopac.py

- outputFileExtension = '.out'
-
- executablesToTry = ('MOPAC2016.exe', 'MOPAC2012.exe', 'MOPAC2009.exe', 'mopac')
+ inputFileExtension = ".mop"


should we be making these snake_case?

rmgpy/qm/mopac.py

rwest · 2023-05-09T17:37:20Z

rmgpy/qm/mopac.py

+ mopacEnv = os.getenv("MOPAC_DIR", default="/opt/mopac")
+ executablePath = os.path.join(mopacEnv, "mopac")
+ if not os.path.exists(executablePath):
+ executablePath = os.path.join(mopacEnv, "(MOPAC 2009 or 2012 or 2016)")


Think this line needs revision?

This entire block does not make sense to me, I left it in to spark some discussion.

I think in the previous versions we may have allowed users to specify a different MOPAC executable and then retrieved it from there, but this isn't something we are interested in now since we ship with the latest release.

I will just remove this. It's dead and untested anyway.

JacksonBurns · 2023-05-09T20:26:28Z

@rwest I made the requested changes, I uploaded PyDAS and PyDQED for Linux to the RMG channel, and @hwpang and I uploaded the conda packages for PyDAS and PyDQED for MacOS.

Most of the edits shown in the diff are just from applying PEP8.

rwest · 2023-05-10T17:18:09Z

Regression test error TypeError: quantum_mechanics() got an unexpected keyword argument 'fileStore' ?

this might end up being blocked by the same PRs as before

I have rebuilt pydas and pydqd in an environment with an updated version of libgfortran-ng in hopes that pydas can be resolved with a modern version of MOPAC. this process will need to be repeated for pydqed

- allowed difference is now 0.01% instead of zero decimal places (we can't expect that between different versions anyway)

…lways I think the executable finding thing in mopac.py is going to fail

…th black

@rwest

Comment from review by @rwest Co-authored-by: Richard West <r.west@northeastern.edu>

rwest · 2023-05-10T17:44:24Z

I think I see the problem. Your gigantic PEP8 refactor for all things touching QMSettings commit has changed some things that are part of input file syntax, not just internal variable names. I think we shouldn't make breaking changes like this to an input file format just for style consistency, especially through blindly applying an automated tool, on a PR that's unrelated.

To preserve things in case I messed up I have pushed your branch to a new one called new-mopac-backup which we can delete once everything is resolved. I have then rebased a few times, merging some commits, trying to tidy up a bit, and dropped the gigantic PEP8 refactor commit. I have force-pushed the result to this new-mopac branch, and we'll see if the test suite runs. If I've messed up, feel free to revert to new-mopac-backup and try to fix things yourself.

JacksonBurns · 2023-05-10T17:49:12Z

@rwest thanks for resolving that - I tried to get all the instances fixed but apparently missed some. Agreed that it would be a bad idea to change input file format for something like this. Thanks for taking care of the fix! Should pass.

rwest · 2023-05-10T17:52:28Z

Not yet sure if it's the changes on this branch, or a problem already in main, but making a new conda environment on my mac is faling:

git:(new-mopac) ✗ conda env create -f environment.yml 
Channels:
 - defaults
 - rmg
 - conda-forge
 - cantera
 - pr-omethe-us
 - kyleniemeyer
Platform: osx-64
Collecting package metadata (repodata.json): done
Solving environment: | error    libmamba Selected channel specific (or force-reinstall) job, but package is not available from channel. Solve job will fail.
- error    libmamba Selected channel specific (or force-reinstall) job, but package is not available from channel. Solve job will fail.
failed
Channels:
 - defaults
 - rmg
 - conda-forge
 - cantera
 - pr-omethe-us
 - kyleniemeyer
Platform: osx-64
Collecting package metadata (repodata.json): done
Solving environment: \ error    libmamba Selected channel specific (or force-reinstall) job, but package is not available from channel. Solve job will fail.
| error    libmamba Selected channel specific (or force-reinstall) job, but package is not available from channel. Solve job will fail.
failed

LibMambaUnsatisfiableError: Encountered problems while solving:
  - unsupported request

hwpang · 2023-05-10T17:54:23Z

@rwest

This is probably because of this package version specification

conda-forge::libgfortran-ng>=10.0

JacksonBurns · 2023-05-10T17:56:23Z

Not yet sure if it's the changes on this branch, or a problem already in main, but making a new conda environment on my mac is faling:

git:(new-mopac) ✗ conda env create -f environment.yml 
Channels:
 - defaults
 - rmg
 - conda-forge
 - cantera
 - pr-omethe-us
 - kyleniemeyer
Platform: osx-64
Collecting package metadata (repodata.json): done
Solving environment: | error    libmamba Selected channel specific (or force-reinstall) job, but package is not available from channel. Solve job will fail.
- error    libmamba Selected channel specific (or force-reinstall) job, but package is not available from channel. Solve job will fail.
failed
Channels:
 - defaults
 - rmg
 - conda-forge
 - cantera
 - pr-omethe-us
 - kyleniemeyer
Platform: osx-64
Collecting package metadata (repodata.json): done
Solving environment: \ error    libmamba Selected channel specific (or force-reinstall) job, but package is not available from channel. Solve job will fail.
| error    libmamba Selected channel specific (or force-reinstall) job, but package is not available from channel. Solve job will fail.
failed

LibMambaUnsatisfiableError: Encountered problems while solving:
  - unsupported request

Can you try deleting - conda-forge::libgfortran-ng>=10.0 from the environment file and running again? @hwpang and I ran into a problem while building PyDAS and PyDQED on MacOS yesterday that this package is not available on mac, but that conda finds a different one that fits the requirements. I had only added this to the list to be explicit, but I only ever dev on ubuntu.

This package can't be found on MacOS, and apparently doesn't need to be specified explicitly for linux.

rwest · 2023-05-10T18:02:08Z

I removed that line as you suggest. It seems to solve OK on my mac (though haven't yet checked the resulting environment works). Have pushed it to see if the CI tests pass.

rwest · 2023-05-10T19:38:19Z

This looks promising. Seems to build on my mac, and the CI tests haven't failed.
But we updated these in the environment.yml

   - rmg::pydas >=1.0.3
   - rmg::pydqed >=1.0.3

but the .conda/meta.yaml still has

    - pydas >=1.0.2
    - pydqed >=1.0.1

should that be updated too? Any other changes to conda stuff?
(I'm not up to date with how the conda binary building works)

JacksonBurns · 2023-05-10T19:43:59Z

This looks promising. Seems to build on my mac, and the CI tests haven't failed.
But we updated these in the environment.yml
   - rmg::pydas >=1.0.3
   - rmg::pydqed >=1.0.3
but the .conda/meta.yaml still has
    - pydas >=1.0.2
    - pydqed >=1.0.1
should that be updated too? Any other changes to conda stuff?
(I'm not up to date with how the conda binary building works)

I think they should be updated, I also don't know anything about actually building the RMG binaries - but we don't need to build anaconda RMG binaries anymore since we moved to docker. We could update them for now, with a separate PR to remove the file (likely .conda as a whole)?

rwest · 2023-05-10T19:46:39Z

Any other changes to conda stuff?

I think they should be updated, I also don't know anything about actually building the RMG binaries - but we don't need to build anaconda RMG binaries anymore since we moved to docker. We could update them for now, with a separate PR to remove the file (likely .conda as a whole)?

Let's update the .conda folder in a separate PR. I notice the cantera version got left behind too. Then this PR is more nearly ready to merge (as soon as tests pass?)

JacksonBurns · 2023-05-10T19:49:41Z

Any other changes to conda stuff?

I think they should be updated, I also don't know anything about actually building the RMG binaries - but we don't need to build anaconda RMG binaries anymore since we moved to docker. We could update them for now, with a separate PR to remove the file (likely .conda as a whole)?

Let's update the .conda folder in a separate PR. I notice the cantera version got left behind too. Then this PR is more nearly ready to merge (as soon as tests pass?)

Ok, I will open a PR to update .conda following this. You are correct, we missed this in the Canter 2.6 PR as well.

Yes, assuming this passes it can be merged!

rwest · 2023-05-10T19:54:30Z

A slight confusion - at least confused me for a while - is that the new pydqed 1.0.3 binary you built reports that it is 1.0.1 when you run make check.

(rmg_env) ➜  RMG-Py git:(new-mopac) ✗ conda list pydqed
# packages in environment at /opt/miniconda3/envs/rmg_env:
#
# Name                    Version                   Build  Channel
pydqed                    1.0.3            py37h41275ba_6    rmg

(rmg_env) ➜  RMG-Py git:(new-mopac) ✗ make check

Checking vital dependencies...

Package        Version        Location
lpsolve55                     /opt/miniconda3/envs/rmg_env/lib/python3.7/site-packages/lpsolve55.cpython-37m-darwin.so
OpenBabel      3.1.0          /opt/miniconda3/envs/rmg_env/lib/python3.7/site-packages/openbabel/openbabel.py
pydqed         1.0.1          /opt/miniconda3/envs/rmg_env/lib/python3.7/site-packages/pydqed.cpython-37m-darwin.so
pyrdl                         /opt/miniconda3/envs/rmg_env/lib/python3.7/site-packages/py_rdl/__init__.py
RDKit          2020.03.3      /opt/miniconda3/envs/rmg_env/lib/python3.7/site-packages/rdkit/__init__.py
symmetry       1.16           /opt/miniconda3/envs/rmg_env/bin/symmetry

Everything was found :)

hwpang · 2023-05-10T19:57:50Z

A slight confusion - at least confused me for a while - is that the new pydqed 1.0.3 binary you built reports that it is 1.0.1 when you run make check.

(rmg_env) ➜  RMG-Py git:(new-mopac) ✗ conda list pydqed
# packages in environment at /opt/miniconda3/envs/rmg_env:
#
# Name                    Version                   Build  Channel
pydqed                    1.0.3            py37h41275ba_6    rmg

(rmg_env) ➜  RMG-Py git:(new-mopac) ✗ make check

Checking vital dependencies...

Package        Version        Location
lpsolve55                     /opt/miniconda3/envs/rmg_env/lib/python3.7/site-packages/lpsolve55.cpython-37m-darwin.so
OpenBabel      3.1.0          /opt/miniconda3/envs/rmg_env/lib/python3.7/site-packages/openbabel/openbabel.py
pydqed         1.0.1          /opt/miniconda3/envs/rmg_env/lib/python3.7/site-packages/pydqed.cpython-37m-darwin.so
pyrdl                         /opt/miniconda3/envs/rmg_env/lib/python3.7/site-packages/py_rdl/__init__.py
RDKit          2020.03.3      /opt/miniconda3/envs/rmg_env/lib/python3.7/site-packages/rdkit/__init__.py
symmetry       1.16           /opt/miniconda3/envs/rmg_env/bin/symmetry

Everything was found :)

We probably need to update this line: https://github.com/ReactionMechanismGenerator/PyDQED/blob/libfortran-update/pydqed.pyx#L48 to 1.0.3

JacksonBurns · 2023-05-10T20:00:17Z

The anaconda package is 1.0.3 but we made no changes to PyDQED itself so it still shows the old version. We can update the version in PyDQED to match the conda package, but it does not affect performance.

rwest

Not a huge fan of the code churn from autoformatting, but there's no good answer really. At least once it's done it's done. I noticed one inconsistency but it's minor. I think this is fine to merge, in my opinion.

rwest · 2023-05-10T21:16:20Z

rmgpy/qm/gaussian.py


- executablesToTry = ('g16', 'g09', 'g03')
+ executablesToTry = ("g16", "g09", "g03")


if we're going through all the code churn of running black on the file and changing ' to " etc., should we also change executablesToTry and possibleDirs (below) to snake_case?
(I'm not a huge far of the un-necessarry code churn of black).

JacksonBurns added Type: Deprecation Complexity: Medium Status: WIP This is currently work-in-progress labels Apr 20, 2023

JacksonBurns marked this pull request as draft April 21, 2023 00:13

JacksonBurns mentioned this pull request May 8, 2023

CI.yml and environment.yml updates to run CI on MacOS and Ubuntu #2415

Merged

JacksonBurns requested review from hwpang and rwest May 9, 2023 15:51

JacksonBurns marked this pull request as ready for review May 9, 2023 16:00

hwpang previously approved these changes May 9, 2023

View reviewed changes

rwest reviewed May 9, 2023

View reviewed changes

JacksonBurns dismissed hwpang’s stale review via 5a00455 May 9, 2023 18:14

JacksonBurns requested review from rwest and hwpang May 9, 2023 20:24

JacksonBurns and others added 13 commits May 10, 2023 13:24

switch to conda-force mopac in environment.yml, skip install step in CI

a88238b

add libfortran for mopac, float cantera to potentially unsupported ver.

0915745

this might end up being blocked by the same PRs as before

Use newer pydas and pydqed.

77be5e5

I have rebuilt pydas and pydqd in an environment with an updated version of libgfortran-ng in hopes that pydas can be resolved with a modern version of MOPAC. this process will need to be repeated for pydqed

change mopac unit tests

f38d45a

- allowed difference is now 0.01% instead of zero decimal places (we can't expect that between different versions anyway)

update documentation regarding use of MOPAC

2068326

delete NO_LICENSE and NO_MOPAC checks since it will now be included a…

0fe291c

…lways I think the executable finding thing in mopac.py is going to fail

format mopac.py with black, remove checks for old unsupported versions

ebea50e

fix typo (delta not selta), decrease required close enough, format wi…

853a9cb

…th black

set reproduce threshold to half a percent

dd98e07

Update rmgpy/qm/mopac.py

795de00

Comment from review by @rwest Co-authored-by: Richard West <r.west@northeastern.edu>

black formatting, rename variables for PEP8

101f8b5

module-level point group dictionary should be all caps (PEP8)

3b444a9

remove dead, untested, and undocumented MOPAC_DIR lines

b20c813

rwest force-pushed the new-mopac branch from cc17997 to b20c813 Compare May 10, 2023 17:42

Remove conda-forge::libgfortran-ng>=10.0 from environment.yml for MacOS

ddf97ee

This package can't be found on MacOS, and apparently doesn't need to be specified explicitly for linux.

rwest approved these changes May 10, 2023

View reviewed changes

JacksonBurns mentioned this pull request May 10, 2023

Update RMG website environmental file ReactionMechanismGenerator/RMG-website#253

Merged

hwpang merged commit 8cfbf5c into main May 10, 2023

hwpang deleted the new-mopac branch May 10, 2023 21:49

This was referenced May 12, 2023

multiprocessing: TypeError: __init__() takes from 1 to 7 positional arguments but 8 were given #2342

Closed

Mopac .out file missing from temp directory #2023

Closed

Create YAML file outputs in RMG for use in Cantera #2321

Draft

rwest mentioned this pull request May 22, 2023

Updating Conda environment requirements #2322

Closed

2 tasks

JacksonBurns mentioned this pull request May 23, 2023

MOPAC no longer uses Licenses #2215

Closed

JacksonBurns mentioned this pull request Jun 21, 2023

Parallelization #2021

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to OpenMOPAC #2417

Upgrade to OpenMOPAC #2417

JacksonBurns commented Apr 20, 2023

codecov bot commented May 9, 2023 •

edited

Loading

hwpang left a comment

rwest left a comment

rwest May 9, 2023

JacksonBurns May 9, 2023

rwest May 9, 2023

rwest May 9, 2023

JacksonBurns May 9, 2023

JacksonBurns commented May 9, 2023

rwest commented May 10, 2023

rwest commented May 10, 2023

JacksonBurns commented May 10, 2023

rwest commented May 10, 2023

hwpang commented May 10, 2023

JacksonBurns commented May 10, 2023

rwest commented May 10, 2023

rwest commented May 10, 2023

JacksonBurns commented May 10, 2023

rwest commented May 10, 2023

JacksonBurns commented May 10, 2023

rwest commented May 10, 2023

hwpang commented May 10, 2023

JacksonBurns commented May 10, 2023

rwest left a comment

rwest May 10, 2023


		executablesToTry = ('g16', 'g09', 'g03')
		executablesToTry = ("g16", "g09", "g03")

Upgrade to OpenMOPAC #2417

Upgrade to OpenMOPAC #2417

Conversation

JacksonBurns commented Apr 20, 2023

codecov bot commented May 9, 2023 • edited Loading

Codecov Report

hwpang left a comment

Choose a reason for hiding this comment

rwest left a comment

Choose a reason for hiding this comment

rwest May 9, 2023

Choose a reason for hiding this comment

JacksonBurns May 9, 2023

Choose a reason for hiding this comment

rwest May 9, 2023

Choose a reason for hiding this comment

rwest May 9, 2023

Choose a reason for hiding this comment

JacksonBurns May 9, 2023

Choose a reason for hiding this comment

JacksonBurns commented May 9, 2023

rwest commented May 10, 2023

rwest commented May 10, 2023

JacksonBurns commented May 10, 2023

rwest commented May 10, 2023

hwpang commented May 10, 2023

JacksonBurns commented May 10, 2023

rwest commented May 10, 2023

rwest commented May 10, 2023

JacksonBurns commented May 10, 2023

rwest commented May 10, 2023

JacksonBurns commented May 10, 2023

rwest commented May 10, 2023

hwpang commented May 10, 2023

JacksonBurns commented May 10, 2023

rwest left a comment

Choose a reason for hiding this comment

rwest May 10, 2023

Choose a reason for hiding this comment

codecov bot commented May 9, 2023 •

edited

Loading