Conversation
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
|
I don't know what's up with the tyk2 CLI test, but locally the restart is working! |
atravitz
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1848 +/- ##
==========================================
- Coverage 94.96% 91.90% -3.06%
==========================================
Files 206 206
Lines 18181 18256 +75
==========================================
- Hits 17265 16778 -487
- Misses 916 1478 +562
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
question: do we want this to be the default behavior, or require |
atravitz
force-pushed
the
quickrun_resume
branch
from
8de3040 to
d735c7f
Compare
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Just the one question but otherwise lgtm. I'll retest on a real FEP job today too.
atravitz
force-pushed
the
quickrun_resume
branch
from
588b124 to
fe746c8
Compare
| dag = trans.create() | ||
| # Attempt to either deserialize or freshly create DAG | ||
| cache_basedir = work_dir / "quickrun_cache" | ||
| trans_DAG_json = cache_basedir / f"{trans.key}-ProtocolDAG.json" |
There was a problem hiding this comment.
right now the directory structure (with https://github.com/OpenFreeEnergy/gufe/pull/764/files) looks like:
quickrun_cache/
├── ProtocolDAG-8090c89c950e976b33829a15e662ed89-results_cache/
└── Transformation-dbea03c534737749bb413e01a382f3af-protocolDAG.json
is there a better way to indicate that Transformation-dbea03c534737749bb413e01a382f3af-ProtocolDAG.json is the ProtocolDAG corresponding to Transformation-dbea03c534737749bb413e01a382f3af?
There was a problem hiding this comment.
You could move the dag creation up a little bit, and just use the dag key as part of the name?
I.e. of you define trans_DAG_json after you define dag, it could be f"ProtocolDAG-{dag.key}.json" instead.
There was a problem hiding this comment.
in offline conversation, @IAlibay and I agree that moving dag creation up will not work, because each time .create() is called, a new unique ProtocolDAG is created.
We also realize that, as of right now, the cache is only unique based on the transformation key and the -d argument. This means that if a user called the following two commands in succession:
openfe quickrun transformation.json -o result1.json -d results/
openfe quickrun transformation.json -o result2.json -d results/
openfe will treat these as a re-execution and raise an error telling the user that they should pass in --resume or delete the file and restart.
There are two possible solutions to this:
- Enforce user behavior to have separate
-dvalues for each repeat (essentially what is implemented now) - Build the hash based on the uniqueness of all 3 1. transformation key 2.
-o, and 3.-d. Since -o can be any arbitrary filepath we may want to hash-oand store it as:
[-d arg]/quickrun_cache/dagcache-[hash(transformation.key, -o arg)].
|
Testing locally with multiple interrupts / resumes is working as expected. |
|
No API break detected ✅ |
4 participants
Minimal changes to quickrun in order to allow resuming by serializing ProtocolDAGs.
Related to & requires OpenFreeEnergy/gufe#738
Checklist
newsentry, or the changes are not user-facing.pre-commit.ci autofix.Manual Tests: these are slow so don't need to be run every commit, only before merging and when relevant changes are made (generally at reviewer-discretion).
Developers certificate of origin