quickrun CLI command #171

dwhswenson · 2022-09-22T19:51:14Z

Takes a ProtocolDAG saved as JSON, runs the DAG, and returns results.

A few things before this is ready to merge:

Decide whether the input is a serialized ProtocolDAG or a serialized Transformation. These are essentially equivalent, but there's some discussion to be had on what it means from user experience.
Decide on how to ensure serializability of results. In particular, we need to decide how to handle Path objects, and whether any non-JSON-serializables other than Paths are allowed return values.
- implemented in OpenFreeEnergy/gufe#68
Add a test
~~Merge Fix ProtocolUnit deserialization gufe#62~~ This isn't actually needed here, but will be needed in the future.
Merge PR that adds GufeTokenizable support to atom mappings (included in Using gufe protocols #142, but might split that part off of that PR)

Resolves #172.

richardjgowers

I think this will require a Transformation not just a DAG. A DAG doesn't quite have enough info to analyse itself, the results it passes out are paths to raw data, the Transformation has a reference to the Protocol, which has the gather() method to analyse that data.

richardjgowers · 2022-09-23T14:38:38Z

Re serialising, since DAGResults are consumed by the Protocol.gather(), we can just pass strings and know they're paths.

I think where this might become less preferable is when thinking about storage, the storage system isn't privy to knowing what strings are actually paths, and so serialising them as Path objects is much nicer

richardjgowers · 2022-09-23T16:29:55Z

openfecli/commands/quickrun.py

+    from gufe.protocols.protocoldag import execute, ProtocolDAG
+    dct = json.load(dagfile)
+    dag = ProtocolDAG.from_dict(dct)
+    result = execute(dag)


create a gufe.Context with a temp dir. and pass in here

richardjgowers · 2022-10-04T08:27:07Z

openfecli/commands/quickrun.py

+    """
+    import gufe
+    from gufe.protocols.protocoldag import execute
+    print("Loading file...")


this is fine for quick and dirty, but we need to figure out how we're doing logging eventually

richardjgowers

lgtm, would be nice to also test failure modes

richardjgowers · 2022-10-04T09:07:03Z

openfecli/commands/quickrun.py

+    import gufe
+    from gufe.protocols.protocoldag import execute
+    print("Loading file...")
+    dct = json.load(transformation)


also worth thinking about wrapping each step in a try/except and catching any errors? (speaking of, what are these errors...)

If there are errors that we can handle, then we could. If this is just to avoid dumping a stack trace, then let click do it for it (it does it automatically).

codecov · 2022-10-20T16:38:12Z

Codecov Report

Base: 92.12% // Head: 92.29% // Increases project coverage by +0.17% 🎉

Coverage data is based on head (ac23e42) compared to base (bd12355).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
+ Coverage   92.12%   92.29%   +0.17%     
==========================================
  Files          59       59              
  Lines        4076     4140      +64     
==========================================
+ Hits         3755     3821      +66     
+ Misses        321      319       -2

Impacted Files	Coverage Δ
openfecli/commands/quickrun.py	`100.00% <100.00%> (ø)`
openfecli/parameters/output.py	`100.00% <100.00%> (ø)`
openfecli/tests/commands/test_quickrun.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

dwhswenson · 2022-10-20T16:54:52Z

So, apparently I had already written a test for this, and I'd just forgotten about it. Assuming the MacOS tests pass, this is ready for review.

openfecli/commands/quickrun.py

IAlibay · 2022-10-21T17:07:10Z

openfecli/commands/quickrun.py

+    print("Planning the campaign...")
+    dag = trans.create()
+    print("Running the campaign...")
+    dagresult = execute(dag)


We should have a way to pass shared here? That way the files aren't temporary?

Added -d / --directory option. Can rename to --shared if that makes more sense.

Still need to test that; it didn't seem used in a manual test with the DummyProtocol, but that may be the protocol.

Thanks, --directory seems like the best option for users imho :)

richardjgowers

couple of points but looks good

openfecli/tests/commands/test_quickrun.py

openfecli/commands/quickrun.py

openfecli/tests/dev/write_transformation_json.py

openfecli/commands/quickrun.py

Still need to add tests for this. Also, it looks like DummyProtocol isn't doing any file output here ... need to figure out why

dwhswenson · 2022-10-24T19:52:13Z

@IAlibay @richardjgowers : This should be ready for another look.

I haven't tested the shared directory stuff yet, since DummyProtocol doesn't support it. However, I do pass the shared directory to execute, so if there are problems, it will be with the underlying protocols and not the CLI wrapper. One thing that isn't clear to me is whether protocols expect the shared directory to already exist, or if they create them on the fly if needed.

…quickrun-cli

IAlibay

this looks fine to me

IAlibay · 2022-10-25T10:00:09Z

One thing that isn't clear to me is whether protocols expect the shared directory to already exist, or if they create them on the fly if needed.

My understanding is that if shared == None then tempfile is used https://github.com/OpenFreeEnergy/gufe/blob/78942e6c8c0eb1cd311e83881932c0a596e2ab7e/gufe/protocols/protocoldag.py#L234-L236

dwhswenson · 2022-10-25T13:44:52Z

My understanding is that if shared == None then tempfile is used

I meant that if a directory is provided, then someone will have to do the equivalent of mkdir -p so that the directories exist when files are written to them. My gut is that this belongs at the level of ProtocolUnit, although I'm not sure whether we can provide something to automatically handle that or whether the responsibility falls on the author of each unit. Other options might be to require that the user-specified directory exists, or to make (potentially empty) directories at each level (e.g., create the shared directory as part of the execute method, then responsibility for any subdirectories may come at the ProtocolUnit level). In any case, creating that directory should not be the responsibility of the CLI.

IAlibay · 2022-10-25T13:46:29Z

It creates the directory. I've not had to create them for things to work for the example notebooks.

quickrun CLI command

232aa73

richardjgowers reviewed Sep 23, 2022

View reviewed changes

dwhswenson added 5 commits September 29, 2022 13:28

switch to taking transformation as input

b4b73ca

Fix name error

9b2b231

Working version of the quickrun, with results!

2104d86

improve command docs

6450b5e

Add test for quickrun

a2e987a

richardjgowers reviewed Oct 4, 2022

View reviewed changes

Merge branch 'main' into quickrun-cli

c7b04d0

move write_transformation_json to avoid coverage

c68c8f0

dwhswenson changed the title ~~[WIP] quickrun CLI command~~ quickrun CLI command Oct 20, 2022

dwhswenson requested review from richardjgowers and IAlibay October 20, 2022 16:54

IAlibay reviewed Oct 21, 2022

View reviewed changes

richardjgowers reviewed Oct 21, 2022

View reviewed changes

openfecli/tests/commands/test_quickrun.py Outdated Show resolved Hide resolved

openfecli/commands/quickrun.py Show resolved Hide resolved

openfecli/tests/dev/write_transformation_json.py Outdated Show resolved Hide resolved

richardjgowers reviewed Oct 21, 2022

View reviewed changes

openfecli/commands/quickrun.py Outdated Show resolved Hide resolved

dwhswenson added 3 commits October 24, 2022 10:27

Update based on review comments

6447b91

Still need to add tests for this. Also, it looks like DummyProtocol isn't doing any file output here ... need to figure out why

Error if JSON output file already exists

f060782

improve JSON output file contents

0264737

dwhswenson added 3 commits October 24, 2022 15:11

Merge branch 'main' into quickrun-cli

c65f722

add JSON_HANDLER stuff to quickrun docstring

2e38cf9

Merge branch 'quickrun-cli' of github.com:OpenFreeEnergy/openfe into …

e1ffdb3

…quickrun-cli

IAlibay approved these changes Oct 25, 2022

View reviewed changes

use JSON_HANDLER.decoder on loading

ac23e42

richardjgowers approved these changes Oct 26, 2022

View reviewed changes

richardjgowers merged commit 159ebf3 into main Oct 26, 2022

richardjgowers deleted the quickrun-cli branch October 26, 2022 13:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

quickrun CLI command #171

quickrun CLI command #171

dwhswenson commented Sep 22, 2022 •

edited

Loading

richardjgowers left a comment

richardjgowers commented Sep 23, 2022

richardjgowers Sep 23, 2022

richardjgowers Oct 4, 2022

richardjgowers left a comment

richardjgowers Oct 4, 2022

dwhswenson Oct 20, 2022

codecov bot commented Oct 20, 2022 •

edited

Loading

dwhswenson commented Oct 20, 2022

IAlibay Oct 21, 2022

dwhswenson Oct 24, 2022

IAlibay Oct 24, 2022

richardjgowers left a comment

dwhswenson commented Oct 24, 2022 •

edited

Loading

IAlibay left a comment

IAlibay commented Oct 25, 2022

dwhswenson commented Oct 25, 2022 •

edited

Loading

IAlibay commented Oct 25, 2022

quickrun CLI command #171

quickrun CLI command #171

Conversation

dwhswenson commented Sep 22, 2022 • edited Loading

richardjgowers left a comment

Choose a reason for hiding this comment

richardjgowers commented Sep 23, 2022

richardjgowers Sep 23, 2022

Choose a reason for hiding this comment

richardjgowers Oct 4, 2022

Choose a reason for hiding this comment

richardjgowers left a comment

Choose a reason for hiding this comment

richardjgowers Oct 4, 2022

Choose a reason for hiding this comment

dwhswenson Oct 20, 2022

Choose a reason for hiding this comment

codecov bot commented Oct 20, 2022 • edited Loading

Codecov Report

dwhswenson commented Oct 20, 2022

IAlibay Oct 21, 2022

Choose a reason for hiding this comment

dwhswenson Oct 24, 2022

Choose a reason for hiding this comment

IAlibay Oct 24, 2022

Choose a reason for hiding this comment

richardjgowers left a comment

Choose a reason for hiding this comment

dwhswenson commented Oct 24, 2022 • edited Loading

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay commented Oct 25, 2022

dwhswenson commented Oct 25, 2022 • edited Loading

IAlibay commented Oct 25, 2022

dwhswenson commented Sep 22, 2022 •

edited

Loading

codecov bot commented Oct 20, 2022 •

edited

Loading

dwhswenson commented Oct 24, 2022 •

edited

Loading

dwhswenson commented Oct 25, 2022 •

edited

Loading