8000 Changed simtk.openmm to openmm by jlmaccal · Pull Request #1173 · ParmEd/ParmEd · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Changed simtk.openmm to openmm #1173

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
Closed
wants to merge 1 commit into from
Closed

Changed simtk.openmm to openmm #1173

wants to merge 1 commit into from

Conversation

jlmaccal
Copy link
Contributor

New versions of openmm have switched to the openmm namespace, rather than simtk.openmm. Imports of simtk.openmm produce a warning a runtime.

This PR should remove all references to simtk.

@justinGilmer
Copy link
Contributor

Those changes seem reasonable to me. Is there a specific omm version where this change occurs? Could modify the requirements to specify the versions that are compatible with this change as well.

@mattwthompson
Copy link
Contributor

This is probably a majorly breaking change - the PR making that change was merged months ago but it has not made its way into a release. The tests using OpenMM are likely all skipped via the @needs_openmm decorator, which probably also toggles off all OpenMM-related functionality.

ParmEd/parmed/openmm/topsystem.py

Lines 21 to 27 in 5f75d10

from ..utils.decorators import needs_openmm
__all__ = ['load_topology']
@needs_openmm
def load_topology(topology, system=None, xyz=None, box=None, condense_atom_types=True):

@jlmaccal
Copy link
Contributor Author
jlmaccal commented May 13, 2021 •
edited
Loading

I’m not sure about the openmm version where these changes occur, but the requirement would need to be updated. It’s also fine with me if you don’t want to merge this. I made this change in our code base, but was still getting a warning about simtk, until I realized parmed (which we use) is also importing simtk.openmm.

It might also be possible to make this change in a more backwards compatible way. I think the goal would be that import parmed doesn’t cause the warning about simtk. This would require something like:

try:
    import openmm as mm
except ImportError:
    import simtk.openmm as mm

This change would only need to be made in the main code path, not in all the tests.

Does this seem like a better approach?

@mattwthompson
Copy link
Contributor

I don't have the final say but that does seem like a better approach to me - since the change (whenever it goes into a release, I don't know when this is going to happen) is limited to the import path and not the rest of the API, people are likely going to be using a mix of older and newer versions for a while.

@swails
Copy link
Contributor
swails commented May 13, 2021

I had at one point tried to centralize the OpenMM imports and use some tricks to fall back on simtk if openmm wasn't available. It wasn't worth the hassle for an optional dependency that is completely free.

I was holding off on making this change until OpenMM cut a release with this switch (which it hasn't yet).

The unit tests do currently exercise OpenMM, but the reason they aren't all failing now is because, as @mattwthompson pointed out, the testing framework identifies OpenMM as not being available and just skips them (which is why the test coverage of this PR would be substantially lower than what the whole test suite currently achieves).

I'm going to wait on merging this until after OpenMM has a release so I have the freedom to cut a new ParmEd release before OpenMM does theirs.

@swails
Copy link
Contributor
swails commented May 13, 2021

An alternative is to try and convince @peastman to create a transitional stub library that is pip/conda-installable that literally just imports stuff from the simtk namespace into the package (named openmm) as a way of making older versions forwards-compatible through the name change.

OTOH, hoisting deprecation warnings for a transitional period doesn't seem that horrible to me (plenty of popular packages I work with do it).

@peastman
Copy link
Contributor

We haven't scheduled the 7.6 release yet, which is where the new namespace will be introduced. Until it's released, I would recommend sticking with simtk. The worst that happens is that if someone has installed a development version of OpenMM, they get a deprecation warning.

@swails
Copy link
Contributor
swails commented May 13, 2021

We haven't scheduled the 7.6 release yet, which is where the new namespace will be introduced.

This strikes me as a very strong justification for cutting an 8.0, for my 2c.

@mattwthompson mattwthompson mentioned this pull request May 21, 2021
Closed
This was referenced May 24, 2021
Closed
Merged
@mattwthompson
Copy link
Contributor

The OpenMM feedstock maintainers have released a 7.6 beta which can now be used to test the new namespace:

mamba install -c conda-forge/label/openmm_rc -c conda-forge "openmm=7.6beta"

@peastman
Copy link
Contributor

I think this can now be merged?

@peastman
Copy link
Contributor
peastman commented Sep 8, 2021

Is there anything still blocking this PR? It needs to be merged before I can fix the AMOEBA code.

@peastman
Copy link
Contributor
peastman commented Oct 1, 2021

Just a reminded that I'm still waiting on this!

@jchodera
Copy link
Contributor
jchodera commented Oct 1, 2021

@swails: Are there other developers with commit access that could help, or would you be willing to give someone from the OpenMM/OpenFF community commit access to help maintain ParmEd?

@swails
Copy link
Contributor
swails commented Oct 2, 2021

There's another PR that fixes ParmEd with OpenMM 7.6 (#1199) that still maintains backwards compatibility with older versions (for now).

Is that sufficient for the amoeba work to begin? It pins omm 7.6 in the test environment now.

@swails
Copy link
Contributor
swails commented Oct 2, 2021

I don't think anyone else has commit access, but I'm open to granting it to someone that's interested.

@peastman
Copy link
Contributor
peastman commented Oct 2, 2021

That one would work too. Thanks!

@swails swails mentioned this pull request Oct 2, 2021
Merged
@swails
Copy link
Contributor
swails commented Oct 3, 2021

Resolved in #1199

@swails swails closed this Oct 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jlmaccal @justinGilmer @mattwthompson @swails @peastman @jchodera
0