Skip to content

Fix so SimStore tests pass without simtk.unit#957

Merged
dwhswenson merged 1 commit intoopenpathsampling:masterfrom
dwhswenson:fix-simstore-req-simtk
Dec 28, 2020
Merged

Fix so SimStore tests pass without simtk.unit#957
dwhswenson merged 1 commit intoopenpathsampling:masterfrom
dwhswenson:fix-simstore-req-simtk

Conversation

@dwhswenson
Copy link
Member

Without simtk.unit installed, SimStore wasn't working. We only test SimStore with full integrations (similar problem to #954).

This also adds an extras for SimStore, so (in future releases) pip install openpathsampling[simstore] will install the extra requirements for SimStore as well.

@dwhswenson dwhswenson added misc PR bugfix PRs fixing bugs and removed misc PR bugfix PRs fixing bugs labels Dec 27, 2020
@dwhswenson
Copy link
Member Author

This is ready for review and comment. I will leave it open for at least 24 hours, merging no earlier than Mon 28 Dec 14:00 GMT (15:00 local).

NB: This particular issue is blocking the next release of the OPS CLI (which will include SimStore support!) and so I'll probably go ahead and release a 1.4.1 shortly after this is in.

@sroet
Copy link
Member

sroet commented Dec 28, 2020

LGTM, 1 quick question:
I don't see where pip install openpathsampling[simstore] would install simtk? (I only see sqlalchemy and dill added to the cfg)

Is simtk only a testing requirement for SimStore or did you forget to add it to the cfg? (just making sure it was intentional, I am not too familiar with the internals of simstore anymore)

@dwhswenson
Copy link
Member Author

Is simtk only a testing requirement for SimStore or did you forget to add it to the cfg? (just making sure it was intentional, I am not too familiar with the internals of simstore anymore)

Sorry, the PR message wasn't clear. Two separate issues here:

  1. Using SimStore should not require that simtk.unit is present in the environment (you should be able to use and test the new storage without OpenMM installed). But you couldn't use experimental.Storage without importing simtk.unit -- that's the main thing this PR fixes.

  2. As a separate issue, currently I say "if you want to use SimStore, you need to install a couple extra packages..." -- adding the simstore extra in setup.cfg makes that process a little more elegant (at least, in some use cases).

@sroet
Copy link
Member

sroet commented Dec 28, 2020

Sorry, the PR message wasn't clear. Two separate issues here:

Using SimStore should not require that simtk.unit is present in the environment (you should be able to use and test the new storage without OpenMM installed). But you couldn't use experimental.Storage without importing simtk.unit -- that's the main thing this PR fixes.

As a separate issue, currently I say "if you want to use SimStore, you need to install a couple extra packages..." -- adding the simstore extra in setup.cfg makes that process a little more elegant (at least, in some use cases).

Ah, thanks for the clarification! All looks good to me then :)

@dwhswenson dwhswenson merged commit f809081 into openpathsampling:master Dec 28, 2020
@dwhswenson dwhswenson deleted the fix-simstore-req-simtk branch December 28, 2020 14:37
@dwhswenson dwhswenson mentioned this pull request Dec 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants