search

OpenFreeEnergy / feflow

Recipes and protocols for molecular free energy calculations using the openmmtools/perses and Open Free Energy toolkits
MIT License
13 stars 1 forks source link

Enable extending NonEquilibriumCyclingProtocols #44

Open ianmkenney opened 7 months ago

ianmkenney commented 7 months ago

This PR introduces the ability to extend a NonEquilibriumCycling protocol by including serialized and compressed OpenMM State, System, and Integrator in the main simulation units. When instantiating new NonEquilbriumProtocols which extend a previous result, these objects are used to bypass the normal function of the SetupUnit. Closes #2.

dotsdl commented 7 months ago

One limitation of GufeTokenizables, including ProtocolDAGResults: they must be JSON-serializable, and therefore can't have binary data. So any outputs of a ProtocolUnit._execute method must also be data types that are JSON-serializable.

Is this the case for the outputs added in this PR?

ianmkenney commented 7 months ago

These are not json serializable. I can add another step in (de)serialization for converting to and from base64.

ianmkenney commented 7 months ago

@ijpulidos any idea why my tests fail on license keys?

ijpulidos commented 7 months ago

@ianmkenney My guess is that the secrets fail since it's coming from a fork? Let me dig around and see if there's a way to make this work.

One way would be making a separate PR in the repo, but I'll see if there's another way.

mikemhenry commented 7 months ago

@ianmkenney My guess is that the secrets fail since it's coming from a fork? Let me dig around and see if there's a way to make this work.

One way would be making a separate PR in the repo, but I'll see if there's another way.

That is exactly what is going on, the quick fix is for someone who has write access, to push this branch to the choderalab/feflow remote, that will trigger the tests to run with the secrets and since the commit hash is the same, github will report the test results here

mikemhenry commented 7 months ago

to kick off ci (you need write access to the repo for this to work)

$ gh pr checkout 44
$ git push origin feature-noneq_cycling_extends
dotsdl commented 7 months ago

This is currently blocked by #38.

ijpulidos commented 7 months ago

@ianmkenney @dotsdl Is this ready to be reviewed? If so, can you mark it as such? Thanks!

ianmkenney commented 7 months ago

@ijpulidos I think an initial review works, not sure if you want to wait for gufe/openfe v1.0.0 before merging though.

dotsdl commented 4 months ago

@ijpulidos will make another review now that #38 is in! Thanks @ijpulidos!

ijpulidos commented 1 month ago

I was looking over these changes again, and I think we should take a closer look at how we’re supporting extensions. While this works, I’m not sure it will generalize well to other protocols we plan to host here. Probably worth a bit more discussion to make sure we are not limiting ourselves down the line.

IAlibay commented 1 month ago

From today's iteration meeting - let's put this on the agenda for the next protocol devs call.

ijpulidos commented 3 weeks ago

We reached a consensus on that extending protocols depend a lot on what the protocols do themselves, therefore it is expected that create method of the protocol to deal with it in a specific manner.

We should be good to solve the merge conflicts and merge this.