Skip to content

Support futures in from_delayed#1961

Merged
mrocklin merged 7 commits intodask:masterfrom
mrocklin:from-delayed-futures
Feb 12, 2017
Merged

Support futures in from_delayed#1961
mrocklin merged 7 commits intodask:masterfrom
mrocklin:from-delayed-futures

Conversation

@mrocklin
Copy link
Copy Markdown
Member

@mrocklin mrocklin commented Feb 3, 2017

This is a bit wonky in that I had to compute before calling assert_eq.

Still this is a common tripping point.

cc @jcrist for review

@mrocklin mrocklin force-pushed the from-delayed-futures branch from 14fc657 to b16c2e2 Compare February 5, 2017 17:28
@mrocklin
Copy link
Copy Markdown
Member Author

mrocklin commented Feb 6, 2017

cc @jcrist can I get a quick yes/no on if you agree with this change?

Copy link
Copy Markdown
Member

@jcrist jcrist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this seems fine to me.

from dask.delayed import Delayed
from dask.delayed import Delayed, delayed
if not isinstance(value, Delayed) and hasattr(value, 'key'):
value = delayed(value)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the check for 'key' here but not elsewhere (I assume this is to duck-type on futures)?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now added the check for key elsewhere

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mrocklin mrocklin merged commit c53ad8c into dask:master Feb 12, 2017
@mrocklin mrocklin deleted the from-delayed-futures branch February 12, 2017 16:33
@sinhrks sinhrks added this to the 0.14.0 milestone Mar 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants