Skip to content

Use the new HLG pack/unpack API in Dask#4489

Merged
jrbourbeau merged 8 commits intodask:masterfrom
madsbk:hlg_pack_move_to_dask
Feb 26, 2021
Merged

Use the new HLG pack/unpack API in Dask#4489
jrbourbeau merged 8 commits intodask:masterfrom
madsbk:hlg_pack_move_to_dask

Conversation

@madsbk
Copy link
Contributor

@madsbk madsbk commented Feb 8, 2021 •
edited
Loading

This PR makes Distributed use the new high level graph pack/unpack API in Dask: dask/dask#7179

  • Tests added / passed
  • Passes black distributed / flake8 distributed

@madsbk madsbk mentioned this pull request Feb 8, 2021
Merged
3 tasks
@madsbk madsbk changed the title Use the HLG pack/unpack from Dask Use the new HLG pack/unpack API in Dask Feb 8, 2021
ian-r-rose
ian-r-rose reviewed Feb 12, 2021
View reviewed changes
Copy link
Collaborator

@ian-r-rose ian-r-rose left a comment

Choose a reason for hiding this comment

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

👍

@madsbk madsbk force-pushed the hlg_pack_move_to_dask branch from 349e26e to 2550bee Compare February 19, 2021 09:21
jrbourbeau
jrbourbeau reviewed Feb 25, 2021
View reviewed changes
Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks for your work here @madsbk! I pushed an update to run CI against dask/dask#7179 to ensure that the distributed test suite passes too.

We'll also want to update the minimum supported version of dask to make sure HighLevelGraphs have the new __dask_distributed_(un)pack__ API. Historically what we've done in this case is update distributeds requirements.txt to point to the master branch of dask and add a TODO comment to update to the latest dask release prior to releasing distributed

@madsbk madsbk force-pushed the hlg_pack_move_to_dask branch from d5718f9 to b6bb295 Compare February 25, 2021 13:54
@madsbk
Copy link
Contributor Author

madsbk commented Feb 25, 2021 •
edited
Loading

Thanks @jrbourbeau, both PRs should work now. I have added some tests of the issue in Dask and renamed anno => annotations here.

Some of the CI runs fails in test_register_backend_entrypoint, maybe related to the CI pointing to Dask PR dask/dask#7179 ?

@jrbourbeau
Copy link
Member

jrbourbeau commented Feb 26, 2021

Some of the CI runs fails in test_register_backend_entrypoint, maybe related to the CI pointing to Dask PR dask/dask#7179 ?

Yeah you're correct, that failure is be related to pointing CI to your branch. Since our versioning system, versioneer, uses commit tags to determine the package version, and your branch most recent tag is 1.2.2, we're not meeting some minimum requirement in the dask version that test relies on.

pkg_resources.VersionConflict: (dask 1.2.2+1025.g9d91e6a0 (/usr/share/miniconda3/envs/dask-distributed/lib/python3.6/site-packages), Requirement.parse('dask>=2021.02.0'))

This is okay as it's only an artifact of pointing to a fork of dask

jrbourbeau
jrbourbeau approved these changes Feb 26, 2021
View reviewed changes
Copy link
Member

@jrbourbeau jrbourbeau left a comment

Choose a reason for hiding this comment

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

Thanks @madsbk! This LGTM. See my comment at dask/dask#7179 (review) about waiting a day to merge this PR

@jrbourbeau jrbourbeau merged commit 31119c4 into dask:master Feb 26, 2021
@jrbourbeau jrbourbeau mentioned this pull request Feb 26, 2021
Merged
@madsbk madsbk deleted the hlg_pack_move_to_dask branch March 1, 2021 08:03
@jrbourbeau jrbourbeau mentioned this pull request Mar 5, 2021
Merged
@jakirkham jakirkham mentioned this pull request Mar 8, 2021
Closed
@bnaul bnaul mentioned this pull request Apr 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ian-r-rose ian-r-rose ian-r-rose left review comments

@jrbourbeau jrbourbeau jrbourbeau approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@madsbk @jrbourbeau @ian-r-rose