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.

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

Remove chemprop-dependent Code #2450

Conversation

JacksonBurns
Copy link
Contributor

Motivation or Problem

As discussed in #2445 our build of chemprop is very old and does not support Python 3.11, which will prevent us from doing so as well.

Description of Changes

Right now the only change is to switch the channel in environment.yml from rmg to conda-forge for chemprop. This will likely break other code in RMG-Py and require additional changes afterward.

Testing

I have done a --dry-run of resolving this environment file so that at least works, but there will be other things to fix as revealed by the CI.

Notes

This PR will later need to be followed by an upgrade PR (as part of #2445) to move to chemprop v2, which will support Python 3.11

@JacksonBurns JacksonBurns self-assigned this May 31, 2023
@JacksonBurns
Copy link
Contributor Author

This commit might be helpful in fixing this first failed CI run.

@JacksonBurns
Copy link
Contributor Author

JacksonBurns commented Jun 7, 2023

Note from the RMG developers meeting - the chemprop model included in RMG-Py right now may not survive the upgrade to Python 3.11. The timeline of the upgrade to Python 3.11 and the difficulty of training the model and then fixing the API are challenging. It is likely that we will need to add the ML estimators back after the transition to Python 3.11

@JacksonBurns JacksonBurns changed the base branch from main to py311 June 8, 2023 12:24
@JacksonBurns JacksonBurns added the Python 3.11 Transition PRs and Issues related to transitioning from Python 3.7 to 3.11 label Jun 8, 2023
@JacksonBurns JacksonBurns changed the title Use conda-forge build of chemprop instead of rmg Remove chemprop-dependent Code Jun 8, 2023
@JacksonBurns
Copy link
Contributor Author

For the transition to Python 3.11, features requiring chemprop will be removed. They can be added back in later, but that will require:

  • retraining the ML models
  • re-writing the ML estimator to use a more updated version of chemprop
  • validating that the new models are better than the current capabilities of RMG

There is also discussion of build a more flexible interface to ML-based estimators, such that the estimator itself would not be distributed with RMG but would just need to inherit from some ML Estimator class in RMG that defines a get_enthalpy-esque set of methods.

@rwest rwest marked this pull request as draft June 9, 2023 20:13
@github-actions
Copy link

github-actions bot commented Sep 8, 2023

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 Sep 8, 2023
@github-actions github-actions bot added the abandoned abandoned issue/PR as determined by actions bot label Oct 9, 2023
@github-actions github-actions bot closed this Oct 9, 2023
@JacksonBurns
Copy link
Contributor Author

chemprop version 2 is upcoming and will add support for Python 3.11 and 3.12 but will face the same issues as above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abandoned abandoned issue/PR as determined by actions bot Python 3.11 Transition PRs and Issues related to transitioning from Python 3.7 to 3.11 stale stale issue/PR as determined by actions bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant