Skip to content
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

Can we demistify the order of @annotate and @delayed? #10410

Closed
crusaderky opened this issue Jul 13, 2023 · 4 comments · Fixed by dask/distributed#8120
Closed

Can we demistify the order of @annotate and @delayed? #10410

crusaderky opened this issue Jul 13, 2023 · 4 comments · Fixed by dask/distributed#8120
Labels
discussion Discussing a topic with no specific actions yet

Comments

@crusaderky
Copy link
Collaborator

Consider the following:

@annotate(resources={"GPU": 1})
@delayed
def f():
    a = ...  # define dask collection
    distributed.secede()
    return a.compute()

@delayed
@annotate(resources={"GPU": 1})
def g():
    a = ...  # define dask collection
    distributed.secede()
    return a.compute()

Both of the examples below look very elegant, and both make sense in certain conditions, but are fundamentally different:

  • f() will run on a worker with GPU, but the tasks it spawns won't
  • g() will run on any worker, and all the tasks it spawns will require a GPU

It's very hard, for a novice dask user, to understand the difference between the two.
Is there anything we can do to mitigate this problem?

@github-actions github-actions bot added the needs triage Needs a response from a contributor label Jul 13, 2023
@fjetter fjetter added discussion Discussing a topic with no specific actions yet and removed needs triage Needs a response from a contributor labels Jul 20, 2023
@benrutter
Copy link
Contributor

Just to chip in on this discussion- I agree it's unclear to a novice, but I'm not sure it isn't just a consequence of Python decorators which often seem to cause confusion like this.

Is a user likely to intend f() over g()? I think there are probably some legitimate use cases, but if there weren't or if there were relatively few, dask could raise a warning in the case of f?

@fjetter
Copy link
Member

fjetter commented Aug 15, 2023

I think we're not advertising (nor testing) annotate as a decorator anywhere. The documentation defines it as a contextmanager https://docs.dask.org/en/stable/api.html#dask.annotate and I couldn't find any references to a decorator anywhere.

The fact that this works is more or less an implementation detail (it is using contextlib.contextmanager which defines the decorated function in a way that works for both) and I wouldn't even want to guarantee that this is something we'll keep supporting in the future (e.g. annotate could become a simple class implementing __enter__ and __exit__ such that this is not even possible any longer)

Given the limitations that are imposed by the language, I'll go ahead and close this as a won't fix.

@fjetter fjetter closed this as not planned Won't fix, can't repro, duplicate, stale Aug 15, 2023
@crusaderky
Copy link
Collaborator Author

crusaderky commented Aug 15, 2023

I couldn't find any references to a decorator anywhere.

span is a thin wrapper around annotate, and is advertised as a possible decorator:
https://distributed.dask.org/en/stable/spans.html

@fjetter
Copy link
Member

fjetter commented Aug 15, 2023

span is a thin wrapper around annotate, and is advertised as a possible decorator:

In dask/distributed#7954 (comment) I'm suggesting to remove the decorator for spans since the semantics are not straight forward. This issue is reinforcing this opinion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussing a topic with no specific actions yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants