Skip to content

Conversation

@sjperkins
Copy link
Member

No description provided.

@sjperkins sjperkins marked this pull request as draft November 25, 2020 09:07
@sjperkins
Copy link
Member Author

Depends on dask/dask#6889

@sjperkins sjperkins force-pushed the layer-annotations-to-scheduler branch from 19b83ad to dadc7ac Compare November 25, 2020 13:21
@mrocklin
Copy link
Member

I have an alternative here: #4288

It's not quite as comprehensive (annotations other than priority/retires/workers don't get transmitted) but it's simple and this approach covers what we can do today I think.

@sjperkins sjperkins marked this pull request as ready for review December 1, 2020 16:31
Copy link
Member

@mrocklin mrocklin left a comment

Choose a reason for hiding this comment

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

A few comments. Some are somewhat pedantic. I hope that that is ok.

@sjperkins
Copy link
Member Author

A few comments. Some are somewhat pedantic. I hope that that is ok.

No complaints here. The above makes sense to me and I think I understand where you're coming from, as I've been labelled "fussy about how things are done" upon occasion. ;-)

@mrocklin
Copy link
Member

mrocklin commented Dec 2, 2020

This all seems good to me. I'm happy to merge.

For future work (or now, if you feel like you're on a roll) we might consider assigning the annotations to the TaskState objects, such that the following might work:

>>> ts.annotations
{"foo": 1}

The test failure here seems unrelated. I'll push up a separate PR with a fix.

@sjperkins
Copy link
Member Author

For future work (or now, if you feel like you're on a roll) we might consider assigning the annotations to the TaskState objects, such that the following might work:

>>> ts.annotations
{"foo": 1}

Done.

One of the future additions I'm considering is transmitting annotations to the workers, but I haven't looked into any of the details yet. I think it would be good to leave that to the scope of a new PR.

mrocklin pushed a commit to dask/dask that referenced this pull request Dec 3, 2020
@mrocklin mrocklin merged commit dcdb465 into dask:master Dec 3, 2020
@sjperkins sjperkins deleted the layer-annotations-to-scheduler branch December 3, 2020 17:48
@jakirkham
Copy link
Member

It looks like this checked an empty file, distributed/tests/test_highgraph.py. Is that needed for some reason or should we drop that?

@jrbourbeau
Copy link
Member

Thanks for catching that, I believe it can be dropped

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.

4 participants