-
Notifications
You must be signed in to change notification settings - Fork 195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add pluggable DependencyResolvers #3111
Changes from 24 commits
fd9f668
020a37f
1889bf9
3241189
9fabe49
e382e39
c0827be
99ec556
01f51b2
d979b08
fc45588
7a64b9c
b7fbd43
48bfb58
282f609
77caf05
93e4b9a
7ac5b72
c1d7206
3f54c17
0234d45
f781148
ffc6b4f
59628e0
b703b91
961bb6d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current tests in this PR go through the whole Parsl machinery -- good! But an ancillary set of unit tests that verify strictly this class in isolation would be a good value-add. That is, this is a decently isolated class that doesn't depend on Parsl, so it's functionality could be verified independently of the rest of the infrastructure. (Note that we've only recently added the |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
from concurrent.futures import Future | ||
from dataclasses import dataclass | ||
from functools import singledispatch | ||
from typing import Callable | ||
|
||
|
||
@dataclass | ||
class DependencyResolver: | ||
"""A DependencyResolver describes how app dependencies can be resolved. | ||
It is specified as two functions: `traverse_to_gather` which turns an | ||
app parameter into a list of futures which must be waited for before | ||
the task can be executed (for example, in the case of | ||
`DEEP_DEPENDENCY_RESOLVER` this traverses structures such as lists to | ||
find every contained ``Future``), and `traverse_to_unwrap` which turns an | ||
app parameter into it's value to be passed to the app on execution | ||
(for example in the case of `DEEP_DEPENDENCY_RESOLVER` this replaces a | ||
benclifford marked this conversation as resolved.
Show resolved
Hide resolved
|
||
list containing futures with a new list containing the values of those | ||
resolved futures). | ||
|
||
By default, Parsl will use `SHALLOW_DEPENDENCY_RESOLVER` which only | ||
resolves Futures passed directly as arguments. | ||
""" | ||
traverse_to_gather: Callable | ||
traverse_to_unwrap: Callable | ||
|
||
|
||
@singledispatch | ||
def shallow_traverse_to_gather(o): | ||
# objects in general do not expose futures that we can see | ||
return [] | ||
|
||
|
||
@singledispatch | ||
def shallow_traverse_to_unwrap(o): | ||
# objects in general unwrap to themselves | ||
return o | ||
|
||
|
||
@shallow_traverse_to_gather.register | ||
def _(fut: Future): | ||
return [fut] | ||
|
||
|
||
@shallow_traverse_to_unwrap.register | ||
@singledispatch | ||
def _(fut: Future): | ||
return fut.result() | ||
|
||
|
||
@singledispatch | ||
def deep_traverse_to_gather(o): | ||
# objects in general do not expose futures that we can see | ||
return [] | ||
|
||
|
||
@singledispatch | ||
def deep_traverse_to_unwrap(o): | ||
# objects in general unwrap to themselves | ||
return o | ||
|
||
|
||
@deep_traverse_to_gather.register | ||
def _(fut: Future): | ||
return [fut] | ||
|
||
|
||
@deep_traverse_to_unwrap.register | ||
@singledispatch | ||
def _(fut: Future): | ||
return fut.result() | ||
|
||
|
||
@deep_traverse_to_gather.register(tuple) | ||
@deep_traverse_to_gather.register(list) | ||
@deep_traverse_to_gather.register(set) | ||
def _(iterable): | ||
return [e for v in iterable for e in deep_traverse_to_gather(v) if isinstance(e, Future)] | ||
|
||
|
||
@deep_traverse_to_unwrap.register(tuple) | ||
@deep_traverse_to_unwrap.register(list) | ||
@deep_traverse_to_unwrap.register(set) | ||
@singledispatch | ||
def _(iterable): | ||
|
||
type_ = type(iterable) | ||
return type_(map(deep_traverse_to_unwrap, iterable)) | ||
|
||
|
||
@deep_traverse_to_gather.register(dict) | ||
def _(dictionary): | ||
futures = [] | ||
for key, value in dictionary.items(): | ||
if isinstance(key, Future): | ||
futures.append(key) | ||
if isinstance(value, Future): | ||
futures.append(value) | ||
return futures | ||
|
||
|
||
@deep_traverse_to_unwrap.register(dict) | ||
def _(dictionary): | ||
unwrapped_dict = {} | ||
for key, value in dictionary.items(): | ||
key = deep_traverse_to_unwrap(key) | ||
value = deep_traverse_to_unwrap(value) | ||
unwrapped_dict[key] = value | ||
return unwrapped_dict | ||
|
||
|
||
DEEP_DEPENDENCY_RESOLVER = DependencyResolver(traverse_to_gather=deep_traverse_to_gather, | ||
traverse_to_unwrap=deep_traverse_to_unwrap) | ||
|
||
SHALLOW_DEPENDENCY_RESOLVER = DependencyResolver(traverse_to_gather=shallow_traverse_to_gather, | ||
traverse_to_unwrap=shallow_traverse_to_unwrap) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ | |
from parsl.config import Config | ||
from parsl.data_provider.data_manager import DataManager | ||
from parsl.data_provider.files import File | ||
from parsl.dataflow.dependency_resolvers import SHALLOW_DEPENDENCY_RESOLVER | ||
from parsl.dataflow.errors import BadCheckpoint, DependencyError, JoinError | ||
from parsl.dataflow.futures import AppFuture | ||
from parsl.dataflow.memoization import Memoizer | ||
|
@@ -203,6 +204,9 @@ def __init__(self, config: Config) -> None: | |
self.tasks: Dict[int, TaskRecord] = {} | ||
self.submitter_lock = threading.Lock() | ||
|
||
self.dependency_resolver = self.config.dependency_resolver if self.config.dependency_resolver is not None \ | ||
else SHALLOW_DEPENDENCY_RESOLVER | ||
|
||
Comment on lines
+207
to
+209
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This implies that class Config(...):
def __init__(
...
dependency_resolver: Optional[DependencyResolver],
...
):
if dependency_resolver is None:
dependency_resolver = SHALLOW_DEPENDENCY_RESOLVER
self.dependency_resolver = dependency_resolver Mypy may complain with that particular construction and not- Functionally a wash, I think (so I won't be fussed about this), but thinking in terms of overall clarity for when someone is poking at the REPL or CLI. |
||
atexit.register(self.atexit_cleanup) | ||
|
||
def __enter__(self): | ||
|
@@ -852,8 +856,11 @@ def _gather_all_deps(self, args: Sequence[Any], kwargs: Dict[str, Any]) -> List[ | |
depends: List[Future] = [] | ||
|
||
def check_dep(d: Any) -> None: | ||
if isinstance(d, Future): | ||
depends.extend([d]) | ||
try: | ||
depends.extend(self.dependency_resolver.traverse_to_gather(d)) | ||
except Exception: | ||
logger.exception("Exception in dependency_resolver.traverse_to_gather") | ||
raise | ||
|
||
# Check the positional args | ||
for dep in args: | ||
|
@@ -905,34 +912,27 @@ def append_failure(e: Exception, dep: Future) -> None: | |
# Replace item in args | ||
new_args = [] | ||
for dep in args: | ||
if isinstance(dep, Future): | ||
try: | ||
new_args.extend([dep.result()]) | ||
except Exception as e: | ||
append_failure(e, dep) | ||
else: | ||
new_args.extend([dep]) | ||
try: | ||
new_args.extend([self.dependency_resolver.traverse_to_unwrap(dep)]) | ||
except Exception as e: | ||
append_failure(e, dep) | ||
|
||
# Check for explicit kwargs ex, fu_1=<fut> | ||
for key in kwargs: | ||
dep = kwargs[key] | ||
if isinstance(dep, Future): | ||
try: | ||
kwargs[key] = dep.result() | ||
except Exception as e: | ||
append_failure(e, dep) | ||
try: | ||
kwargs[key] = self.dependency_resolver.traverse_to_unwrap(dep) | ||
except Exception as e: | ||
append_failure(e, dep) | ||
|
||
# Check for futures in inputs=[<fut>...] | ||
if 'inputs' in kwargs: | ||
new_inputs = [] | ||
for dep in kwargs['inputs']: | ||
if isinstance(dep, Future): | ||
try: | ||
new_inputs.extend([dep.result()]) | ||
except Exception as e: | ||
append_failure(e, dep) | ||
else: | ||
new_inputs.extend([dep]) | ||
try: | ||
new_inputs.extend([self.dependency_resolver.traverse_to_unwrap(dep)]) | ||
except Exception as e: | ||
append_failure(e, dep) | ||
kwargs['inputs'] = new_inputs | ||
|
||
return new_args, kwargs, dep_failures | ||
|
@@ -1037,6 +1037,8 @@ def submit(self, | |
|
||
func = self._add_output_deps(executor, app_args, app_kwargs, app_fu, func) | ||
|
||
logger.debug("Added output dependencies") | ||
|
||
# Replace the function invocation in the TaskRecord with whatever file-staging | ||
# substitutions have been made. | ||
task_record.update({ | ||
|
@@ -1048,8 +1050,10 @@ def submit(self, | |
|
||
self.tasks[task_id] = task_record | ||
|
||
logger.debug("Gathering dependencies") | ||
# Get the list of dependencies for the task | ||
depends = self._gather_all_deps(app_args, app_kwargs) | ||
logger.debug("Gathered dependencies") | ||
task_record['depends'] = depends | ||
|
||
depend_descs = [] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This text is accurate, and points to the right place, but as a "documentation consumer," I find myself without a proper mental model for what this looks like. Would an example implementation be an undue burden to place here? Or at the end of one of the links?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working on a presentation for this for next Tuesday so I'll try to use the preparation for that as a way to get my head around more introductory material.