-
-
Notifications
You must be signed in to change notification settings - Fork 720
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
Dynamic annotations #5207
Draft
madsbk
wants to merge
7
commits into
dask:main
Choose a base branch
from
madsbk:dynamic_annotations
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Dynamic annotations #5207
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
dae7d59
Clean up and type hints
madsbk 1ea3725
Implemented annotators
madsbk ee5374e
Merge branch 'main' of github.com:dask/distributed into dynamic_annot…
madsbk 682e3d1
Merge branch 'main' into dynamic_annotations
madsbk 8ad9cee
Redesign annotator functions
madsbk da122c3
Fix tests
madsbk 7494179
annotate_task(): don't access private attributes
madsbk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,7 @@ | |
from datetime import timedelta | ||
from inspect import isawaitable | ||
from pickle import PicklingError | ||
from typing import Dict, Hashable, Iterable, Optional | ||
from typing import Callable, Dict, Hashable, Iterable, Mapping, Optional | ||
|
||
from tlz import first, keymap, merge, pluck # noqa: F401 | ||
from tornado.ioloop import IOLoop, PeriodicCallback | ||
|
@@ -174,7 +174,7 @@ def __init__(self, key, runspec=None): | |
self.who_has = set() | ||
self.coming_from = None | ||
self.waiting_for_data = set() | ||
self.resource_restrictions = None | ||
self.resource_restrictions = {} | ||
self.exception = None | ||
self.exception_text = "" | ||
self.traceback = None | ||
|
@@ -186,7 +186,7 @@ def __init__(self, key, runspec=None): | |
self.stop_time = None | ||
self.metadata = {} | ||
self.nbytes = None | ||
self.annotations = None | ||
self.annotations = {} | ||
self.scheduler_holds_ref = False | ||
|
||
def __repr__(self): | ||
|
@@ -352,6 +352,10 @@ class Worker(ServerNode): | |
lifetime_restart: bool | ||
Whether or not to restart a worker after it has reached its lifetime | ||
Default False | ||
annotators: Iterable[Callable[[TaskState, object], bool]], optional | ||
List of annotators, which are functions that given a `TaskState` and task | ||
output modifies the `TaskState` and return whether any changes was made to | ||
the `TaskState` or not. | ||
|
||
Examples | ||
-------- | ||
|
@@ -418,9 +422,10 @@ def __init__( | |
lifetime=None, | ||
lifetime_stagger=None, | ||
lifetime_restart=None, | ||
annotators=None, | ||
**kwargs, | ||
): | ||
self.tasks = dict() | ||
self.tasks: Dict[str, TaskState] = dict() | ||
self.waiting_for_data_count = 0 | ||
self.has_what = defaultdict(set) | ||
self.pending_data_per_worker = defaultdict(deque) | ||
|
@@ -687,6 +692,10 @@ def __init__( | |
|
||
self.low_level_profiler = low_level_profiler | ||
|
||
self.annotators: Iterable[Callable[[TaskState, object], Mapping]] = ( | ||
annotators or () | ||
) | ||
|
||
handlers = { | ||
"gather": self.gather, | ||
"run": self.run, | ||
|
@@ -1496,11 +1505,15 @@ def update_data(self, comm=None, data=None, report=True, serializers=None): | |
self.log.append((key, "receive-from-scatter")) | ||
|
||
if report: | ||
|
||
self.log.append( | ||
("Notifying scheduler about in-memory in update-data", list(data)) | ||
) | ||
self.batched_stream.send({"op": "add-keys", "keys": list(data)}) | ||
|
||
if self.annotators: | ||
for key, value in data.items(): | ||
self.handle_annotators(self.tasks[key], value) | ||
|
||
info = {"nbytes": {k: sizeof(v) for k, v in data.items()}, "status": "OK"} | ||
return info | ||
|
||
|
@@ -1646,7 +1659,7 @@ def add_task( | |
ts.duration = duration | ||
if resource_restrictions: | ||
ts.resource_restrictions = resource_restrictions | ||
ts.annotations = annotations | ||
ts.annotations = annotations or {} | ||
|
||
who_has = who_has or {} | ||
|
||
|
@@ -1896,6 +1909,7 @@ def transition_flight_memory(self, ts, value=None): | |
|
||
self.log.append(("Notifying scheduler about in-memory", ts.key)) | ||
self.batched_stream.send({"op": "add-keys", "keys": [ts.key]}) | ||
self.handle_annotators(ts, value) | ||
|
||
except Exception as e: | ||
logger.exception(e) | ||
|
@@ -1919,7 +1933,7 @@ def transition_waiting_ready(self, ts): | |
|
||
self.has_what[self.address].discard(ts.key) | ||
|
||
if ts.resource_restrictions is not None: | ||
if ts.resource_restrictions: | ||
self.constrained.append(ts.key) | ||
return "constrained" | ||
else: | ||
|
@@ -1942,6 +1956,7 @@ def transition_waiting_done(self, ts, value=None): | |
ts.waiting_for_data.clear() | ||
if value is not None: | ||
self.put_key_in_memory(ts, value) | ||
self.handle_annotators(ts, value) | ||
self.send_task_state_to_scheduler(ts) | ||
except Exception as e: | ||
logger.exception(e) | ||
|
@@ -2002,9 +2017,8 @@ def transition_executing_done(self, ts, value=no_value, report=True): | |
assert ts.key not in self.ready | ||
|
||
out = None | ||
if ts.resource_restrictions is not None: | ||
for resource, quantity in ts.resource_restrictions.items(): | ||
self.available_resources[resource] += quantity | ||
for resource, quantity in ts.resource_restrictions.items(): | ||
self.available_resources[resource] += quantity | ||
|
||
if ts.state == "executing": | ||
self.executing_count -= 1 | ||
|
@@ -2027,11 +2041,12 @@ def transition_executing_done(self, ts, value=no_value, report=True): | |
for d in ts.dependents: | ||
d.waiting_for_data.add(ts.key) | ||
|
||
if report and self.batched_stream and self.status == Status.running: | ||
self.send_task_state_to_scheduler(ts) | ||
if self.batched_stream and self.status == Status.running: | ||
self.handle_annotators(ts, value) | ||
if report: | ||
self.send_task_state_to_scheduler(ts) | ||
else: | ||
raise CommClosedError | ||
|
||
return out | ||
|
||
except OSError: | ||
|
@@ -2212,7 +2227,7 @@ def ensure_communicating(self): | |
pdb.set_trace() | ||
raise | ||
|
||
def send_task_state_to_scheduler(self, ts): | ||
def send_task_state_to_scheduler(self, ts: TaskState): | ||
if ts.key in self.data or self.actors.get(ts.key): | ||
typ = ts.type | ||
if ts.nbytes is None or typ is None: | ||
|
@@ -2292,6 +2307,48 @@ def put_key_in_memory(self, ts, value, transition=True): | |
|
||
self.log.append((ts.key, "put-in-memory")) | ||
|
||
def handle_annotators( | ||
self, | ||
ts: TaskState, | ||
value: object = no_value, | ||
): | ||
"""Annotate task if applicable and report back to the scheduler""" | ||
|
||
# Separate annotation updates into two cases: | ||
a1 = {} # when the task's dependents should also be updated | ||
a2 = {} # when only the task itself should be updated | ||
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. Do you have thoughts on better names here? 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. Heh, yeah I can do that :) |
||
for func in self.annotators: | ||
try: | ||
res = func(ts, value) | ||
except Exception as e: | ||
logger.exception(e) | ||
else: | ||
if res: | ||
if res["dependents"]: | ||
a1.update(res["annotations"]) | ||
else: | ||
a2.update(res["annotations"]) | ||
if a1: | ||
ts.annotations.update(a1) | ||
self.batched_stream.send( | ||
{ | ||
"op": "annotate-task", | ||
"key": ts.key, | ||
"annotations": a1, | ||
"dependents": True, | ||
} | ||
) | ||
if a2: | ||
ts.annotations.update(a2) | ||
self.batched_stream.send( | ||
{ | ||
"op": "annotate-task", | ||
"key": ts.key, | ||
"annotations": a2, | ||
"dependents": False, | ||
} | ||
) | ||
|
||
def select_keys_for_gather(self, worker, dep): | ||
assert isinstance(dep, str) | ||
deps = {dep} | ||
|
@@ -2851,12 +2908,9 @@ def actor_attribute(self, comm=None, actor=None, attribute=None): | |
|
||
def meets_resource_constraints(self, key): | ||
ts = self.tasks[key] | ||
if not ts.resource_restrictions: | ||
return True | ||
for resource, needed in ts.resource_restrictions.items(): | ||
if self.available_resources[resource] < needed: | ||
return False | ||
|
||
return True | ||
|
||
async def _maybe_deserialize_task(self, ts): | ||
|
@@ -2943,10 +2997,7 @@ async def execute(self, key): | |
|
||
args2, kwargs2 = self._prepare_args_for_execution(ts, args, kwargs) | ||
|
||
if ts.annotations is not None and "executor" in ts.annotations: | ||
executor = ts.annotations["executor"] | ||
else: | ||
executor = "default" | ||
executor = ts.annotations.get("executor", "default") | ||
assert executor in self.executors | ||
assert key == ts.key | ||
self.active_keys.add(ts.key) | ||
|
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
The fact that you had to use get/compute here is intersting. There are interesting challenges with doing this with futures / dependencies that might change. I still think that this is a useful feature to have, but it seems like this approach might not be comprehensive if we want to solve things across update_graph calls. Agree or disagree?
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.
Agree! I hadn't thought about this issue before today. As you say, it is still useful but I think we should put this PR on hold for a bit and see if we can come up with a better approach.