Enable extending NonEquilibriumCyclingProtocols #44
Conversation
* Added new (de)compression + (de)serialize functions in feflow.utils.data * Include compressed System, State, and Integrator as a result for the main simulation unit. * SetupUnits now take `extends_data`, which when populated, spoof the running of the `SetupUnit._execute` method. It instead immediately returns results with values consistent to the end state of the extended SimulationUnit. * Added new test `test_pdr_extend` to test the extension functionality.
ianmkenney
changed the title
ianmkenney
force-pushed
the
feature-noneq_cycling_extends
branch
from
2894417
to
3991187
Compare
for more information, see https://pre-commit.ci
|
One limitation of Is this the case for the outputs added in this PR? |
|
These are not json serializable. I can add another step in (de)serialization for converting to and from base64. |
GufeTokenizables must be JSON serializable, so including bytes is not an option. Instead, we take the compressed bytes and encode them into a Base64 string.
for more information, see https://pre-commit.ci
* When `mapping` is `None`, use the mapping provided by the ProtocolDAGResult
ianmkenney
force-pushed
the
feature-noneq_cycling_extends
branch
from
b2aa129
to
8c69b31
Compare
for more information, see https://pre-commit.ci
|
@ijpulidos any idea why my tests fail on license keys? |
|
@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 |
|
to kick off ci (you need write access to the repo for this to work) |
|
This is currently blocked by #38. |
|
@ianmkenney @dotsdl Is this ready to be reviewed? If so, can you mark it as such? Thanks! |
ianmkenney
changed the title
|
@ijpulidos I think an initial review works, not sure if you want to wait for gufe/openfe v1.0.0 before merging though. |
| # TODO: are there instances where this is too strict? | ||
| if setup.inputs["settings"] != self.settings: | ||
| raise ValueError( | ||
| "protocol settings are not consistent with those present in the SetupUnit of the 'extends' ProtocolDAGResult." | ||
| ) |
There was a problem hiding this comment.
I think this is a good check to have, and is not too strict at all. The philosophy behind a Protocol with a given Settings object is that those Settings are immutable once given to the Protocol object, and so calling Protocol.create(..., extends=<protocol_dag_result>) should feature identical settings between the calling Protocol and the ProtocolDAGResult we wish to extend from.
There was a problem hiding this comment.
Just chiming in here - there are a few cases where this is going to be "not ideal" (mostly QoL but possibly could be frustrating).
So one example here could be a case where you extend but you switch compute platform - the results should be the same, but the settings will be different. Similarly the trajectory write frequency (you can imagine a case where someone decides that it's just not that necessary to have the trajectory after extending), etc...
It's of course not a blocker right now, but a "smarter" equality might be necessary (e.g. OpenFreeEnergy/gufe#329).
There was a problem hiding this comment.
I see your point, and think it makes sense to have a thermodynamically_equals method or function for Protocol settings objects for this purpose.
Systems like alchemiscale may limit the way users can do extends in the sense that Tasks may only extend other Tasks from the same Transformation, but the Protocol system on its own isn't as strict, since the concept of a Transformation sits outside of it.
|
@ijpulidos will make another review now that #38 is in! Thanks @ijpulidos! |
|
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. |
|
From today's iteration meeting - let's put this on the agenda for the next protocol devs call. |
|
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. |
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.