Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Build a rmgmolecule Subpackage in Place, fix rmg conda recipe, initial SimulatorInterface Efforts #2539

Closed
wants to merge 17 commits into from
Closed

Build a rmgmolecule Subpackage in Place, fix rmg conda recipe, initial SimulatorInterface Efforts #2539

wants to merge 17 commits into from

Conversation

JacksonBurns
Copy link
Contributor

@JacksonBurns JacksonBurns commented Sep 12, 2023 •
edited
Loading

Summary

This PR is attempting to do 3 things:

  1. Build a subpackage from RMG-Py called rmgmolecule, which contains just the code contained in rmgpy/molecule, so that other softwares can use it without having to install all of RMG-Py.
  2. Fix the RMG conda recipe, since doing 1. required setting up custom actions runners anyway and it makes organizational sense to pair these efforts.
  3. Separate the pure-python and mixed-python components of RMG (namely RMG and RMS) via a SimulatorInterface, which will allow users to install RMG without installing RMS and its additional requirements.
    • this became necessary to build the conda package (2.), since Julia/PyJulia/RMS could not work
    • see Arkane takes a long time to run due to loading Julia dependencies #2547 for an example of why having RMS included by default is expensive
    • this opens the door to eventually writing a SimulatorInterface class which will allow users to 'plug-in' whatever simulators they want, but for the time being the solution is going to look more like "if you want RMS, follow these additional installation instructions after installing RMG" (which are going to be the dev installation instructions for RMS as given in RMG)

This was originally small in scope but these sub-goals have required one another to proceed.

TODO

  • clean up the git history, which currently contains a number of experimental commits needed to test the self-hosted runner and CI
  • actually fix the conda build for RMG, which is currently broken because the testing environment resolves Julia, which is broken
  • check if rmgmolecule can be built on windows, and or more version of python/numpy/etc., since it now has lighter dependencies
  • also lighten the dependencies for rmgmolecule
  • change reactors.py to julia_reactors.py, bury the import statement and add a runtime warning when attempting to import it that says RMS must be installed, and then update the docs to explain how to install RMS
  • update the CI to perform the default RMG install and then the RMS installation steps to be described in the above point, and then continue executing tests as we are right now (covering RMS anyway)

@codecov

This comment was marked as off-topic.

Sign in to view
rwest
rwest reviewed Sep 14, 2023
View reviewed changes
Copy link
Member

@rwest rwest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks promising.

When would we publish new conda binaries of the molecule package?
I suggest we try building it on every push to detect errors, but presumably only want to publish a new "release" whenever something significant changes that RMS/pynta/etc. or another package is depending on? Would this be done by just incrementing a minor version number in a yml file or something? We might wish to do it more often than a full RMG release.

.rmgmolecule_conda/conda_build_config.yaml Outdated Show resolved Hide resolved
rmgpy/molecule/draw.py Outdated Show resolved Hide resolved
@JacksonBurns JacksonBurns force-pushed the molecule_subpackage branch 2 times, most recently from 4ced2b0 to bd8f7c3 Compare September 17, 2023 00:16
@JacksonBurns JacksonBurns changed the title Build a rmgmolecule Subpackage in Place Build a rmgmolecule Subpackage in Place, fic rmg conda recipe Sep 17, 2023
@JacksonBurns JacksonBurns changed the title Build a rmgmolecule Subpackage in Place, fic rmg conda recipe Build a rmgmolecule Subpackage in Place, fix rmg conda recipe Sep 17, 2023
@JacksonBurns
Copy link
Contributor Author

Building RMG binary with Julia included is proving to be annoying. Currently attempting two fixes: (1) relax version requirements on RMS to try and get the environment to solve and precompile and (2) pin packages that are compatible.

Alternative to these alternatives is to ship the binary with only the basic RMG python functionality, and then add installation steps for installing RMS from the command line (this might be a really good idea). Would need to change tests to use Python instead of python-jl.

@JacksonBurns
Copy link
Contributor Author

Julia is attempting to install the rmg conda package... while building the rmg conda package. This is disastrous for obvious reasons.

@JacksonBurns JacksonBurns changed the title Build a rmgmolecule Subpackage in Place, fix rmg conda recipe Build a rmgmolecule Subpackage in Place, fix rmg conda recipe, initial SimulatorInterface Efforts Sep 21, 2023
@JacksonBurns

This comment was marked as resolved.

Sign in to view
@JacksonBurns
Copy link
Contributor Author

Have to use conda-build==3.24.0 for some reason, see conda/conda-build#4993 (comment)

@JacksonBurns
Copy link
Contributor Author

libmamba must be set as the solver inside the solving environment to force conda build to actually solve.

@JacksonBurns
Copy link
Contributor Author

squashed debugging commits and rebased onto main

@JacksonBurns JacksonBurns linked an issue Sep 28, 2023 that may be closed by this pull request
superfluous relational operator Warning when building conda environment with latest conda version #2551
Closed
@JacksonBurns
Copy link
Contributor Author

This PR should fix the linked issue when cleaning the environment file.

JacksonBurns and others added 17 commits October 27, 2023 12:32
@JacksonBurns
build an rmgmolecule package in place
d326a1b 
 - small updates to rmg-py recipe, more coming later
 - removed conda_build.yml action since it will now be done in a private repo
 - added a recipe for rmgmolecule
 - add warnings on molecule when running from rmgmolecule that some functionality is not available
@JacksonBurns
fix the RMG-Py conda build
4a0ff2d 
changes required to make the RMG-Py conda build work again. detailed description:
 - recipe now contains the appropriate requirements
 - removed macos travis-specific thing
 - added comments in build.sh for historical preservation of my failed efforts
 - other changes are the initial steps toward making the conda build work, future commits will make the build tests pass

the ci runs on a self-hosted runner in the green group offices. required a specific version of conda build and libmamba to be installed (see PR of this commit for more details), currently fails tests because rmg tries to import Julia during testing. will now need to 'bury' the Julia import and make it optional, including docs updates, etc.
@JacksonBurns
slightly reduce the required dependencies for rmgmolecule
2f48a09
@JacksonBurns
remove libmkl trick - incompatible with latest conda
16e285f
@JacksonBurns
bury import of Julia dependencies, avoid extra julia operations
d8cae99 
 - the import for the Julia dependencies is now properly buried, s.t. it will not interfere with running Arkane w/o Julia installed
 - inside main, input, and model there is a new argument requires_rms. This argument is passed to prune, add_species*, and other functions that use those functions to avoid moving things to RMS when we either (1) are not using an RMS reactor or (2) don't even have Julia dependencies installed at all
@JacksonBurns
add back the optional RMS install to the CI and the Dockerfile
3ff4ab5 
will also need to add to the docs for the installation instructions for installation using conda binaries and installation from source
@JacksonBurns
bury make new species RMS-dependent check
a02dd48
@JacksonBurns
skip registering rms listener when not needed, more rms use patches
aff2038 
missed some rms-dependent calls in previous commits
@JacksonBurns
incorrect capitalization in Arkane test
e8c776a
@JacksonBurns
another syntax error in arkane testing call
d93ed00
@JacksonBurns
override channels in CI install of RMS to ensure we get right channel
6e011ff
@JacksonBurns
can we build docs without installing RMS?
64dd407
@JacksonBurns
add nomkl back to environment
80cf457
@JacksonBurns
squash
f4b6791
@JacksonBurns
import Reactor as RMSReactor inside Model for clarity
292718f
@JacksonBurns
missed some requires_rms in enlarge()
34796db
@JacksonBurns
add requires_rms to pdep, thermo filtering, and process_new_rxns
7de8a3a
@github-actions

This comment was marked as outdated.

Sign in to view
@JacksonBurns JacksonBurns mentioned this pull request Jan 8, 2024
Open
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 2, 2024

This pull request is being automatically marked as stale because it has not received any interaction in the last 90 days. Please leave a comment if this is still a relevant pull request, otherwise it will automatically be closed in 30 days.

@github-actions github-actions bot added the stale stale issue/PR as determined by actions bot label Feb 2, 2024
@JacksonBurns
Copy link
Contributor Author

WIP

@github-actions github-actions bot removed the stale stale issue/PR as determined by actions bot label Feb 3, 2024
@JacksonBurns
Copy link
Contributor Author

Splitting this into three PRs:

  1. The NO_JULIA internal tracking variable
  2. Fix the RMG conda recipe
  3. Add the subpackage for molecule (still might drop this one)

This was referenced Mar 8, 2024
Closed
Open
This was referenced Mar 15, 2024
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hwpang hwpang hwpang left review comments

@rwest rwest rwest left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

superfluous relational operator Warning when building conda environment with latest conda version
3 participants
@JacksonBurns @rwest @hwpang