openforcefield / openff-qcsubmit

Automated tools for submitting molecules to QCFractal
https://openff-qcsubmit.readthedocs.io/en/latest/index.html
MIT License
26 stars 4 forks source link

Toolkit 0.11 support #204

Closed Yoshanuikabundi closed 1 year ago

Yoshanuikabundi commented 1 year ago

Description

Update QCSubmit to work with the OpenFF Toolkit 0.11+

Supersedes #203

Todos

Test failures (running locally on my machine):

Status

jthorton commented 1 year ago

@Yoshanuikabundi I noticed you have unpinned psi4 which is a great idea, I would recommend following this environment to get a psi4 version compatible with conda-forge. I think the important ones are

jthorton commented 1 year ago

Ahh looks like the channel priority might need to be updated as well I think

Yoshanuikabundi commented 1 year ago

Love it when an environment solves but doesn't work!

jthorton commented 1 year ago

Love it when an environment solves but doesn't work!

That's half the fun of using Psi4!

Yoshanuikabundi commented 1 year ago

OK I think the remaining errors are just waiting on the upstream updates.

jthorton commented 1 year ago

Most of the errors seem related to qcengine. This error looks like something to do with the toolkit though did something change with the properties dictionary?

Yoshanuikabundi commented 1 year ago

Yeah, __deepcopy__ was implemented for performance reasons and had a bug, the fix is merged and will be in the next toolkit release. Writing this PR is how we learned that 😂

I can temporarily pin the appropriate branches in the CI environment to make sure it all works if you like? But I figured we may as well wait for the releases.

jthorton commented 1 year ago

Yeah, deepcopy was implemented for performance reasons and had a bug, the fix is merged and will be in the next toolkit release. Writing this PR is how we learned that 😂

Ahh thats great that you have a solution already!

But I figured we may as well wait for the releases.

Yeah happy to wait, just wanted to check it was fixed good job!

codecov[bot] commented 1 year ago

Codecov Report

Merging #204 (b9d399f) into main (84ce17a) will decrease coverage by 0.29%. The diff coverage is 100.00%.

:exclamation: Current head b9d399f differs from pull request most recent head ebc3b01. Consider uploading reports for the commit ebc3b01 to get more accurate results

Additional details and impacted files
mattwthompson commented 1 year ago

I updated the environments to install against the versions one would expect to work, just in case they don't ... and they do!

I will make a toolkit release in the coming days. I'll check with MolSSI on the status of a QCEngine release.

mattwthompson commented 1 year ago

This is soft blocked by https://github.com/openforcefield/openff-units/pull/51 being in a release because the next QCEngine release will include some fixes for the new pint - though Lori's changes might be backwards-compatible

mattwthompson commented 1 year ago

Assuming the checks of ebc3b015bc729adf2c961ef7ca6ee75ccc279b18 pass, I'm going to go ahead and merge this and cut a release later today or tomorrow.

My commits were prodding along with changes to upstreams and isolating a weird bug in the upstream packaging. They mostly cancel each other out, resulting in no changes to the source code. So I consider the earlier approval from @j-wags still valid here.