[WIP] Use `StorageManager` in `execute_DAG` #258

dwhswenson · 2023-12-01T19:10:01Z

This is a subset of the work in #234, split off to facilitate review. This PR will merge into #186 (making it easier to see the additions specific to the PR, but meaning that this should be merged before that). It also includes the work in #257 (one of the old tests fails without that).

This PR integrates StorageManager into execute_DAG. Note that this still isn't a complete replacement of the storage system: a follow-up PR will handle serialization of StagingPaths.

TODO:

Check that we can exactly reproduce the old behavior with the ReproduceOldBehaviorStorageManager; I think some paths aren't quite right.
Add some tests that the directories are as expected in the ExecutionStorageDemoTest subclasses
Revisit when/where dag_label is given (also needed in #186)
Switch it so the context managers return Context (executor shouldn't need to assemble that)

…o shared-object-v2

…ect-v2

Makes much more sense as _safe_to_delete_holding

right now, we seem to not be getting anything; see if this is because someone has added a handler on the root logger or because we can't caplog from a parent logger (may need specific __name__)

…o shared-object-v2

also update for changes in other PR

This will make it easier to test that we create the same old directories

richardjgowers · 2023-12-12T17:43:26Z

gufe/protocols/protocoldag.py

+from ..storage.storagemanager import StorageManager
+from ..storage.externalresource.filestorage import FileStorage
+from ..storage.externalresource.base import ExternalStorage


can we fix the namespace of storage so these are all available at the same level? from ..storage import StorageManager, FileStorage, etc

richardjgowers · 2023-12-12T17:44:16Z

gufe/protocols/protocoldag.py

+        )
+        return storage_manager
+
+
 def execute_DAG(protocoldag: ProtocolDAG, *,


are you keeping this for comparison for the PR? I don't think we need to keep the old behaviour around

I think we told Joe that the API was stable enough that he could use it in workflows. So yeah, I spent a lot of time ensuring that there would be a code path that doesn't break the current API.

richardjgowers · 2023-12-12T17:45:00Z

gufe/protocols/protocoldag.py

+
+def new_execute_DAG(  # TODO: this is a terrible name
+    protocoldag: ProtocolDAG,
+    dag_label: str,


why is a DAG label now required?

and why can't the label just be taken off the input DAG?

Breaks current behavior, where the equivalent of the DAG label is given by the user as the -o from quickrun.

Make a function that returns a pathlib.Path be the main thing, instead of the property that returns a string. Almost all usage of `.fspath` was to then wrap it in a pathlib.Path. This is more convenient for users.

dwhswenson

Commenting on a couple of breaking changes these PRs introduce that people should be aware of.

dwhswenson · 2023-12-21T14:51:07Z

gufe/tests/test_protocoldag.py

+        with open(ctx.shared / f'unit_{my_id}_shared.txt', 'w') as out:
            out.write(f'unit {my_id} existed!\n')
-        with open(os.path.join(ctx.scratch, f'unit_{my_id}_scratch.txt'), 'w') as out:
+        with open(ctx.scratch / f'unit_{my_id}_scratch.txt', 'w') as out:


Breaking change: os.path functionality should not be used. Switch to the pathlib-like semantics of using /.

I don't see a way to have this not be breaking, since the PathLike created here needs to be one of ours. Options that involve keeping os.path would be (1) require something other than the system open, or (2) require manual conversion of any user-created path to one of our objects. We do support option 2 for cases where legacy code can't be converted, but the pathlib-like syntax is more readable and allows us to silently support any protocol that already uses that syntax.

dwhswenson · 2023-12-21T15:00:53Z

gufe/tests/test_protocol.py

+        assert len(list(shared.iterdir())) == 0
+        # assert len(list(shared.iterdir())) == 5


(Extremely minor) breaking change: Previously, this left behind 5 empty shared directories. In the current implementation, empty directories are, by default, automatically deleted, so we have no directories within the shared space. Had these directories contained any files, you would still find all 5.

There's a parameter to keep empty directories, but the effect will also be to keep empty subdirectories, which I didn't think was the desired default behavior. My assumption is that it will be relatively rare to keep shared anyway, because there should be something in settings to determine whether any given file is scratch/shared/permanent, and anything intended for long-term storage will be put in permanent.

Adds a user story test where a type supported by a custom JSON codec is added to the result dict, and that codec isn't registered as of serialization object creation.

This is to avoid likely footguns related to using os.path.join. Includes test of error on os.path.join.

richardjgowers · 2024-01-04T18:34:56Z

gufe/protocols/protocoldag.py

+                with dag_ctx.running_unit(
+                    dag_label, unit.key, attempt=attempt
+                ) as context:
+                    _logger.info("Starting unit {label}")


missing an f-string thingy here

richardjgowers · 2024-01-15T14:34:13Z

gufe/protocols/protocolunit.py

    scratch: PathLike
-    shared: PathLike
+    shared: StagingRegistry
+    permanent: StagingRegistry


I think these are meant to be stagingpaths not registries?

dwhswenson and others added 30 commits April 26, 2023 13:05

PathLike wrapper/cache for ExternalStorage

ffdb901

Merge branch 'main' into shared-object-v2

4ad7fb1

mypy

eb19e0f

Merge branch 'shared-object-v2' of github.com:OpenFreeEnergy/gufe int…

8e6a78c

…o shared-object-v2

docstrings

3be13c0

Merge branch 'main' of github.com:OpenFreeEnergy/gufe into shared-obj…

dda02e2

…ect-v2

Add StorageManager code

472151e

rename to Stagine

b692c2f

Add tests for _delete_empty_dirs

7432ff4

Storage docs

319d0d0

outline of storage manager tests

5650609

minor improvements on staging directory

ddbbd19

first storage lifecycle test works

c5ce48a

Merge branch 'main' of github.com:OpenFreeEnergy/gufe into shared-obj…

236b263

…ect-v2

cleanup mypy

b805aac

change to unit taking in the label

ed5e83c

lots of updates; switched to harness for tests

6181039

Big reorg for shared overlapping staging

1880f73

remove _storage_path_conflict

1e4ca2c

Makes much more sense as _safe_to_delete_holding

docs & types

a6d26f3

docs, types, logging

aabbc33

finish TestHoldingOverlapsPermanentStorageManager

b4d73b3

mypy

7af006e

test_repr

58a58bc

renaming around DAGContextManager

8e429f5

holding => staging

b70df48

finish docs (I think?)

08e3ac2

remove completed TODO

ca7871b

start to testing edge case logging

2aa0616

right now, we seem to not be getting anything; see if this is because someone has added a handler on the root logger or because we can't caplog from a parent logger (may need specific __name__)

Update stagingdirectory.py

7c03dcd

dwhswenson added 7 commits December 11, 2023 12:46

Add logging to not clean up registered directory

ce12326

Merge branch 'shared-object-v2' of github.com:OpenFreeEnergy/gufe int…

9afeb2f

…o shared-object-v2

Merge branch 'shared-object-v2' into staging-execute-dag

b3fea47

remove unneeded comment

2420f6c

yield Context instead of tuple

fe64baa

also update for changes in other PR

ReproduceOldBehaviorStorageManager.from_old_args

288e6cc

This will make it easier to test that we create the same old directories

tests for ReproduceOldBehaviorStorageManager.from_old_args

bc1f36f

richardjgowers reviewed Dec 12, 2023

View reviewed changes

dwhswenson added 11 commits December 13, 2023 16:28

StagingPath.fspath => StagingPath.as_path()

aaa2aab

Make a function that returns a pathlib.Path be the main thing, instead of the property that returns a string. Almost all usage of `.fspath` was to then wrap it in a pathlib.Path. This is more convenient for users.

pep8

cf60d1b

Merge branch 'shared-object-v2' into staging-execute-dag

d0af5cf

Add tests for nested files in shared

4861b3e

tests for empty directories

c0a37b3

tests for the directories in the overlap example

3d7ca7b

Merge branch 'staging-execute-dag' into staging-serialization

529b8a6

update for changes in preceding PRs

c412611

First version of the change storage backend story

bf49c34

docstrings; sketch out yet ANOTHER user story

10a40eb

minor docs update

4b2510c

dwhswenson commented Dec 21, 2023

View reviewed changes

dwhswenson added 5 commits December 21, 2023 17:44

test new codec added in staging serialization

3390bb0

Adds a user story test where a type supported by a custom JSON codec is added to the result dict, and that codec isn't registered as of serialization object creation.

remove fspath from StagingRegistry

da9955e

This is to avoid likely footguns related to using os.path.join. Includes test of error on os.path.join.

Merge branch 'shared-object-v2' into staging-execute-dag

b72e6f9

Merge branch 'staging-execute-dag' into staging-serialization

409ec06

Attach StorageSerialization to StorageManager

b22445b

richardjgowers reviewed Jan 4, 2024

View reviewed changes

richardjgowers reviewed Jan 15, 2024

View reviewed changes

update for some review comments

8534d71

dwhswenson changed the base branch from shared-object-v2 to main January 22, 2024 16:52

Merge branch 'main' into staging-execute-dag

01db5b1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Use `StorageManager` in `execute_DAG` #258

[WIP] Use `StorageManager` in `execute_DAG` #258

dwhswenson commented Dec 1, 2023 •

edited

Loading

richardjgowers Dec 12, 2023

richardjgowers Dec 12, 2023

dwhswenson Dec 12, 2023

richardjgowers Dec 12, 2023

dwhswenson Dec 12, 2023

dwhswenson left a comment

dwhswenson Dec 21, 2023

dwhswenson Dec 21, 2023

richardjgowers Jan 4, 2024

richardjgowers Jan 15, 2024

		assert len(list(shared.iterdir())) == 0
		# assert len(list(shared.iterdir())) == 5

[WIP] Use StorageManager in execute_DAG #258

Are you sure you want to change the base?

[WIP] Use StorageManager in execute_DAG #258

Conversation

dwhswenson commented Dec 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwhswenson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[WIP] Use `StorageManager` in `execute_DAG` #258

[WIP] Use `StorageManager` in `execute_DAG` #258

dwhswenson commented Dec 1, 2023 •

edited

Loading